public inbox for [email protected]  

help / color / mirror / Atom feed

From: Junwang Zhao <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: shihao zhong <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: Fixes inconsistent behavior in vacuum when it processes multiple relations
Date: Fri, 20 Jun 2025 22:40:07 +0800
Message-ID: <CAEG8a3+JeCt0xa_N9asAXxG6Xdc0tx-psJW4Od98bB8it1T8pg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAGRkXqTo+aK=GTy5pSc-9cy8H2F2TJvcrZ-zXEiNJj93np1UUw@mail.gmail.com>
	<aFLp9sMEHFHULOFx@nathan>
	<CAGRkXqTp9k_7X1OqqQSnVfoyGxAM3SQ083CcyaezBg9qQ8i+qg@mail.gmail.com>
	<aFRzYhOTZcRgKPLu@nathan>
	<[email protected]>

On Fri, Jun 20, 2025 at 9:34 AM Michael Paquier <[email protected]> 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

view thread (32+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Fixes inconsistent behavior in vacuum when it processes multiple relations
  In-Reply-To: <CAEG8a3+JeCt0xa_N9asAXxG6Xdc0tx-psJW4Od98bB8it1T8pg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox