On Wed, Mar 01, 2023 at 09:26:37AM -0800, Nathan Bossart wrote:
> Thanks for taking a look.
>
> On Wed, Mar 01, 2023 at 03:31:48PM +0900, Michael Paquier wrote:
> > PROCESS_TOAST has that:
> > /* sanity check for PROCESS_TOAST */
> > if ((params->options & VACOPT_FULL) != 0 &&
> > (params->options & VACOPT_PROCESS_TOAST) == 0)
> > ereport(ERROR,
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > errmsg("PROCESS_TOAST required with VACUUM FULL")));
> > [...]
> > - if (params->options & VACOPT_FULL)
> > + if (params->options & VACOPT_FULL &&
> > + params->options & VACOPT_PROCESS_MAIN)
> > {
> >
> > Shouldn't we apply the same rule for PROCESS_MAIN? One of the
> > regression tests added means that FULL takes priority over
> > PROCESS_MAIN=FALSE, which is a bit confusing IMO.
>
> I don't think so. We disallow FULL without PROCESS_TOAST because there
> presently isn't a way to VACUUM FULL the main relation without rebuilding
> its TOAST table. However, FULL without PROCESS_MAIN can be used to run
> VACUUM FULL on only the TOAST table.
- if (params->options & VACOPT_FULL)
+ if (params->options & VACOPT_FULL &&
+ params->options & VACOPT_PROCESS_MAIN)
Perhaps this is a bit under-parenthesized, while reading through it
once again..
+ {
+ /* we force VACOPT_PROCESS_MAIN so vacuum_rel() processes it */
+ bool force_opt = ((params->options & VACOPT_PROCESS_MAIN) == 0);
+
+ params->options |= VACOPT_PROCESS_MAIN;
vacuum_rel(toast_relid, NULL, params, true);
+ if (force_opt)
+ params->options &= ~VACOPT_PROCESS_MAIN;
Zigzagging with the centralized VacuumParams is a bit inelegant.
Perhaps it would be neater to copy VacuumParams and append
VACOPT_PROCESS_MAIN on the copy?
An extra question: should we check the behavior of the commands when
applying a list of relations to VACUUM?
--
Michael
