From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: add PROCESS_MAIN to VACUUM
Date: Thu, 2 Mar 2023 14:53:02 +0900
Message-ID: <ZAA5vt7jUTRpPDmL@paquier.xyz> (raw)
In-Reply-To: <20230301172637.GA1456203@nathanxps13>
References: <20221230000028.GA435655@nathanxps13>
<20230107050724.GA914213@nathanxps13>
<CALDaNm1bKSNG9Jc1mrtk2ZWsVdvTQw+XOnbB3yL_Xf57oAdg5Q@mail.gmail.com>
<20230119190807.GA4062411@nathanxps13>
<Y/7xVESfLgw3E/F/@paquier.xyz>
<20230301172637.GA1456203@nathanxps13>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../ZAA5vt7jUTRpPDmL@paquier.xyz/2-signature.asc)
download
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: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, nathandbossart@gmail.com, vignesh21@gmail.com
Subject: Re: add PROCESS_MAIN to VACUUM
In-Reply-To: <ZAA5vt7jUTRpPDmL@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox