Date: Wed, 1 Mar 2023 09:26:37 -0800
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: vignesh C <vignesh21@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: add PROCESS_MAIN to VACUUM
Message-ID: <20230301172637.GA1456203@nathanxps13>
References: <20221230000028.GA435655@nathanxps13>
 <20230107050724.GA914213@nathanxps13>
 <CALDaNm1bKSNG9Jc1mrtk2ZWsVdvTQw+XOnbB3yL_Xf57oAdg5Q@mail.gmail.com>
 <20230119190807.GA4062411@nathanxps13>
 <Y/7xVESfLgw3E/F/@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y/7xVESfLgw3E/F/@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20230301172637.GA1456203%40nathanxps13>
Precedence: bulk

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.

> @@ -190,6 +190,7 @@ typedef struct VacAttrStats
>  #define VACOPT_DISABLE_PAGE_SKIPPING 0x80  /* don't skip any pages */
>  #define VACOPT_SKIP_DATABASE_STATS 0x100   /* skip vac_update_datfrozenxid() */
>  #define VACOPT_ONLY_DATABASE_STATS 0x200   /* only vac_update_datfrozenxid() */
> +#define VACOPT_PROCESS_MAIN 0x400  /* process main relation */
> 
> Perhaps the options had better be reorganized so as PROCESS_MAIN is
> just before PROCESS_TOAST?

Sure.

> +-- PROCESS_MAIN option
> +VACUUM (PROCESS_MAIN FALSE) vactst;
> +VACUUM (PROCESS_MAIN FALSE, PROCESS_TOAST FALSE) vactst;
> +VACUUM (PROCESS_MAIN FALSE, FULL) vactst;
> 
> Thinking a bit here.  This set of tests does not make sure that the
> main relation and/or the toast relation have been actually processed. 
> pg_stat_user_tables does not track what's happening on the toast
> relations.  So...  What about adding some tests in 100_vacuumdb.pl
> that rely on vacuumdb --verbose and check the logs produced?  We
> should make sure that the toast or the main relation are processed,
> by tracking, for example, logs like vacuuming "schema.table".  When
> FULL is involved, we may want to track the changes on relfilenodes
> depending on what's wanted.

That seems like a good idea.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com