Call lazy_check_wraparound_failsafe earlier for parallel vacuum
While looking through vacuum code, I noticed that
unlike non-parallel vacuum, parallel vacuum only gets
a failsafe check after an entire index cycle completes.
In vacuumlazy.c, lazy_check_wraparound_failsafe is checked
after every index completes, while in parallel, it is checked
after an entire index cycle completed.
if (!ParallelVacuumIsActive(vacrel))
{
for (int idx = 0; idx < vacrel->nindexes; idx++)
{
Relation indrel = vacrel->indrels[idx];
IndexBulkDeleteResult *istat = vacrel->indstats[idx];
vacrel->indstats[idx] =
lazy_vacuum_one_index(indrel, istat, vacrel->old_live_tuples,
vacrel);
/*
* Done vacuuming an index. Increment the indexes completed
*/
pgstat_progress_update_param(PROGRESS_VACUUM_INDEX_COMPLETED,
idx + 1);
if (lazy_check_wraparound_failsafe(vacrel))
{
/* Wraparound emergency -- end current index scan */
allindexes = false;
break;
}
}
}
else
{
/* Outsource everything to parallel variant */
parallel_vacuum_bulkdel_all_indexes(vacrel->pvs, vacrel->old_live_tuples,
vacrel->num_index_scans);
/*
* Do a postcheck to consider applying wraparound failsafe now. Note
* that parallel VACUUM only gets the precheck and this postcheck.
*/
if (lazy_check_wraparound_failsafe(vacrel))
allindexes = false;
}
When a user is running a parallel vacuum and the vacuum is long running
due to many large indexes, it would make sense to check for failsafe earlier.
Also, checking after every index for parallel vacuum will provide the same
failsafe behavior for both parallel and non-parallel vacuums.
To make this work, it is possible to call lazy_check_wraparound_failsafe
inside parallel_vacuum_process_unsafe_indexes and
parallel_vacuum_process_safe_indexes of vacuumparallel.c
Regards,
Sami Imseih
Amazon Web Services (AWS)
On Wed, Nov 9, 2022 at 6:29 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
When a user is running a parallel vacuum and the vacuum is long running
due to many large indexes, it would make sense to check for failsafe earlier.
It makes sense to prefer consistency here, I suppose. The reason why
we're not consistent is because it was easier not to be, which isn't
exactly the best reason (nor the worst).
I don't think that it's obvious that we need to call the failsafe at
any particular frequency. There is probably an argument to be made for
the idea that we're not checking frequently enough (even in the common
serial VACUUM case), just as there is an argument to be made for the
opposite idea. It's not like there is some simple linear relationship
(or any kind of relationship) between the amount of physical work
performed by one VACUUM operation, and the rate of XID consumption by
the system as a whole. And so the details of how we do it have plenty
to do with what we can afford to do.
My gut instinct is that the most important thing is to at least call
lazy_check_wraparound_failsafe() once per index scan. Multiple index
scans are disproportionately involved in VACUUMs that take far longer
than expected, which are presumably the kind of VACUUMs that tend to
be running when the failsafe actually triggers. We don't really expect
the failsafe to trigger, so presumably when it actually does things haven't
been going well for some time. (Index corruption that prevents forward
progress on one particular index is another example.)
That said, one thing that does bother me in this area occurs to me: we
call lazy_check_wraparound_failsafe() from lazy_scan_heap() (before we
get to the index scans that you're talking about) at an interval that
is based on how many heap pages we've either processed (and recorded
as a scanned_pages page) *or* have skipped over using the visibility
map. In other words we use blkno here, when we should really be using
scanned_pages instead:
if (blkno - next_failsafe_block >= FAILSAFE_EVERY_PAGES)
{
lazy_check_wraparound_failsafe(vacrel);
next_failsafe_block = blkno;
}
This code effectively treats pages skipped using the visibility map as
equivalent to pages physically scanned (scanned_pages), even though
skipping requires essentially no work at all. That just isn't logical,
and feels like something worth fixing. The fundamental unit of work in
lazy_scan_heap() is a *scanned* heap page.
--
Peter Geoghegan
It makes sense to prefer consistency here, I suppose. The reason why
we're not consistent is because it was easier not to be, which isn't
exactly the best reason (nor the worst).
Consistency is the key point here. It is odd that a serial
vacuum may skip the remainder of the indexes if failsafe
kicks-in, but in the parallel case it will go through the entire index
cycle.
My gut instinct is that the most important thing is to at least call
lazy_check_wraparound_failsafe() once per index scan.
I agree. And this should happen in the serial and parallel case.
That said, one thing that does bother me in this area occurs to me: we
call lazy_check_wraparound_failsafe() from lazy_scan_heap() (before we
get to the index scans that you're talking about) at an interval that
is based on how many heap pages we've either processed (and recorded
as a scanned_pages page) *or* have skipped over using the visibility
map. In other words we use blkno here, when we should really be using
scanned_pages instead:
if (blkno - next_failsafe_block >= FAILSAFE_EVERY_PAGES)
{
lazy_check_wraparound_failsafe(vacrel);
next_failsafe_block = blkno;
}
This code effectively treats pages skipped using the visibility map as
equivalent to pages physically scanned (scanned_pages), even though
skipping requires essentially no work at all. That just isn't logical,
and feels like something worth fixing. The fundamental unit of work in
lazy_scan_heap() is a *scanned* heap page.
It makes perfect sense to use the scanned_pages instead.
Regards,
Sami imseih
Amazon Web Services (AWS)
On Thu, Nov 10, 2022 at 10:20 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
Consistency is the key point here. It is odd that a serial
vacuum may skip the remainder of the indexes if failsafe
kicks-in, but in the parallel case it will go through the entire index
cycle.
Yeah, it's a little inconsistent.
My gut instinct is that the most important thing is to at least call
lazy_check_wraparound_failsafe() once per index scan.I agree. And this should happen in the serial and parallel case.
I meant that there should definitely be a check between each round of
index scans (one index scan here affects each and every index). Doing
more than a single index scan is presumably rare, but are likely
common among VACUUM operations that take an unusually long time --
which is where the failsafe is relevant.
I'm just highlighting that multiple index scans (rather than just 0 or
1 index scans) is by far the primary risk factor that leads to a
VACUUM that takes way longer than is typical. (The other notable risk
comes from aggressive VACUUMs that freeze a great deal of heap pages
all at once, which I'm currently addressing by getting rid of the
whole concept of discrete aggressive mode VACUUM operations.)
This code effectively treats pages skipped using the visibility map as
equivalent to pages physically scanned (scanned_pages), even though
skipping requires essentially no work at all. That just isn't logical,
and feels like something worth fixing. The fundamental unit of work in
lazy_scan_heap() is a *scanned* heap page.It makes perfect sense to use the scanned_pages instead.
Want to have a go at writing a patch for that?
--
Peter Geoghegan
Yeah, it's a little inconsistent.
Yes, this should be corrected by calling the failsafe
inside the parallel vacuum loops and handling the case by exiting
the loop and parallel vacuum if failsafe kicks in.
I meant that there should definitely be a check between each round of
index scans (one index scan here affects each and every index). Doing
more than a single index scan is presumably rare, but are likely
common among VACUUM operations that take an unusually long time --
which is where the failsafe is relevant.
Ah, OK. I was confused by the terminology. I read "index scans" as a single
Index scan rather than a index scan cycle.
FWIW, even in the parallel case, the failsafe is checked after every index
scan cycle.
Want to have a go at writing a patch for that?
Yes, I can.
Regards,
Sami Imseih
Amazon Web Services (AWS)
Attached is a patch to check scanned pages rather
than blockno.
Regards,
Sami Imseih
Amazon Web Services (AWS)
Attachments:
v1-0001-fixed-when-wraparound-failsafe-is-checked.patchapplication/octet-stream; name=v1-0001-fixed-when-wraparound-failsafe-is-checked.patchDownload
From 634904f9b76c8072b14fd123207f66c7196a0c8f Mon Sep 17 00:00:00 2001
From: "Imseih (AWS)" <simseih@88665a22795f.ant.amazon.com>
Date: Tue, 20 Dec 2022 11:17:08 -0600
Subject: [PATCH 1/1] fixed when wraparound failsafe is checked.
Trigger lazy_check_wraparound_failsafe when
FAILSAFE_EVERY_PAGES of heap pages have been scanned
including pages that may have been skipped. This ensures
that lazy_check_wraparound_failsafe is checked
consistently even in cases in which many pages may be
skipped.
Discussion: https://www.postgresql.org/message-id/flat/401CE010-4049-4B94-9961-0B610A5D254D%40amazon.com
---
src/backend/access/heap/vacuumlazy.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index d59711b7ec..e3833dff75 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -851,7 +851,6 @@ lazy_scan_heap(LVRelState *vacrel)
BlockNumber rel_pages = vacrel->rel_pages,
blkno,
next_unskippable_block,
- next_failsafe_block = 0,
next_fsm_block_to_vacuum = 0;
VacDeadItems *dead_items = vacrel->dead_items;
Buffer vmbuffer = InvalidBuffer;
@@ -925,11 +924,8 @@ lazy_scan_heap(LVRelState *vacrel)
* one-pass strategy, and the two-pass strategy with the index_cleanup
* param set to 'off'.
*/
- if (blkno - next_failsafe_block >= FAILSAFE_EVERY_PAGES)
- {
+ if (vacrel->scanned_pages % FAILSAFE_EVERY_PAGES == 0)
lazy_check_wraparound_failsafe(vacrel);
- next_failsafe_block = blkno;
- }
/*
* Consider if we definitely have enough space to process TIDs on page
--
2.32.1 (Apple Git-133)
On Sat, Nov 12, 2022 at 12:28 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
Yeah, it's a little inconsistent.
Yes, this should be corrected by calling the failsafe
inside the parallel vacuum loops and handling the case by exiting
the loop and parallel vacuum if failsafe kicks in.
I agree it's better to be consistent but I think we cannot simply call
lazy_check_wraparound_failsafe() inside the parallel vacuum loops.
IIUC the failsafe is heap (or lazyvacuum ) specific, whereas parallel
vacuum is a common infrastructure to do index vacuum in parallel. We
should not break this design. For example, we would need to have a
callback for index scan loop so that the caller (i.e. lazy vacuum) can
do its work.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Wed, Dec 21, 2022 at 2:44 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
Attached is a patch to check scanned pages rather
than blockno.
Thank you for the patch. It looks good to me.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Tue, Dec 20, 2022 at 9:44 AM Imseih (AWS), Sami <simseih@amazon.com> wrote:
Attached is a patch to check scanned pages rather
than blockno.
Pushed, thanks!
I adjusted the FAILSAFE_EVERY_PAGES comments, which now point out that
FAILSAFE_EVERY_PAGES is a power-of-two. The implication is that the
compiler is all but guaranteed to be able to reduce the modulo
division into a shift in the lazy_scan_heap loop, at the point of the
failsafe check. I doubt that it would really matter if the compiler
had to generate a DIV instruction, but it seems like a good idea to
avoid it on general principle, at least in performance sensitive code.
--
Peter Geoghegan
I adjusted the FAILSAFE_EVERY_PAGES comments, which now point out that
FAILSAFE_EVERY_PAGES is a power-of-two. The implication is that the
compiler is all but guaranteed to be able to reduce the modulo
division into a shift in the lazy_scan_heap loop, at the point of the
failsafe check. I doubt that it would really matter if the compiler
had to generate a DIV instruction, but it seems like a good idea to
avoid it on general principle, at least in performance sensitive code.
Thank you!
Regards,
Sami Imseih
Amazon Web Services (AWS)