Avoid scribbling of VACUUM options

This fixes two issues with the handling of VacuumParams in vacuum_rel().
This code path has the idea to change the passed-in pointer of
VacuumParams for the "truncate" and "index_cleanup" options for the
relation worked on, impacting the two following scenarios where
incorrect options may be used because a VacuumParams pointer is shared
across multiple relations:
- Multiple relations in a single VACUUM command.
- TOAST relations vacuumed with their main relation.

The problem is avoided by providing to the two callers of vacuum_rel()
copies of VacuumParams, before the pointer is updated for the "truncate"
and "index_cleanup" options.

The refactoring of the VACUUM option and parameters done in 0d831389749a
did not introduce an issue, but it has encouraged the problem we are
dealing with in this commit, with b84dbc8eb80b for "truncate" and
a96c41feec6b for "index_cleanup" that have been added a couple of years
after the initial refactoring.  HEAD will be improved with a different
patch that hardens the uses of VacuumParams across the tree.  This
cannot be backpatched as it introduces an ABI breakage.

The backend portion of the patch has been authored by Nathan, while I
have implemented the tests.  The tests rely on injection points to check
the option values, making them faster, more reliable than the tests
originally proposed by Shihao, and they also provide more coverage.
This part can only be backpatched down to v17.

Reported-by: Shihao Zhong <zhong950419@gmail.com>
Author: Nathan Bossart <nathandbossart@gmail.com>
Co-authored-by: Michael Paquier <michael@paquier.xyz>
Discussion: https://postgr.es/m/CAGRkXqTo+aK=GTy5pSc-9cy8H2F2TJvcrZ-zXEiNJj93np1UUw@mail.gmail.com
Backpatch-through: 13

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 9f3af7a22c82eccfee59a851910db62659faa265..a2132ecedaf8592bd93b166b45701465174cccb2 100644 (file)
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -56,6 +56,7 @@
 #include "utils/fmgroids.h"
 #include "utils/guc.h"
 #include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
@@ -619,7 +620,15 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
                        if (params->options & VACOPT_VACUUM)
                        {
-                               if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy))
+                               VacuumParams params_copy;
+
+                               /*
+                                * vacuum_rel() scribbles on the parameters, so give it a copy
+                                * to avoid affecting other relations.
+                                */
+                               memcpy(&params_copy, params, sizeof(VacuumParams));
+
+                               if (!vacuum_rel(vrel->oid, vrel->relation, &params_copy, bstrategy))
                                        continue;
                        }
 
@@ -1972,9 +1981,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
        Oid                     save_userid;
        int                     save_sec_context;
        int                     save_nestlevel;
+       VacuumParams toast_vacuum_params;
 
        Assert(params != NULL);
 
+       /*
+        * This function scribbles on the parameters, so make a copy early to
+        * avoid affecting the TOAST table (if we do end up recursing to it).
+        */
+       memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
+
        /* Begin a transaction for vacuuming this relation */
        StartTransactionCommand();
 
@@ -2155,6 +2171,15 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
                }
        }
 
+#ifdef USE_INJECTION_POINTS
+       if (params->index_cleanup == VACOPTVALUE_AUTO)
+               INJECTION_POINT("vacuum-index-cleanup-auto");
+       else if (params->index_cleanup == VACOPTVALUE_DISABLED)
+               INJECTION_POINT("vacuum-index-cleanup-disabled");
+       else if (params->index_cleanup == VACOPTVALUE_ENABLED)
+               INJECTION_POINT("vacuum-index-cleanup-enabled");
+#endif
+
        /*
         * Set truncate option based on truncate reloption if it wasn't specified
         * in VACUUM command, or when running in an autovacuum worker
@@ -2168,6 +2193,15 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
                        params->truncate = VACOPTVALUE_DISABLED;
        }
 
+#ifdef USE_INJECTION_POINTS
+       if (params->truncate == VACOPTVALUE_AUTO)
+               INJECTION_POINT("vacuum-truncate-auto");
+       else if (params->truncate == VACOPTVALUE_DISABLED)
+               INJECTION_POINT("vacuum-truncate-disabled");
+       else if (params->truncate == VACOPTVALUE_ENABLED)
+               INJECTION_POINT("vacuum-truncate-enabled");
+#endif
+
        /*
         * Remember the relation's TOAST relation for later, if the caller asked
         * us to process it.  In VACUUM FULL, though, the toast table is
@@ -2247,15 +2281,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
         */
        if (toast_relid != InvalidOid)
        {
-               VacuumParams toast_vacuum_params;
-
                /*
                 * Force VACOPT_PROCESS_MAIN so vacuum_rel() processes it.  Likewise,
                 * set toast_parent so that the privilege checks are done on the main
                 * relation.  NB: This is only safe to do because we hold a session
                 * lock on the main relation that prevents concurrent deletion.
                 */
-               memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
                toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
                toast_vacuum_params.toast_parent = relid;
 

diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index f19c9643ba968db9099770d284817f0e72375941..84ff27c5c5672a14fa09a986819b7b0fa1aeff95 100644 (file)
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -9,7 +9,7 @@ EXTENSION = injection_points
 DATA = injection_points--1.0.sql
 PGFILEDESC = "injection_points - facility for injection points"
 
-REGRESS = injection_points reindex_conc
+REGRESS = injection_points reindex_conc vacuum
 REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
 
 ISOLATION = inplace syscache-update-pruned

diff --git a/src/test/modules/injection_points/expected/vacuum.out b/src/test/modules/injection_points/expected/vacuum.out
new file mode 100644 (file)
index 0000000..f071523
--- /dev/null
+++ b/src/test/modules/injection_points/expected/vacuum.out
@@ -0,0 +1,103 @@
+-- Tests for VACUUM
+CREATE EXTENSION injection_points;
+SELECT injection_points_set_local();
+ injection_points_set_local 
+----------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('vacuum-index-cleanup-auto', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('vacuum-index-cleanup-disabled', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('vacuum-index-cleanup-enabled', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('vacuum-truncate-auto', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('vacuum-truncate-disabled', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_attach('vacuum-truncate-enabled', 'notice');
+ injection_points_attach 
+-------------------------
+ 
+(1 row)
+
+-- Check state of index_cleanup and truncate in VACUUM.
+CREATE TABLE vac_tab_on_toast_off(i int, j text) WITH
+  (autovacuum_enabled=false,
+   vacuum_index_cleanup=true, toast.vacuum_index_cleanup=false,
+   vacuum_truncate=true, toast.vacuum_truncate=false);
+CREATE TABLE vac_tab_off_toast_on(i int, j text) WITH
+  (autovacuum_enabled=false,
+   vacuum_index_cleanup=false, toast.vacuum_index_cleanup=true,
+   vacuum_truncate=false, toast.vacuum_truncate=true);
+-- Multiple relations should use their options in isolation.
+VACUUM vac_tab_on_toast_off, vac_tab_off_toast_on;
+NOTICE:  notice triggered for injection point vacuum-index-cleanup-enabled
+NOTICE:  notice triggered for injection point vacuum-truncate-enabled
+NOTICE:  notice triggered for injection point vacuum-index-cleanup-disabled
+NOTICE:  notice triggered for injection point vacuum-truncate-disabled
+NOTICE:  notice triggered for injection point vacuum-index-cleanup-disabled
+NOTICE:  notice triggered for injection point vacuum-truncate-disabled
+NOTICE:  notice triggered for injection point vacuum-index-cleanup-enabled
+NOTICE:  notice triggered for injection point vacuum-truncate-enabled
+DROP TABLE vac_tab_on_toast_off;
+DROP TABLE vac_tab_off_toast_on;
+-- Cleanup
+SELECT injection_points_detach('vacuum-index-cleanup-auto');
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_detach('vacuum-index-cleanup-disabled');
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_detach('vacuum-index-cleanup-enabled');
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_detach('vacuum-truncate-auto');
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_detach('vacuum-truncate-disabled');
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+SELECT injection_points_detach('vacuum-truncate-enabled');
+ injection_points_detach 
+-------------------------
+ 
+(1 row)
+
+DROP EXTENSION injection_points;

diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 169c415f9c478819962a069c37d1e39322f30822..c1892d760aa5555ed3fcb6bd5f4c6fe05408afa3 100644 (file)
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -34,6 +34,7 @@ tests += {
     'sql': [
       'injection_points',
       'reindex_conc',
+      'vacuum',
     ],
     'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
     # The injection points are cluster-wide, so disable installcheck

diff --git a/src/test/modules/injection_points/sql/vacuum.sql b/src/test/modules/injection_points/sql/vacuum.sql
new file mode 100644 (file)
index 0000000..a799116
--- /dev/null
+++ b/src/test/modules/injection_points/sql/vacuum.sql
@@ -0,0 +1,35 @@
+-- Tests for VACUUM
+
+CREATE EXTENSION injection_points;
+
+SELECT injection_points_set_local();
+SELECT injection_points_attach('vacuum-index-cleanup-auto', 'notice');
+SELECT injection_points_attach('vacuum-index-cleanup-disabled', 'notice');
+SELECT injection_points_attach('vacuum-index-cleanup-enabled', 'notice');
+SELECT injection_points_attach('vacuum-truncate-auto', 'notice');
+SELECT injection_points_attach('vacuum-truncate-disabled', 'notice');
+SELECT injection_points_attach('vacuum-truncate-enabled', 'notice');
+
+-- Check state of index_cleanup and truncate in VACUUM.
+CREATE TABLE vac_tab_on_toast_off(i int, j text) WITH
+  (autovacuum_enabled=false,
+   vacuum_index_cleanup=true, toast.vacuum_index_cleanup=false,
+   vacuum_truncate=true, toast.vacuum_truncate=false);
+CREATE TABLE vac_tab_off_toast_on(i int, j text) WITH
+  (autovacuum_enabled=false,
+   vacuum_index_cleanup=false, toast.vacuum_index_cleanup=true,
+   vacuum_truncate=false, toast.vacuum_truncate=true);
+-- Multiple relations should use their options in isolation.
+VACUUM vac_tab_on_toast_off, vac_tab_off_toast_on;
+
+DROP TABLE vac_tab_on_toast_off;
+DROP TABLE vac_tab_off_toast_on;
+
+-- Cleanup
+SELECT injection_points_detach('vacuum-index-cleanup-auto');
+SELECT injection_points_detach('vacuum-index-cleanup-disabled');
+SELECT injection_points_detach('vacuum-index-cleanup-enabled');
+SELECT injection_points_detach('vacuum-truncate-auto');
+SELECT injection_points_detach('vacuum-truncate-disabled');
+SELECT injection_points_detach('vacuum-truncate-enabled');
+DROP EXTENSION injection_points;