On Fri, Jun 20, 2025 at 9:34 AM Michael Paquier <mich...@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
