pgsql: Don't consider newly inserted tuples in nbtree VACUUM.

Don't consider newly inserted tuples in nbtree VACUUM.

Remove the entire idea of "stale stats" within nbtree VACUUM (stop
caring about stats involving the number of inserted tuples). Also
remove the vacuum_cleanup_index_scale_factor GUC/param on the master
branch (though just disable them on postgres 13).

The vacuum_cleanup_index_scale_factor/stats interface made the nbtree AM
partially responsible for deciding when pg_class.reltuples stats needed
to be updated. This seems contrary to the spirit of the index AM API,
though -- it is not actually necessary for an index AM's bulk delete and
cleanup callbacks to provide accurate stats when it happens to be
inconvenient. The core code owns that. (Index AMs have the authority
to perform or not perform certain kinds of deferred cleanup based on
their own considerations, such as page deletion and recycling, but that
has little to do with pg_class.reltuples/num_index_tuples.)

This issue was fairly harmless until the introduction of the
autovacuum_vacuum_insert_threshold feature by commit b07642db, which had
an undesirable interaction with the vacuum_cleanup_index_scale_factor
mechanism: it made insert-driven autovacuums perform full index scans,
even though there is no real benefit to doing so. This has been tied to
a regression with an append-only insert benchmark [1].

Also have remaining cases that perform a full scan of an index during a
cleanup-only nbtree VACUUM indicate that the final tuple count is only
an estimate. This prevents vacuumlazy.c from setting the index's
pg_class.reltuples in those cases (it will now only update pg_class when
vacuumlazy.c had TIDs for nbtree to bulk delete). This arguably fixes
an oversight in deduplication-related bugfix commit 48e12913.

[1] https://smalldatum.blogspot.com/2021/01/insert-benchmark-postgres-is-still.html

Author: Peter Geoghegan <pg(at)bowt(dot)ie>
Reviewed-By: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Discussion: https://postgr.es/m/CAD21AoA4WHthN5uU6+WScZ7+J_RcEjmcuH94qcoUPuB42ShXzg@mail.gmail.com
Backpatch: 13-, where autovacuum_vacuum_insert_threshold was added.

Branch
------
REL_13_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/9663d124466f09696170631a2f2c0b40114166c8

Modified Files
--------------
doc/src/sgml/config.sgml | 41 ------------
doc/src/sgml/ref/create_index.sgml | 14 ----
src/backend/access/nbtree/nbtpage.c | 22 +++----
src/backend/access/nbtree/nbtree.c | 102 +++++++++++++++---------------
src/test/regress/expected/btree_index.out | 29 ---------
src/test/regress/sql/btree_index.sql | 19 ------
6 files changed, 60 insertions(+), 167 deletions(-)

Hi Peter,

On Thu, Mar 11, 2021 at 12:27:25AM +0000, Peter Geoghegan wrote:
> The vacuum_cleanup_index_scale_factor/stats interface made the nbtree AM
> partially responsible for deciding when pg_class.reltuples stats needed
> to be updated. This seems contrary to the spirit of the index AM API,
> though -- it is not actually necessary for an index AM's bulk delete and
> cleanup callbacks to provide accurate stats when it happens to be
> inconvenient. The core code owns that. (Index AMs have the authority
> to perform or not perform certain kinds of deferred cleanup based on
> their own considerations, such as page deletion and recycling, but that
> has little to do with pg_class.reltuples/num_index_tuples.)

I think that this commit has some issues that need more thoughts.

The removal of vacuum_cleanup_index_scale_factor means that any
existing deployment of Postgres that includes at least one index using
this parameter would fail in the middle of the restore during
pg_upgrade, when restoring the binary dump. It seems to me that it is
user-unfriendly to complain in the middle of an upgrade, as it could
take time before being detected. In my opinion, it would be better to
do something else. Here are three possibilities:
1) Make pg_upgrade check at its beginning if any databases to upgrade
include an index with this parameter set.
2) Tweak the upgrade/restore to ignore this parameter if set.
3) Let the option live in the existing set of relation options, but
just ignore it in the backend code.

On top of that, you have a second issue here: a pg_dump from 14
dumping from an older server would also happily dump any indexes with
this parameter set, making the dump incompatible with 14.

Thanks,
--
Michael

On Wed, Mar 10, 2021 at 10:57 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> I think that this commit has some issues that need more thoughts.
>
> The removal of vacuum_cleanup_index_scale_factor means that any
> existing deployment of Postgres that includes at least one index using
> this parameter would fail in the middle of the restore during
> pg_upgrade, when restoring the binary dump.

I don't believe that it's necessary. Partly because it seems rather
unlikely that vacuum_cleanup_index_scale_factor was ever set like this
in practice.

You could have made the same arguments against removing
recheck_on_update in commit 1c53c4de.

--
Peter Geoghegan

On 2021-Mar-10, Peter Geoghegan wrote:

> On Wed, Mar 10, 2021 at 10:57 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > I think that this commit has some issues that need more thoughts.
> >
> > The removal of vacuum_cleanup_index_scale_factor means that any
> > existing deployment of Postgres that includes at least one index using
> > this parameter would fail in the middle of the restore during
> > pg_upgrade, when restoring the binary dump.
>
> I don't believe that it's necessary. Partly because it seems rather
> unlikely that vacuum_cleanup_index_scale_factor was ever set like this
> in practice.

I disagree; that GUC was a feature in its own right, and it seems likely
that people have set it in the hopes that it'd help them, even if it
didn't actually achieve that.

The GUC was added in April 2018 (pg11) and it's been possible to set it
on all releases since 11.0. That's a long time.

> You could have made the same arguments against removing
> recheck_on_update in commit 1c53c4de.

recheck_on_update was born on 11.0 and killed in time for 11.1, so its
opportunity to become set was narrower.

--
Álvaro Herrera 39°49'30"S 73°17'W
"La victoria es para quien se atreve a estar solo"

On Thu, Mar 11, 2021 at 8:13 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> I disagree; that GUC was a feature in its own right, and it seems likely
> that people have set it in the hopes that it'd help them, even if it
> didn't actually achieve that.

Michael was talking about the reloption, not the GUC. Surely the GUC
is not the problem here - there is plenty of precedent for removing
GUCs that were around for many releases, without considering
compatibility.

> > You could have made the same arguments against removing
> > recheck_on_update in commit 1c53c4de.
>
> recheck_on_update was born on 11.0 and killed in time for 11.1, so its
> opportunity to become set was narrower.

New reloptions are added infrequently. And they're practically never
removed. So recheck_on_update seems to be the closest thing to a
precedent.

I now accept that the dump/reload hazards when upgrading to 14 are not
acceptable. ISTM that adding back the reloption on master/Postgres 14
(without doing anything with it) is the only way to fix everything.

Any thoughts on that plan? I can do that in the next few hours if
there are no objections.

--
Peter Geoghegan

Peter Geoghegan <pg(at)bowt(dot)ie> writes:
> I now accept that the dump/reload hazards when upgrading to 14 are not
> acceptable. ISTM that adding back the reloption on master/Postgres 14
> (without doing anything with it) is the only way to fix everything.

Agreed, just keep it as a no-op. Not sure whether there should be
a documentation entry for it, but I bet we get questions if there
isn't.

regards, tom lane

On Thu, Mar 11, 2021 at 9:03 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Agreed, just keep it as a no-op. Not sure whether there should be
> a documentation entry for it, but I bet we get questions if there
> isn't.

I won't be adding back the psql completion stuff. Why can't adding
back the reloption have the desired effect of avoiding issues with
pg_upgrade and pg_restore, while also leaving the reloption
practically invisible to users?

Naturally I would prefer to keep cruft about an inert reloption out of
the documentation.
--
Peter Geoghegan

On Thu, Mar 11, 2021 at 1:57 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> I think that this commit has some issues that need more thoughts.

My biggest issue with this commit is that I can't understand from the
commit message why it's a bug fix that deserves to be back-patched. To
be honest, I can't even really understand whether it's a good idea in
master.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Mar 11, 2021 at 11:13 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Mar 11, 2021 at 1:57 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> > I think that this commit has some issues that need more thoughts.
>
> My biggest issue with this commit is that I can't understand from the
> commit message why it's a bug fix that deserves to be back-patched. To
> be honest, I can't even really understand whether it's a good idea in
> master.

It is convoluted. The issue that led to my backpatching to 13 was tied
to a 4% - 16% regression in an append-only workload by Mark Callaghan
(he is well known as the main person behind MyRocks at Facebook):

https://smalldatum.blogspot.com/2021/01/insert-benchmark-postgres-is-still.html

Autovacuums driven by autovacuum_vacuum_insert_threshold scan indexes
without receiving any benefit for it. Clearly
vacuum_cleanup_index_scale_factor was misdesigned, but that only
became a real problem with autovacuum_vacuum_insert_threshold. That
did not seem acceptable to me. It'll get worse and worse as you add
more and more indexes.

--
Peter Geoghegan

On Thu, Mar 11, 2021 at 2:22 PM Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> It is convoluted. The issue that led to my backpatching to 13 was tied
> to a 4% - 16% regression in an append-only workload by Mark Callaghan
> (he is well known as the main person behind MyRocks at Facebook):
>
> https://smalldatum.blogspot.com/2021/01/insert-benchmark-postgres-is-still.html
>
> Autovacuums driven by autovacuum_vacuum_insert_threshold scan indexes
> without receiving any benefit for it. Clearly
> vacuum_cleanup_index_scale_factor was misdesigned, but that only
> became a real problem with autovacuum_vacuum_insert_threshold. That
> did not seem acceptable to me. It'll get worse and worse as you add
> more and more indexes.

I can understand that those two settings might interact in some way
that is bad or unintended, but I feel like if I can't understand what
exactly the bad interaction is after reading the commit message,
that's probably a sign that the commit message isn't as clear as it
could be.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Thu, Mar 11, 2021 at 11:25 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I can understand that those two settings might interact in some way
> that is bad or unintended, but I feel like if I can't understand what
> exactly the bad interaction is after reading the commit message,
> that's probably a sign that the commit message isn't as clear as it
> could be.

You have to think about it from first principles, I suppose.

What was the point of vacuum_cleanup_index_scale_factor in the first
place, back when it was committed to Postgres 11? It was something
like "have btvacuumcleanup() make sure that pg_class.reltuples gets
updated when most full scans during nbtree VACUUM get skipped". But
that's fundamentally not the responsibility of the index AM. As a
counterpoint, hash indexes simply return NULL when there is a
cleanup-only VACUUM (i.e. no hashbulkdelete() call for the VACUUM
operation), without worrying about pg_class being updated for the
index entry on any particular timeline. Because why would an index AM
ever need to worry about that? Index AMs are not supposed to concern
themselves with that in the case where the index hasn't been modified,
per the amvacuumcleanup() sgml docs.

This confusion over which component is responsible for maintaining
pg_class.reltuples makes vacuum_cleanup_index_scale_factor-driven
autovacuums uselessly perform full scans of all indexes on an
append-only table. In my judgement that was a bug that needed to be
fixed. Certainly, disabling a GUC in a stable release branch was an
unorthodox approach, but I concluded that it was the lesser evil here.
Scanning the indexes during a VACUUM driven by
vacuum_cleanup_index_scale_factor cannot be dismissed as the cost of
setting the VM bits for the end of the heap relation, because it has
no fixed relationship to what has changed -- it generally makes zero
sense to scan the indexes here.

--
Peter Geoghegan

On 2021-Mar-11, Peter Geoghegan wrote:

> On Thu, Mar 11, 2021 at 8:13 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > I disagree; that GUC was a feature in its own right, and it seems likely
> > that people have set it in the hopes that it'd help them, even if it
> > didn't actually achieve that.
>
> Michael was talking about the reloption, not the GUC. Surely the GUC
> is not the problem here - there is plenty of precedent for removing
> GUCs that were around for many releases, without considering
> compatibility.

Sorry, yes, I meant the reloption. I agree we don't care about the GUC.

> > > You could have made the same arguments against removing
> > > recheck_on_update in commit 1c53c4de.
> >
> > recheck_on_update was born on 11.0 and killed in time for 11.1, so its
> > opportunity to become set was narrower.
>
> New reloptions are added infrequently. And they're practically never
> removed. So recheck_on_update seems to be the closest thing to a
> precedent.

Given the short life of recheck_on_update I don't think we should
consider it a precedent.

> I now accept that the dump/reload hazards when upgrading to 14 are not
> acceptable. ISTM that adding back the reloption on master/Postgres 14
> (without doing anything with it) is the only way to fix everything.

Yeah, I agree. It would be slightly better to ignore it on input (ie.
make ALTER TABLE SET a no-op for that case), but I don't think we have
code to do that.

> Any thoughts on that plan? I can do that in the next few hours if
> there are no objections.

Sounds good to me.

--
Álvaro Herrera 39°49'30"S 73°17'W

On Thu, Mar 11, 2021 at 12:39 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> Given the short life of recheck_on_update I don't think we should
> consider it a precedent.

Now we have a useful precedent. Apparently there is no way to truly
remove a storage parameter. That seems a little annoying, but not
worth spending too much time on.

> > I now accept that the dump/reload hazards when upgrading to 14 are not
> > acceptable. ISTM that adding back the reloption on master/Postgres 14
> > (without doing anything with it) is the only way to fix everything.
>
> Yeah, I agree. It would be slightly better to ignore it on input (ie.
> make ALTER TABLE SET a no-op for that case), but I don't think we have
> code to do that.

That would be better, certainly.

> > Any thoughts on that plan? I can do that in the next few hours if
> > there are no objections.
>
> Sounds good to me.

I just pushed a fix that does that. In addition, I reverted the
changes to REL_11_STABLE and REL_12_STABLE, meaning that crake will go
back to testing

Thanks
--
Peter Geoghegan

On 2021-Mar-11, Peter Geoghegan wrote:

> On Thu, Mar 11, 2021 at 12:39 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> > Given the short life of recheck_on_update I don't think we should
> > consider it a precedent.
>
> Now we have a useful precedent. Apparently there is no way to truly
> remove a storage parameter.

Well, if anybody would write this code:

> > [...] It would be slightly better to ignore it on input (ie. make
> > ALTER TABLE SET a no-op for that case), but I don't think we have
> > code to do that.
>
> That would be better, certainly.

then we could wait until the last release that supported the parameter
goes out of support. (So if we had decided to keep recheck_on_update in
pg11, we could remove it in pg17).

> That seems a little annoying, but not worth spending too much time on.

I guess it'd require that somebody is sufficiently annoyed about the few
lines that supporting a no-op reloption requires. Considering that we
haven't even got the code to release HEAP_MOVED_IN/HEAP_MOVED_OFF, which
has been theoretically possible for ages, I'm not holding my breath.

--
Álvaro Herrera Valdivia, Chile