Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 2 Mar 2023 14:53:02 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: add PROCESS_MAIN to VACUUM
Message-ID: <ZAA5vt7jUTRpPDmL@paquier.xyz>
References: <20221230000028.GA435655@nathanxps13>
 <20230107050724.GA914213@nathanxps13>
 <CALDaNm1bKSNG9Jc1mrtk2ZWsVdvTQw+XOnbB3yL_Xf57oAdg5Q@mail.gmail.com>
 <20230119190807.GA4062411@nathanxps13>
 <Y/7xVESfLgw3E/F/@paquier.xyz>
 <20230301172637.GA1456203@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Ur4zJ8kny/WQlDpy"
Content-Disposition: inline
In-Reply-To: <20230301172637.GA1456203@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/ZAA5vt7jUTRpPDmL%40paquier.xyz>
Precedence: bulk


--Ur4zJ8kny/WQlDpy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 01, 2023 at 09:26:37AM -0800, Nathan Bossart wrote:
> Thanks for taking a look.
>=20
> 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) !=3D 0 &&
> >         (params->options & VACOPT_PROCESS_TOAST) =3D=3D 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)
> >     {
> >=20
> > Shouldn't we apply the same rule for PROCESS_MAIN?  One of the
> > regression tests added means that FULL takes priority over
> > PROCESS_MAIN=3DFALSE, which is a bit confusing IMO. =20
>=20
> 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 =3D ((params->options & VACOPT_PROCESS_MAIN) =3D=3D =
0);
+
+       params->options |=3D VACOPT_PROCESS_MAIN;
        vacuum_rel(toast_relid, NULL, params, true);
+       if (force_opt)
+           params->options &=3D ~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

--Ur4zJ8kny/WQlDpy
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmQAOb4ACgkQnvQgOdby
QH0vMA/9E6gZwBs6NqcyHCzRi7Y0p3sEjtyRG1Jdmu6IhW3RFNxasMmV6rPo7qKN
QwcPRKqw0U2wlKGiQINkRZW4L4f5dvlInQ/ePfUhPjrqkTXJpW/+sku5pl72kBBz
lF3+YIkCOPmI9eNxNbU/yOSBn20f1+EY4EGQ0GO+hLMfmjgBkN0cWwKFaqkxz3Gs
HIT/jsKPMqxH5YNyyqo899xQMTDaQuHQnSB6itbP6wS3poX4q6T5p0F2bJ4T6tJr
+8nXhpe6oO7e+gS4oe/rT1pf7f0BnyJAxzMAcE14wLuGkb7p1vF5HV8KFoEeE5SV
CvUt013cUJ0Cy6Dmd5WxcJrwhu+68TbY3brp72p6B6isgTic/Kzj6pd+B9q1q8L9
jnd7tOrddjrEvhij1TTLObSJLHeJ9qat2Cc/Y6fMrvIZ3nRBB77OirKLElJac2zQ
tpIaCqSUhaGdjver+msyjvTdz8cCPdV0RlvQO+STmslKaxTAgvIPAkuWNqaqtKjI
/VThP9Mt499Gap57TKJwVjMqzqrwsjBU6oRyWQ9Q7sFoPgZL3FH1TE3JZ6m/bHKm
pJY6JvBujW+3+dxtoaqLnrmeR+vkUtjI7+09GzcCQUFn3LbmsaHeD0L5C+26lJnh
zTkWiWxJ2YW83IRZbuMYuHpj6al6RJTpROhKYyfcqdSdMGFrqS4=
=Pn2W
-----END PGP SIGNATURE-----

--Ur4zJ8kny/WQlDpy--