Обсуждение: Re: 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

>> 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] https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan
>
> 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

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 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] 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] https://postgr.es/m/aFRzYhOTZcRgKPLu%40nathan

-- 
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 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

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

Thanks for your explanation!

Hi,

On 2025-06-30 15:49:49 +0900, Michael Paquier wrote:
> 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.

I was just looking at the includes for tableam.h - a quite widely included
header - and got rather sad seeing vacuum.h in there, as it has a quite large
dependency tree and tableam.h is obviously widely included.

I was surprised that I would have added such a wide include when working on
tableam. Turns out I hadn't - it was 2252fcd4276c.


I think this change was quite ill considered. Look at the dependency tree of
vacuum.h:

https://doxygen.postgresql.org/vacuum_8h.html


That's a *lot* of headers to pull into something as widely included as
tableam.h


Why wasn't it enough to add const markers and keep passing by pointer?


Greetings,

Andres

On Fri, Mar 20, 2026 at 12:27:49PM -0400, Andres Freund wrote:
> Why wasn't it enough to add const markers and keep passing by pointer?

IIRC the idea was to prevent similar problems in the future.  To avoid the
extra #includes, we could instead use the back-patched version (e.g.,
commit 661643deda).

-- 
nathan

Hi,

On 2026-03-20 14:39:11 -0500, Nathan Bossart wrote:
> On Fri, Mar 20, 2026 at 12:27:49PM -0400, Andres Freund wrote:
> > Why wasn't it enough to add const markers and keep passing by pointer?
> 
> IIRC the idea was to prevent similar problems in the future.

Seems using const VacuumParams *params should suffice for that? I don't think
it's particularly likely that we'll accept code that casts the const away and
then later get hurt by that.


> To avoid the extra #includes, we could instead use the back-patched version
> (e.g., commit 661643deda).

I'd probably not go quite there, at least the params should largely be const,
with a local on-stack copy where we do need to modify.

Greetings,

Andres Freund

On Fri, Mar 20, 2026 at 04:32:48PM -0400, Andres Freund wrote:
> On 2026-03-20 14:39:11 -0500, Nathan Bossart wrote:
>> On Fri, Mar 20, 2026 at 12:27:49PM -0400, Andres Freund wrote:
>> > Why wasn't it enough to add const markers and keep passing by pointer?
>> 
>> IIRC the idea was to prevent similar problems in the future.
> 
> Seems using const VacuumParams *params should suffice for that? I don't think
> it's particularly likely that we'll accept code that casts the const away and
> then later get hurt by that.
> 
>> To avoid the extra #includes, we could instead use the back-patched version
>> (e.g., commit 661643deda).
> 
> I'd probably not go quite there, at least the params should largely be const,
> with a local on-stack copy where we do need to modify.

Here is a first try.

-- 
nathan

Hi,

On 2026-03-24 16:47:30 -0500, Nathan Bossart wrote:
> On Fri, Mar 20, 2026 at 04:32:48PM -0400, Andres Freund wrote:
> > On 2026-03-20 14:39:11 -0500, Nathan Bossart wrote:
> >> On Fri, Mar 20, 2026 at 12:27:49PM -0400, Andres Freund wrote:
> >> > Why wasn't it enough to add const markers and keep passing by pointer?
> >> 
> >> IIRC the idea was to prevent similar problems in the future.
> > 
> > Seems using const VacuumParams *params should suffice for that? I don't think
> > it's particularly likely that we'll accept code that casts the const away and
> > then later get hurt by that.
> > 
> >> To avoid the extra #includes, we could instead use the back-patched version
> >> (e.g., commit 661643deda).
> > 
> > I'd probably not go quite there, at least the params should largely be const,
> > with a local on-stack copy where we do need to modify.
> 
> Here is a first try.

Thanks!


Looks reasonable on a skim.


>  static bool
> -vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
> +vacuum_rel(Oid relid, RangeVar *relation, const VacuumParams *params,
>             BufferAccessStrategy bstrategy)
>  {
>      LOCKMODE    lmode;
> @@ -2014,18 +2014,21 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
>      Oid            save_userid;
>      int            save_sec_context;
>      int            save_nestlevel;
> +    VacuumParams params_copy;
>      VacuumParams toast_vacuum_params;

I'd maybe not name it _copy, but params_local or params_table or such, as
somehow it seems a bit odd to modify the copy. But I can't really explain why
it feels odd.

I wonder if more of the code in the function should be updated to use the
copy, otherwise it seems like it could more easily happen that a part of the
code not using the modified version is moved until after a modification, and
the code author assumes the modifications now have taken effect.

Greetings,

Andres Freund

On Tue, Mar 24, 2026 at 11:29:52PM -0400, Andres Freund wrote:
> On 2026-03-24 16:47:30 -0500, Nathan Bossart wrote:
>> +    VacuumParams params_copy;
> 
> I'd maybe not name it _copy, but params_local or params_table or such, as
> somehow it seems a bit odd to modify the copy. But I can't really explain why
> it feels odd.

params_local WFM.

> I wonder if more of the code in the function should be updated to use the
> copy, otherwise it seems like it could more easily happen that a part of the
> code not using the modified version is moved until after a modification, and
> the code author assumes the modifications now have taken effect.

Yeah, I was waffling on this.  I updated the rest of the function to use
params_local instead.

-- 
nathan

On Wed, Mar 25, 2026 at 10:33:48AM -0500, Nathan Bossart wrote:
> On Tue, Mar 24, 2026 at 11:29:52PM -0400, Andres Freund wrote:
> > I wonder if more of the code in the function should be updated to use the
> > copy, otherwise it seems like it could more easily happen that a part of the
> > code not using the modified version is moved until after a modification, and
> > the code author assumes the modifications now have taken effect.
>
> Yeah, I was waffling on this.  I updated the rest of the function to use
> params_local instead.

Fine by me with these extra forward declarations and the pointer
manipulations across these VACUUM/ANALYZE calls.  That would be
protective enough in terms of the original intention of these const
markers added in 2252fcd4276c.

Once vacuum_rel() has its VacuumParams changed to a pointer, renaming
it to params_local and use params_local in all its code paths looks
sensible.  On top of what you are doing, I would add a big warning
comment at the top of the routine to mention that "params" should not
be used directly in the routine.  The local copies should be used.
--
Michael

On Thu, Mar 26, 2026 at 11:22:25AM +0900, Michael Paquier wrote:
> Once vacuum_rel() has its VacuumParams changed to a pointer, renaming
> it to params_local and use params_local in all its code paths looks
> sensible.  On top of what you are doing, I would add a big warning
> comment at the top of the routine to mention that "params" should not
> be used directly in the routine.  The local copies should be used.

Wait...  Can't we just pass params by value only to vacuum_rel()?  Doesn't
this accomplish what we need?

-- 
nathan

On Thu, Mar 26, 2026 at 11:07:52AM -0500, Nathan Bossart wrote:
> Wait...  Can't we just pass params by value only to vacuum_rel()?  Doesn't
> this accomplish what we need?

It would accomplish Andres's goal of avoiding a deeper inclusion of
vacuum.h in src/include/.

Still, I slightly prefer your v2, where the interface of vacuum_rel()
is leaner with all the other ones.  It comes at the cost of copying
the input parameters into a temporary "copy" of VacuumParams, but I
see the fact of marking the input "params" with a const as more
valuable in the long-run, with less temptation to manipulate it
directly especially it is not not marked with a const.  One small
worry with v3 is that people like copy-pasting code around, and I
suspect that v2 could discourage better the patterns that 2252fcd4276c
has tried to improve and that 661643dedad9 had to fix.
--
Michael

On Fri, Mar 27, 2026 at 08:03:30AM +0900, Michael Paquier wrote:
> Still, I slightly prefer your v2, where the interface of vacuum_rel()
> is leaner with all the other ones.  It comes at the cost of copying
> the input parameters into a temporary "copy" of VacuumParams, but I
> see the fact of marking the input "params" with a const as more
> valuable in the long-run, with less temptation to manipulate it
> directly especially it is not not marked with a const.  One small
> worry with v3 is that people like copy-pasting code around, and I
> suspect that v2 could discourage better the patterns that 2252fcd4276c
> has tried to improve and that 661643dedad9 had to fix.

I disagree with you here.  By passing the struct by-value, we are avoiding
scribbles on the original one without an explicit memcpy and without a big
comment warning folks to only use the copy (which seems like it'd be easy
to miss).  I think using a const pointer in most places makes sense, but
not if we need to immediately copy the contents to a local variable anyway.

-- 
nathan

Here is a rebased patch.  Provided cfbot is happy with it, I plan to commit
it later today.  Happy to revisit the vacuum_rel() discussion if needed.

-- 
nathan

Committed.

-- 
nathan