add PROCESS_MAIN to VACUUM

rebased for cfbot

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

On Thu, 2022-12-29 at 16:00 -0800, Nathan Bossart wrote:

The motivation for adding this option is to make it easier to VACUUM
only a
relation's TOAST table.  At the moment, you need to find the TOAST
table by
examining a relation's reltoastrelid, and you need USAGE on the
pg_toast
schema.  This option could also help make it possible to call only
vac_update_datfrozenxid() without processing any relations, as
discussed
elsewhere [2].

For completeness, did you consider CLUSTER and REINDEX options as well?

--
Jeff Davis
PostgreSQL Contributor Team - AWS

On Fri, Jan 13, 2023 at 03:24:09PM -0800, Jeff Davis wrote:

For completeness, did you consider CLUSTER and REINDEX options as well?

I have not, but I can put together patches for those as well.

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

On Sat, 7 Jan 2023 at 10:37, Nathan Bossart <nathandbossart@gmail.com> wrote:

rebased for cfbot

The patch does not apply on top of HEAD as in [1]http://cfbot.cputube.org/patch_41_4088.log, please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
d540a02a724b9643205abce8c5644a0f0908f6e3 ===
=== applying patch ./v2-0001-add-PROCESS_MAIN-to-VACUUM.patch
patching file src/backend/commands/vacuum.c
....
Hunk #8 FAILED at 2097.
1 out of 8 hunks FAILED -- saving rejects to file
src/backend/commands/vacuum.c.rej

[1]: http://cfbot.cputube.org/patch_41_4088.log

Regards,
Vignesh

On Thu, Jan 19, 2023 at 05:28:25PM +0530, vignesh C wrote:

The patch does not apply on top of HEAD as in [1], please post a rebased patch:

rebased

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

On Fri, Jan 13, 2023 at 03:30:15PM -0800, Nathan Bossart wrote:

On Fri, Jan 13, 2023 at 03:24:09PM -0800, Jeff Davis wrote:

For completeness, did you consider CLUSTER and REINDEX options as well?

I have not, but I can put together patches for those as well.

Are you planning to do that here, on this thread ?

It seems like a good idea - it would allow simplifying an existing or
future scripts which needs to reindex a toast table, saving the trouble
of looking up the name of the toast table, and a race condition and
server error log if the table itself were processed. I can imagine that
might happen if a separate process used TRUNCATE, for example.

--
Justin

On Mon, Feb 20, 2023 at 10:31:11AM -0600, Justin Pryzby wrote:

On Fri, Jan 13, 2023 at 03:30:15PM -0800, Nathan Bossart wrote:

On Fri, Jan 13, 2023 at 03:24:09PM -0800, Jeff Davis wrote:

For completeness, did you consider CLUSTER and REINDEX options as well?

I have not, but I can put together patches for those as well.

Are you planning to do that here, on this thread ?

Yes, I just haven't made time for it yet. IIRC I briefly looked into
CLUSTER and decided that it was probably not worth the effort, but I still
think it's worth adding this option to REINDEX.

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

On Thu, Jan 19, 2023 at 11:08:07AM -0800, Nathan Bossart wrote:

rebased

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.

@@ -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?

+-- 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.
--
Michael

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

On 2023-Mar-01, Michael Paquier wrote:

+-- 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.

Maybe instead of reading the log, read values from pg_stat_all_tables.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Uno puede defenderse de los ataques; contra los elogios se esta indefenso"

On Wed, Mar 01, 2023 at 07:09:53PM +0100, Alvaro Herrera wrote:

On 2023-Mar-01, Michael Paquier wrote:

+-- 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.

Maybe instead of reading the log, read values from pg_stat_all_tables.

Here is an attempt at that. Thanks for the idea.

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

On Wed, Mar 01, 2023 at 07:09:53PM +0100, Alvaro Herrera wrote:

Maybe instead of reading the log, read values from pg_stat_all_tables.

Ah, right. I was looking at pg_stat_user_tables yesterday, and forgot
that pg_stat_all_tables tracks toast tables, so it should be fine to
do some validation with that.
--
Michael

On Thu, Mar 2, 2023 at 4:13 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

On Wed, Mar 01, 2023 at 07:09:53PM +0100, Alvaro Herrera wrote:

On 2023-Mar-01, Michael Paquier wrote:

+-- 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.

Maybe instead of reading the log, read values from pg_stat_all_tables.

Here is an attempt at that. Thanks for the idea.

I've reviewed the v4 patch. Here is a minor comment:

+SELECT * FROM vactst_vacuum_counts;
+   left   | vacuum_count
+----------+--------------
+ pg_toast |            1
+ vactst   |            0
+(2 rows)

+CREATE VIEW vactst_vacuum_counts AS
+   SELECT left(s.relname, 8), s.vacuum_count
+   FROM pg_stat_all_tables s
+   LEFT JOIN pg_class c ON s.relid = c.reltoastrelid
+   WHERE c.relname = 'vactst' OR s.relname = 'vactst'
+   ORDER BY s.relname;

Cutting the toast relation name to 'pg_toast' is a bit confusing to me
as we have the pg_toast schema. How about using the following query
instead to improve the readability?

SELECT
CASE WHEN c.relname IS NULL THEN
s.relname
ELSE
'toast for ' || c.relname
END as relname,
s.vacuum_count
FROM pg_stat_all_tables s
LEFT JOIN pg_class c ON s.relid = c.reltoastrelid
WHERE c.relname = 'vactst' OR s.relname = 'vactst'

We will get like:

SELECT * FROM vactst_vacuum_counts;
relname | vacuum_count
------------------+--------------
toast for vactst | 0
vactst | 1
(2 rows)

The rest looks good to me.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Mar 02, 2023 at 12:58:32PM +0900, Masahiko Sawada wrote:

Cutting the toast relation name to 'pg_toast' is a bit confusing to me
as we have the pg_toast schema. How about using the following query
instead to improve the readability?

SELECT
CASE WHEN c.relname IS NULL THEN
s.relname
ELSE
'toast for ' || c.relname
END as relname,
s.vacuum_count
FROM pg_stat_all_tables s
LEFT JOIN pg_class c ON s.relid = c.reltoastrelid
WHERE c.relname = 'vactst' OR s.relname = 'vactst'

Another tweak that I have learnt to like is to apply a filter with
regexp_replace(), see 090_reindexdb.pl:
regexp_replace(b.indname::text, '(pg_toast.pg_toast_)\\d+(_index)', '\\1<oid>\\2')

If you make that part of the view definition, the result is the same,
so that's up to which solution one prefers.
--
Michael

On Thu, Mar 02, 2023 at 02:21:08PM +0900, Michael Paquier wrote:

On Thu, Mar 02, 2023 at 12:58:32PM +0900, Masahiko Sawada wrote:

Cutting the toast relation name to 'pg_toast' is a bit confusing to me
as we have the pg_toast schema. How about using the following query
instead to improve the readability?

SELECT
CASE WHEN c.relname IS NULL THEN
s.relname
ELSE
'toast for ' || c.relname
END as relname,
s.vacuum_count
FROM pg_stat_all_tables s
LEFT JOIN pg_class c ON s.relid = c.reltoastrelid
WHERE c.relname = 'vactst' OR s.relname = 'vactst'

Another tweak that I have learnt to like is to apply a filter with
regexp_replace(), see 090_reindexdb.pl:
regexp_replace(b.indname::text, '(pg_toast.pg_toast_)\\d+(_index)', '\\1<oid>\\2')

If you make that part of the view definition, the result is the same,
so that's up to which solution one prefers.

Here's a new version of the patch that uses Sawada-san's suggestion.
Thanks for taking a look.

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

On Thu, Mar 2, 2023 at 2:26 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

On Thu, Mar 02, 2023 at 02:21:08PM +0900, Michael Paquier wrote:

On Thu, Mar 02, 2023 at 12:58:32PM +0900, Masahiko Sawada wrote:

Cutting the toast relation name to 'pg_toast' is a bit confusing to me
as we have the pg_toast schema. How about using the following query
instead to improve the readability?

SELECT
CASE WHEN c.relname IS NULL THEN
s.relname
ELSE
'toast for ' || c.relname
END as relname,
s.vacuum_count
FROM pg_stat_all_tables s
LEFT JOIN pg_class c ON s.relid = c.reltoastrelid
WHERE c.relname = 'vactst' OR s.relname = 'vactst'

Another tweak that I have learnt to like is to apply a filter with
regexp_replace(), see 090_reindexdb.pl:
regexp_replace(b.indname::text, '(pg_toast.pg_toast_)\\d+(_index)', '\\1<oid>\\2')

If you make that part of the view definition, the result is the same,
so that's up to which solution one prefers.

Here's a new version of the patch that uses Sawada-san's suggestion.
Thanks for taking a look.

Thank you for updating the patch. The patch looks good to me.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

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

On Thu, Mar 02, 2023 at 02:53:02PM +0900, Michael Paquier wrote:

-   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..

fixed

+   {
+       /* 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?

done

An extra question: should we check the behavior of the commands when
applying a list of relations to VACUUM?

I don't feel a strong need for that, especially now that we aren't
modifying params anymore.

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

On Wed, Mar 01, 2023 at 10:53:59PM -0800, Nathan Bossart wrote:

I don't feel a strong need for that, especially now that we aren't
modifying params anymore.

That was mostly OK for me, so applied after tweaking a couple of
places in the tests (extra explanations, for one), the comments and
the code.

The part that improved the tests of PROCESS_TOAST was useful on its
own, so I have done that separately as 46d490a. Another thing that I
have found incorrect is the need for two pg_stat_reset() calls that
would reflect on the whole database, in the middle of a test running
in parallel of other things. As far as I understand, you have added
that to provide fresh data after a single command while relying on
vactst, but it is possible to get the same amount of coverage by
relying on cumulated counts, and that gets even more solid with all
these commands run on an independent table.
--
Michael