Removing vacuum_cleanup_index_scale_factor
Hi all,
I've started this new thread separated from the thread[1]/messages/by-id/CAH2-WznUWHOL+nYYT2PLsn+3OWcq8OBfA1sB3FX885rE=ZQVvA@mail.gmail.com to discuss
removing vacuum_cleanup_index_scale_factor GUC parameter proposed by
Peter Geoghegan.
btvacuumcleanup() has been playing two roles: recycling deleted pages
and collecting index statistics. This discussion focuses on the
latter. Since PG11, btvacuumcleanup() uses
vacuum_cleanup_index_scale_factor as a threshold to do an index scan
to update index statistics (pg_class.reltuples and pg_class.relpages).
Therefore, even if there is no update on the btree index at all (e.g.,
btbulkdelete() was not called earlier), btvacuumcleanup() scans the
whole index to collect the index statistics if the number of newly
inserted tuples exceeds the vacuum_cleanup_index_scale_factor fraction
of the total number of heap tuples detected by the previous statistics
collection. On the other hand, those index statistics are updated also
by ANALYZE and autoanalyze. pg_class.reltuples calculated by ANALYZE
is an estimation whereas the value returned by btvacuumcleanup() is an
accurate value. But perhaps we can rely on ANALYZE and autoanalyze to
update those index statistics. The points of this discussion are what
we really need to do in btvacuumcleanup() and whether
btvacuumcleanup() really needs to do an index scan for the purpose of
index statistics update.
The original design that made VACUUM set
pg_class.reltuples/pg_class.relpages in indexes (from 15+ years ago)
assumed that it was cheap to handle statistics in passing. Even if we
have btvacuumcleanup() not do an index scan at all, this is 100%
allowed by the amvacuumcleanup contract described in the
documentation:
"It is OK to return NULL if the index was not changed at all during
the VACUUM operation, but otherwise correct stats should be returned."
The above description was added by commit e57345975cf in 2006 and
hasn't changed for now.
For instance, looking at hash indexes, it hasn't really changed since
2006 in terms of amvacuumcleanup(). hashvacuumcleanup() simply sets
stats->num_pages and stats->num_index_tuples without an index scan.
I'd like to quote the in-depth analysis by Peter Geoghegan:
-----
/*
* Post-VACUUM cleanup.
*
* Result: a palloc'd struct containing statistical info for VACUUM displays.
*/
IndexBulkDeleteResult *
hashvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats)
{
Relation rel = info->index;
BlockNumber num_pages;
/* If hashbulkdelete wasn't called, return NULL signifying no change */
/* Note: this covers the analyze_only case too */
if (stats == NULL)
return NULL;
/* update statistics */
num_pages = RelationGetNumberOfBlocks(rel);
stats->num_pages = num_pages;
return stats;
}
Clearly hashvacuumcleanup() was considered by Tom when he revised the
documentation in 2006. Here are some observations about
hashvacuumcleanup() that seem relevant now:
* There is no "analyze_only" handling, just like nbtree.
"analyze_only" is only used by GIN, even now, 15+ years after it was
added. GIN uses it to make autovacuum workers (never VACUUM outside of
an AV worker) do pending list insertions for ANALYZE -- just to make
it happen more often. This is a niche thing -- clearly we don't have
to care about it in nbtree, even if we make btvacuumcleanup() (almost)
always return NULL when there was no btbulkdelete() call.
* num_pages (which will become pg_class.relpages for the index) is not
set when we return NULL -- hashvacuumcleanup() assumes that ANALYZE
will get to it eventually in the case where VACUUM does no real work
(when it just returns NULL).
* We also use RelationGetNumberOfBlocks() to set pg_class.relpages for
index relations during ANALYZE -- it's called when we call
vac_update_relstats() (I quoted this do_analyze_rel() code to you
directly in a recent email).
* In general, pg_class.relpages isn't an estimate (because we use
RelationGetNumberOfBlocks(), both in the VACUUM-updates case and the
ANALYZE-updates case) -- only pg_class.reltuples is truly an estimate
during ANALYZE, and so getting a "true count" seems to have only
limited practical importance.
I think that this sets a precedent in support of my view that we can
simply get rid of vacuum_cleanup_index_scale_factor without any
special effort to maintain pg_class.reltuples. As I said before, we
can safely make btvacuumcleanup() just like hashvacuumcleanup(),
except when there are known deleted-but-not-recycled pages, where a
full index scan really is necessary for reasons that are not related
to statistics at all (of course we still need the *logic* that was
added to nbtree by the vacuum_cleanup_index_scale_factor commit --
that is clearly necessary). My guess is that Tom would have made
btvacuumcleanup() look identical to hashvacuumcleanup() in 2006 if
nbtree didn't have page deletion to consider -- but that had to be
considered.
-----
The above discussions make sense to me as a support for the "removing
vacuum_cleanup_index_scale_factor GUC" proposal. The difference
between the index statistics taken by ANALYZE and btvacuumcleanup() is
that the former statistics is always an estimation. That’s calculated
by compute_index_stats() whereas the latter uses the result of an
index scan. If btvacuumcleanup() doesn’t scan the index and always
returns NULL, it would become hard to collect accurate index
statistics, for example in a static table case. But if collecting an
accurate pg_class.reltuples is not important in practice, I agree that
we don't need btvacuumcleanup() to do an index scan for collecting
statistics purposes.
What do you think about removing vacuum_cleanup_index_scale_factor
parameter? and we'd like to ask the design principles of
amvacuumcleanup() considered in 2006 by various hackers (mostly Tom).
What do you think, Tom?
Regards,
[1]: /messages/by-id/CAH2-WznUWHOL+nYYT2PLsn+3OWcq8OBfA1sB3FX885rE=ZQVvA@mail.gmail.com
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Mon, Mar 1, 2021 at 10:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
The original design that made VACUUM set
pg_class.reltuples/pg_class.relpages in indexes (from 15+ years ago)
assumed that it was cheap to handle statistics in passing. Even if we
have btvacuumcleanup() not do an index scan at all, this is 100%
allowed by the amvacuumcleanup contract described in the
documentation:"It is OK to return NULL if the index was not changed at all during
the VACUUM operation, but otherwise correct stats should be returned."The above description was added by commit e57345975cf in 2006 and
hasn't changed for now.
The intention here is not to revise the amvacuumcleanup() contract --
the contract already allows us to do what we want inside nbtree. We
want to teach btvacuumcleanup() to not do any real work, at least
outside of rare cases where we have known deleted pages that must
still be placed in the FSM for recycling -- btvacuumcleanup() would
generally just return NULL when there was no btbulkdelete() call
during the same VACUUM operation (the main thing that prevents this
today is vacuum_cleanup_index_scale_factor). More generally, we would
like to change the *general* expectations that we make of index AMs in
places like vacuumlazy.c and analyze.c. But we're worried about
dependencies that aren't formalized anywhere but still matter -- code
may have evolved to assume that index AMs behaved a certain way in
common and important cases (probably also in code like vacuumlazy.c).
That's what we want to avoid breaking.
Masahiko has already given an example of such a problem: currently,
VACUUM ANALYZE simply assumes that its VACUUM will call each indexes'
amvacuumcleanup() routine in all cases, and will have each call set
pg_class.reltuples and pg_class.relpages in respect of each index.
ANALYZE therefore avoids overwriting indexes' pg_class stats inside
do_analyze_rel() (at the point where it calls vac_update_relstats()
for each index). That approach is already wrong with hash indexes, but
under this new directly for btvacuumcleanup(), it would become wrong
in just the same way with nbtree indexes (if left unaddressed).
Clearly "the letter of the law" and "the spirit of the law" must both
be considered. We want to talk about the latter on this thread.
Concretely, here are specific questions (perhaps Tom can weigh in on
these as the principal designer of the relevant interfaces):
1. Any objections to the idea of teaching VACUUM ANALYZE to
distinguish between the cases where VACUUM ran and performed "real
index vacuuming", to make it more intelligent about overwriting
pg_class stats for indexes?
I define "real index vacuuming" as calling any indexes ambulkdelete() routine.
2. Does anybody anticipate any other issues? Possibly an issue that
resembles this existing known VACUUM ANALYZE issue?
Thanks
--
Peter Geoghegan
On Tue, Mar 2, 2021 at 6:01 PM Peter Geoghegan <pg@bowt.ie> wrote:
1. Any objections to the idea of teaching VACUUM ANALYZE to
distinguish between the cases where VACUUM ran and performed "real
index vacuuming", to make it more intelligent about overwriting
pg_class stats for indexes?
I think that a simpler approach would work better: When
ANALYZE/do_analyze_rel() decides whether or not it should call
vac_update_relstats() for each index, it should simply not care
whether or not this is a VACUUM ANALYZE (as opposed to a simple
ANALYZE). This is already what we do for the heap relation itself. Why
shouldn't we do something similar for indexes?
What do you think, Tom? Your bugfix commit b4b6923e03f from 2011
taught do_analyze_rel() to not care about whether VACUUM took place
earlier in the same command -- though only in the case of the heap
relation (not in the case of its indexes). That decision now seems a
bit arbitrary to me.
I should point out that this is the *opposite* of what we did from
2004 - 2011 (following Tom's 2004 commit f0c9397f808). During that
time the policy was to not update pg_class.reltuples inside
do_analyze_rel() when we knew that VACUUM ran. The policy was at least
the same for indexes and the heap/table during this period, so it was
consistent in that sense. However, I don't think that we should
reintroduce that policy now. Doing so would be contrary to the API
contract for index AMs established by Tom's 2006 commit e57345975cf --
that allowed amvacuumcleanup() to be a no-op when there was no
ambulkdelete() call (it also taught hashvacuumcleanup() to do just
that).
To recap, our ultimate goal here is to make btvacuumcleanup() close to
hashvacuumcleanup() -- it should be able to skip all cleanup when
there was no btbulkdelete() call during the same VACUUM (nbtree page
deletion still has cases that force us to do real work in the absence
of a btbulkdelete() call for the VACUUM, but that remaining exception
should be very rare).
--
Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
I think that a simpler approach would work better: When
ANALYZE/do_analyze_rel() decides whether or not it should call
vac_update_relstats() for each index, it should simply not care
whether or not this is a VACUUM ANALYZE (as opposed to a simple
ANALYZE). This is already what we do for the heap relation itself. Why
shouldn't we do something similar for indexes?
What do you think, Tom? Your bugfix commit b4b6923e03f from 2011
taught do_analyze_rel() to not care about whether VACUUM took place
earlier in the same command -- though only in the case of the heap
relation (not in the case of its indexes). That decision now seems a
bit arbitrary to me.
Well, nobody had complained about the index stats at that point,
so I don't think I was thinking about that aspect of it.
As you say, the history here is a bit convoluted, but it seems like
a good principle to avoid interconnections between VACUUM and ANALYZE
as much as we can. I haven't been paying enough attention to this
thread to have more insight than that.
regards, tom lane
On Mon, Mar 8, 2021 at 1:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
As you say, the history here is a bit convoluted, but it seems like
a good principle to avoid interconnections between VACUUM and ANALYZE
as much as we can. I haven't been paying enough attention to this
thread to have more insight than that.
The attached patch does what I proposed earlier today: it teaches
do_analyze_rel() to always set pg_class.reltuples for indexes when it
would do the same thing for the heap/table relation already. It's now
uniform in that sense.
Also included is a patch that removes the
vacuum_cleanup_index_scale_factor mechanism for triggering an index
scan during VACUUM -- that's what the second patch does (this depends
on the first patch, really).
Do you think that a backpatch to Postgres 13 for both of these patches
would be acceptable? There are two main concerns that I have in mind
here, both of which are only issues in Postgres 13:
1. Arguably the question of skipping scanning the index should have been
considered by the autovacuum_vacuum_insert_scale_factor patch when it
was committed for Postgres 13 -- but it wasn't. There is a regression
that was tied to autovacuum_vacuum_insert_scale_factor in Postgres 13
by Mark Callaghan:
https://smalldatum.blogspot.com/2021/01/insert-benchmark-postgres-is-still.html
The blog post says: "Updates - To understand the small regression
mentioned above for the l.i1 test (more CPU & write IO) I repeated the
test with 100M rows using 2 configurations: one disabled index
deduplication and the other disabled insert-triggered autovacuum.
Disabling index deduplication had no effect and disabling
insert-triggered autovacuum resolves the regression."
I think that this regression is almost entirely explainable by the
need to unnecessarily scan indexes for autovacuum VACUUMs that just
need to set the visibility map. This issue is basically avoidable,
just by removing the vacuum_cleanup_index_scale_factor cleanup-only
VACUUM criteria (per my second patch).
2. I fixed a bug in nbtree deduplication btvacuumcleanup() stats in
commit 48e12913. This fix still left things in kind of a bad state:
there are still cases where the btvacuumcleanup()-only VACUUM case
will set pg_class.reltuples to a value that is significantly below
what it should be (it all depends on how effective deduplication is
with the data). These remaining cases are effectively fixed by the
second patch.
I probably should have made btvacuumcleanup()-only VACUUMs set
"stats->estimate_count = true" when I was working on the fix that
became commit 48e12913. Purely because my approach was inherently
approximate with posting list tuples, and so shouldn't be trusted for
anything important (num_index_tuples is suitable for VACUUM VERBOSE
output only in affected cases). I didn't set "stats->estimate_count =
true" in affected cases because I was worried about unforeseen
consequences. But this seems defensible now, all things considered.
There are other things that are slightly broken but will be fixed by
the first patch. But I'm really just worried about these two cases in
Postgres 13.
Thanks for weighing in
--
Peter Geoghegan
Attachments:
0001-VACUUM-ANALYZE-Always-set-pg_class.reltuples.patchapplication/x-patch; name=0001-VACUUM-ANALYZE-Always-set-pg_class.reltuples.patchDownload+31-22
0002-Remove-vacuum_cleanup_index_scale_factor-GUC-param.patchapplication/x-patch; name=0002-Remove-vacuum_cleanup_index_scale_factor-GUC-param.patchDownload+43-209
On Tue, Mar 9, 2021 at 7:35 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Mon, Mar 8, 2021 at 1:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
As you say, the history here is a bit convoluted, but it seems like
a good principle to avoid interconnections between VACUUM and ANALYZE
as much as we can. I haven't been paying enough attention to this
thread to have more insight than that.The attached patch does what I proposed earlier today: it teaches
do_analyze_rel() to always set pg_class.reltuples for indexes when it
would do the same thing for the heap/table relation already. It's now
uniform in that sense.
Thank you for the patches. I looked at 0001 patch and have a comment:
+ * We don't report to the stats collector here because the stats collector
+ * only tracks per-table stats. Reset the changes_since_analyze counter
+ * only if we analyzed all columns; otherwise, there is still work for
+ * auto-analyze to do.I think the comment becomes clearer if we add "if doing inherited
stats" at top of the above paragraph since we actually report to the
stats collector in !inh case.
Also included is a patch that removes the
vacuum_cleanup_index_scale_factor mechanism for triggering an index
scan during VACUUM -- that's what the second patch does (this depends
on the first patch, really).
0002 patch looks good to me.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
On Mon, Mar 8, 2021 at 10:21 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
Thank you for the patches. I looked at 0001 patch and have a comment:
+ * We don't report to the stats collector here because the stats collector + * only tracks per-table stats. Reset the changes_since_analyze counter + * only if we analyzed all columns; otherwise, there is still work for + * auto-analyze to do.I think the comment becomes clearer if we add "if doing inherited
stats" at top of the above paragraph since we actually report to the
stats collector in !inh case.
I messed the comment up. Oops. Fixed now.
Also included is a patch that removes the
vacuum_cleanup_index_scale_factor mechanism for triggering an index
scan during VACUUM -- that's what the second patch does (this depends
on the first patch, really).0002 patch looks good to me.
Great.
Attached revision has a bit more polish. It includes new commit
messages which explains what we're really trying to fix here. I also
included backpatchable versions for Postgres 13 -- that's the other
significant change compared to the last version.
My current plan is to commit everything within the next day or two.
This includes backpatching to Postgres 13 only. I am now leaning
against doing anything in Postgres 11 and 12, for the closely related
btm_last_cleanup_num_heap_tuples VACUUM accuracy issue. There have
been no complaints from users using Postgres 11 or 12, so I'll leave
them alone. (Sorry for changing my mind again and again.)
To be clear: I plan on disabling (though not removing) the
vacuum_cleanup_index_scale_factor GUC and storage parameter on
Postgres 13, even though that is a stable release. This approach is
unorthodox, but it has a kind of a precedent -- the recheck_on_update
storage param was disabled on the Postgres 11 branch by commit
5d28c9bd. More importantly, it just happens to make sense, given the
specifics here.
--
Peter Geoghegan
Attachments:
v2-0002-VACUUM-ANALYZE-Always-update-pg_class.reltuples.patchapplication/octet-stream; name=v2-0002-VACUUM-ANALYZE-Always-update-pg_class.reltuples.patchDownload+34-22
REL_13_STABLE-v2-0002-VACUUM-ANALYZE-Always-update-pg_class.reltuples.patchapplication/octet-stream; name=REL_13_STABLE-v2-0002-VACUUM-ANALYZE-Always-update-pg_class.reltuples.patchDownload+34-22
v2-0001-Don-t-consider-newly-inserted-tuples-in-nbtree-VA.patchapplication/octet-stream; name=v2-0001-Don-t-consider-newly-inserted-tuples-in-nbtree-VA.patchDownload+44-210
REL_13_STABLE-v2-0001-Don-t-consider-newly-inserted-tuples-in-nbtree-VA.patchapplication/octet-stream; name=REL_13_STABLE-v2-0001-Don-t-consider-newly-inserted-tuples-in-nbtree-VA.patchDownload+60-154
On Tue, Mar 9, 2021 at 7:42 PM Peter Geoghegan <pg@bowt.ie> wrote:
My current plan is to commit everything within the next day or two.
This includes backpatching to Postgres 13 only.
Pushed, thanks.
--
Peter Geoghegan
On Thu, Mar 11, 2021 at 10:12 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Tue, Mar 9, 2021 at 7:42 PM Peter Geoghegan <pg@bowt.ie> wrote:
My current plan is to commit everything within the next day or two.
This includes backpatching to Postgres 13 only.Pushed, thanks.
Great! Thank you!
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/