Fixes inconsistent behavior in vacuum when it processes multiple relations

On Wed, Jun 18, 2025 at 11:15:31AM -0400, shihao zhong wrote:

I investigated the code and found a small bug with how we're passing
the VacuumParams pointer.

The call flow is
ExecVacuum -> vacuum -> vacuum_rel

The initial VaccumParams pointer is set in ExecVacuum
In vacuum_rel, this pointer might change because it needs to determine
whether to truncate and perform index_cleanup.

Nice find!

My first reaction is to wonder whether we should 1) also make a similar
change to vacuum() for some future-proofing or 2) just teach vacuum_rel()
to make a local copy of the parameters that it can scribble on. In the
latter case, we might want to assert that the parameters don't change after
calls to vacuum() and vacuum_rel() to prevent this problem from recurring.
That leads me to think (1) might be the better option, although I'm not too
wild about the subtlety of the fix.

--
nathan

That leads me to think (1) might be the better option, although I'm not too
wild about the subtlety of the fix.

Thanks for your feedback. New patch is attached. I also updated the
signature of do_analyze_rel for the same reason.

On Wed, Jun 18, 2025 at 12:31 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:

On Wed, Jun 18, 2025 at 02:48:16PM -0400, shihao zhong wrote:

That leads me to think (1) might be the better option, although I'm not too
wild about the subtlety of the fix.

Thanks for your feedback. New patch is attached. I also updated the
signature of do_analyze_rel for the same reason.

After thinking about this some more, I'm wondering if it would be better to
pursue option (2) because it's a little less invasive (which is important
because this will need to be back-patched). In any case, we have a similar
problem when recursing to the TOAST table, which can be fixed by copying
the params at the top of vacuum_rel().

While testing out the attached patch, I noticed a couple of other
interesting problems in this area [0]/messages/by-id/aFRxC1W_kZU9OjJ9@nathan.

[0]: /messages/by-id/aFRxC1W_kZU9OjJ9@nathan

--
nathan

On Thu, Jun 19, 2025 at 03:30:26PM -0500, Nathan Bossart wrote:

After thinking about this some more, I'm wondering if it would be better to
pursue option (2) because it's a little less invasive (which is important
because this will need to be back-patched). In any case, we have a similar
problem when recursing to the TOAST table, which can be fixed by copying
the params at the top of vacuum_rel().

While testing out the attached patch, I noticed a couple of other
interesting problems in this area [0].

[0] /messages/by-id/aFRxC1W_kZU9OjJ9@nathan

Hmm. I like the simplicity of option 2) for the purpose the back
branches and the post-feature-freeze v18.

However, Option 1) would be my go-to option for HEAD (as of v19
opening for business), but I think that we should harden the code more
than suggested and treat all VacuumParams as purely input arguments
rather keeping some pointers to it depending on the code path we are
dealing with, so as no callers of these inner routines is surprised by
changes that may happen internally. Hence, reading the code of v2,
I'd suggest to apply the same rule to vacuum_get_cutoffs(),
do_analyze_rel() and heap_vacuum_rel(). Except if I am missing
something, it looks like all these calls should be OK with this new
policy. This implies also changing relation_vacuum() in tableam.h,
which can be a HEAD-only change anyway.
--
Michael

However, Option 1) would be my go-to option for HEAD ...

Updated my patch to apply the same rules to all VacuumParams. Also I
am seeing clusterParams as a pass reference, not sure if we should
also change that to prevent future issues. But that should be another
patch.

On Fri, Jun 20, 2025 at 9:34 AM Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Jun 19, 2025 at 03:30:26PM -0500, Nathan Bossart wrote:

After thinking about this some more, I'm wondering if it would be better to
pursue option (2) because it's a little less invasive (which is important
because this will need to be back-patched). In any case, we have a similar
problem when recursing to the TOAST table, which can be fixed by copying
the params at the top of vacuum_rel().

While testing out the attached patch, I noticed a couple of other
interesting problems in this area [0].

[0] /messages/by-id/aFRxC1W_kZU9OjJ9@nathan

Hmm. I like the simplicity of option 2) for the purpose the back
branches and the post-feature-freeze v18.

However, Option 1) would be my go-to option for HEAD (as of v19
opening for business), but I think that we should harden the code more
than suggested and treat all VacuumParams as purely input arguments
rather keeping some pointers to it depending on the code path we are
dealing with, so as no callers of these inner routines is surprised by
changes that may happen internally.

I'd suggest just marking the VacuumParams *params with const, so
that the user can not change its content, or the compiler will error
out, it will force the user to make a copy if changes are needed.
I see vacuum_get_cutoffs already has the const signature.

Passing by const pointer is more efficient than passing by structure value.

Hence, reading the code of v2,
I'd suggest to apply the same rule to vacuum_get_cutoffs(),
do_analyze_rel() and heap_vacuum_rel(). Except if I am missing
something, it looks like all these calls should be OK with this new
policy. This implies also changing relation_vacuum() in tableam.h,
which can be a HEAD-only change anyway.
--
Michael

--
Regards
Junwang Zhao

On Fri, Jun 20, 2025 at 10:14 PM shihao zhong <zhong950419@gmail.com> wrote:

However, Option 1) would be my go-to option for HEAD ...

Updated my patch to apply the same rules to all VacuumParams. Also I
am seeing clusterParams as a pass reference, not sure if we should
also change that to prevent future issues. But that should be another
patch.

 static inline void
-table_relation_vacuum(Relation rel, struct VacuumParams *params,
+table_relation_vacuum(Relation rel, struct VacuumParams params,
    BufferAccessStrategy bstrategy)
 {
  rel->rd_tableam->relation_vacuum(rel, params, bstrategy);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bc37a80dc74..9a8c63352da 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -336,7 +336,7 @@ extern PGDLLIMPORT int64 parallel_vacuum_worker_delay_ns;

 /* in commands/vacuum.c */
 extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool
isTopLevel);
-extern void vacuum(List *relations, VacuumParams *params,
+extern void vacuum(List *relations, VacuumParams params,
     BufferAccessStrategy bstrategy, MemoryContext vac_context,
     bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
@@ -357,7 +357,7 @@ extern void vac_update_relstats(Relation relation,
  bool *frozenxid_updated,
  bool *minmulti_updated,
  bool in_outer_xact);
-extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
+extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams params,
     struct VacuumCutoffs *cutoffs);
 extern bool vacuum_xid_failsafe_check(const struct VacuumCutoffs *cutoffs);
 extern void vac_update_datfrozenxid(void);
@@ -398,7 +398,7 @@ extern void parallel_vacuum_main(dsm_segment *seg,
shm_toc *toc);

 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, RangeVar *relation,
- VacuumParams *params, List *va_cols, bool in_outer_xact,
+ VacuumParams params, List *va_cols, bool in_outer_xact,

It's a bit odd that we have both `VacuumParams *params` and
`struct VacuumParams *params`. Perhaps you could remove
the struct keyword in this patch to make it consistent.

--
Regards
Junwang Zhao

It's a bit odd that we have both `VacuumParams *params` and
`struct VacuumParams *params`.

Thanks for pointing that out, the attached new version fixed that.

I'd suggest just marking the VacuumParams *params with const...

I initially considered that approach. However, the current code path
for vacuum_rel handles table option re-consolidation, rather than the
ExecVacuum caller. This would require moving all parameter checks into
ExecVacuum, which I'm not sure is ideal. For this patch, let's focus
on resolving the inconsistent behavior. We can discuss parameter
re-consolidation further in this thread:
/messages/by-id/aFRxC1W_kZU9OjJ9@nathan

On Fri, Jun 20, 2025 at 10:34:19AM +0900, Michael Paquier wrote:

Hmm. I like the simplicity of option 2) for the purpose the back
branches and the post-feature-freeze v18.

Yeah, let's do this for now.

However, Option 1) would be my go-to option for HEAD (as of v19
opening for business), but I think that we should harden the code more
than suggested and treat all VacuumParams as purely input arguments
rather keeping some pointers to it depending on the code path we are
dealing with, so as no callers of these inner routines is surprised by
changes that may happen internally. Hence, reading the code of v2,
I'd suggest to apply the same rule to vacuum_get_cutoffs(),
do_analyze_rel() and heap_vacuum_rel(). Except if I am missing
something, it looks like all these calls should be OK with this new
policy. This implies also changing relation_vacuum() in tableam.h,
which can be a HEAD-only change anyway.

Sounds reasonable to me.

--
nathan

On Fri, Jun 20, 2025 at 02:16:46PM -0500, Nathan Bossart wrote:

On Fri, Jun 20, 2025 at 10:34:19AM +0900, Michael Paquier wrote:

However, Option 1) would be my go-to option for HEAD (as of v19
opening for business), but I think that we should harden the code more
than suggested and treat all VacuumParams as purely input arguments
rather keeping some pointers to it depending on the code path we are
dealing with, so as no callers of these inner routines is surprised by
changes that may happen internally. Hence, reading the code of v2,
I'd suggest to apply the same rule to vacuum_get_cutoffs(),
do_analyze_rel() and heap_vacuum_rel(). Except if I am missing
something, it looks like all these calls should be OK with this new
policy. This implies also changing relation_vacuum() in tableam.h,
which can be a HEAD-only change anyway.

Sounds reasonable to me.

Looking at the git history, the decision of being able to change
VacuumParams from the reloptions in vacuum_rel() for the index cleanup
dates back to a96c41feec6b (April 2019). It has been copied a couple
of days later for truncate in b84dbc8eb80b (8th of May 2019). The
introduction of VacuumParams comes from 0d831389749a back in 2015,
where more pointers to VacuumParams have been introduced for something
I've authored. Perhaps we should have documented better the fact that
this structure should never be manipulated with const markers back
then, but what's done is done. So I'm at the origin of the design
problem: even if the original refactoring of 0d831389749a did not
break things, it has encouraged the pattern we have to deal with today
because vacuum_rel() has begun this practice.

Anyway, here is an attempt at leaning all that. I am really tempted
to add a couple of const markers to force VacuumParams to be in
read-only mode, even if it means doing so for vacuum() but not touch
at vacuum_rel() where we double-check the reloptions for the truncate
and index cleanup options. That would be of course v19-only material.
Thoughts or opinions?
--
Michael

On Mon, Jun 23, 2025 at 08:48:35AM +0900, Michael Paquier wrote:

Anyway, here is an attempt at leaning all that. I am really tempted
to add a couple of const markers to force VacuumParams to be in
read-only mode, even if it means doing so for vacuum() but not touch
at vacuum_rel() where we double-check the reloptions for the truncate
and index cleanup options. That would be of course v19-only material.
Thoughts or opinions?

Is the idea to do something like this for v19 and this [0]/messages/by-id/aFRzYhOTZcRgKPLu@nathan for the
back-branches?

I think the recurse-to-TOAST-table case is still broken with your patch.
We should probably move the memcpy() for toast_vacuum_params to the very
top of vacuum_rel(). Otherwise, your patch looks generally reasonable to
me.

[0]: /messages/by-id/aFRzYhOTZcRgKPLu@nathan

--
nathan

On Mon, Jun 23, 2025 at 10:38:40AM -0500, Nathan Bossart wrote:

Is the idea to do something like this for v19 and this [0] for the
back-branches?

Yes, that would be the idea. Let's do the bug fix first on all the
branches, then do the refactoring. Knowing that I'm indirectly
responsible for this mess, I would like to take care of that myself.
Would that be OK for you?

I think the recurse-to-TOAST-table case is still broken with your patch.
We should probably move the memcpy() for toast_vacuum_params to the very
top of vacuum_rel(). Otherwise, your patch looks generally reasonable to
me.

Ah, right, I have managed to mess up this part. Sounds to me that we
should provide more coverage for both the truncate and index cleanup
cases, as well.

The updated version attached includes tests that were enough to make
the recursive TOAST table case fail for TRUNCATE and INDEX_CLEANUP,
where I have relied on an EXTERNAL column to fill in the TOAST
relation with data cheaply. The pgstattuple case is less cheap but
I've minimized the load to be able to trigger the index cleanup. It
was tricky to get the threshold right for INDEX_CLEANUP in the case of
TOAST table, but that's small enough to have a short run time while it
should be large enough to trigger a cleanup of the TOAST indexes (my
CI quota has been reached this month, so I can only rely on the CF bot
for some pre-validation job). If the thresholds are too low, we may
need to bump the number of tuples. I'd be OK to remove the TOAST
parts if these prove to be unstable, but let's see. At least these
tests are validating the contents of the patch for the TOAST options,
and they were looking stable on my machine.

Another approach that we could use is an injection point with some
LOGs that show some information based on what VacuumParams holds.
That would be the cheapest method (no need for any data), and entirely
stable as we would look at the stack. Perhaps going down to that is
not really necessary for the sake of this thread.

Attached are two patches to have a clean git history:
- 0001 is the basic fix, to-be-applied first on all the branches.
- 0002 is the refactoring, for v19 once it opens up.

What do you think?
--
Michael

On Tue, Jun 24, 2025 at 10:18:18AM +0900, Michael Paquier wrote:

Knowing that I'm indirectly responsible for this mess, I would like to
take care of that myself. Would that be OK for you?

I would be grateful if you took care of it.

Another approach that we could use is an injection point with some
LOGs that show some information based on what VacuumParams holds.
That would be the cheapest method (no need for any data), and entirely
stable as we would look at the stack. Perhaps going down to that is
not really necessary for the sake of this thread.

+1 for this. I did something similar to verify the other bugs I reported,
and this seems far easier to maintain than potentially-unstable tests that
require lots of setup and that depend on secondary effects.

--
nathan

On Tue, Jun 24, 2025 at 11:30:13AM -0500, Nathan Bossart wrote:

On Tue, Jun 24, 2025 at 10:18:18AM +0900, Michael Paquier wrote:

Knowing that I'm indirectly responsible for this mess, I would like to
take care of that myself. Would that be OK for you?

I would be grateful if you took care of it.

Okay, applied the main fix down to v13 then.

+1 for this. I did something similar to verify the other bugs I reported,
and this seems far easier to maintain than potentially-unstable tests that
require lots of setup and that depend on secondary effects.

After sleeping on this point, I have finished with this idea for tests
down to v17, adding more coverage for v18 with the GUC vacuum_truncate
and the "auto" mode of index_cleanup, while on it. v16 and older
branches don't have tests, but I have checked the sanity of the two
code paths using the tests attached for vacuum.sql and
pgstattuple.sql, at least. Two truncation tests for the "false" cases
were actually unstable. Sometimes the truncation may not happen even
with an aggressive VACUUM. Anyway, it is enough to check the true
case for the two code paths patched, even if it may not happen at
100%. I did not notice a problem with the index_cleanup case as long
as we had enough pages in the TOAST btree index, requiring some
runtime to load the entries, so the cost was annoying.

Attached is the remaining patch for HEAD, planned once v19 opens, and
the tests I have used on the back-branches as a txt to not confuse the
CI, for reference.
--
Michael

On Wed, Jun 25, 2025 at 10:31:35AM +0900, Michael Paquier wrote:

On Tue, Jun 24, 2025 at 11:30:13AM -0500, Nathan Bossart wrote:

On Tue, Jun 24, 2025 at 10:18:18AM +0900, Michael Paquier wrote:

Knowing that I'm indirectly responsible for this mess, I would like to
take care of that myself. Would that be OK for you?

I would be grateful if you took care of it.

Okay, applied the main fix down to v13 then.

Thanks!

Attached is the remaining patch for HEAD, planned once v19 opens, and
the tests I have used on the back-branches as a txt to not confuse the
CI, for reference.

Looks reasonable to me.

--
nathan

Attached is the remaining patch for HEAD, planned once v19 opens, and
the tests I have used on the back-branches as a txt to not confuse the
CI, for reference.

Just want to make sure, are we not going to include my original test
to catch the future regression? Also, could someone please let me know
how to check if the test is stable or not?

Thanks

On Wed, Jun 25, 2025 at 01:12:02PM -0400, shihao zhong wrote:

Just want to make sure, are we not going to include my original test
to catch the future regression? Also, could someone please let me know
how to check if the test is stable or not?

On stable branches, you could reuse the patch I have posted upthread,
even if it is not included in the tree. On HEAD and v17, you can run
the tests of src/test/modules/injection_points/ with
--enable-injection-points set.

Making the index cleanup stable takes a certain amount of cycles
because it requires a minimum amount of data, particularly for the
btree of a TOAST index. The truncation check is halfly stable: it is
possible to check that a truncation does not happen, but it can still
be slightly unstable because the option may not trigger all the time.
So, while the test success rate is perhaps close to 90%, with this
rate going down on slower machines, it is just cheaper and more
reliable to check directly the contents of VacuumParams. Note that
your original set of tests only covered the case of multiple
relations, and it missed coverage for the TOAST relation vacuumed
after its main relation.
--
Michael

Making the index cleanup stable takes a certain amount of cycles,,

Thanks for your explanation!

On Wed, Jun 25, 2025 at 10:21:03AM -0500, Nathan Bossart wrote:

On Wed, Jun 25, 2025 at 10:31:35AM +0900, Michael Paquier wrote:

Attached is the remaining patch for HEAD, planned once v19 opens, and
the tests I have used on the back-branches as a txt to not confuse the
CI, for reference.

Looks reasonable to me.

Thanks. Applied on HEAD, then.
--
Michael