Skip to content

Commit 661643d

Browse files
michaelpqmichaelpq
committed
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 0d83138 did not introduce an issue, but it has encouraged the problem we are dealing with in this commit, with b84dbc8 for "truncate" and a96c41f 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
1 parent 82015fd commit 661643d

File tree

5 files changed

+206
-5
lines changed

5 files changed

+206
-5
lines changed

‎src/backend/commands/vacuum.c

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
#include "utils/fmgroids.h"
5757
#include "utils/guc.h"
5858
#include "utils/guc_hooks.h"
59+
#include "utils/injection_point.h"
5960
#include "utils/memutils.h"
6061
#include "utils/snapmgr.h"
6162
#include "utils/syscache.h"
@@ -634,7 +635,15 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
634635

635636
if (params->options & VACOPT_VACUUM)
636637
{
637-
if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy))
638+
VacuumParams params_copy;
639+
640+
/*
641+
* vacuum_rel() scribbles on the parameters, so give it a copy
642+
* to avoid affecting other relations.
643+
*/
644+
memcpy(&params_copy, params, sizeof(VacuumParams));
645+
646+
if (!vacuum_rel(vrel->oid, vrel->relation, &params_copy, bstrategy))
638647
continue;
639648
}
640649

@@ -2008,9 +2017,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
20082017
Oid save_userid;
20092018
int save_sec_context;
20102019
int save_nestlevel;
2020+
VacuumParams toast_vacuum_params;
20112021

20122022
Assert(params != NULL);
20132023

2024+
/*
2025+
* This function scribbles on the parameters, so make a copy early to
2026+
* avoid affecting the TOAST table (if we do end up recursing to it).
2027+
*/
2028+
memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
2029+
20142030
/* Begin a transaction for vacuuming this relation */
20152031
StartTransactionCommand();
20162032

@@ -2191,6 +2207,15 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
21912207
}
21922208
}
21932209

2210+
#ifdef USE_INJECTION_POINTS
2211+
if (params->index_cleanup == VACOPTVALUE_AUTO)
2212+
INJECTION_POINT("vacuum-index-cleanup-auto", NULL);
2213+
else if (params->index_cleanup == VACOPTVALUE_DISABLED)
2214+
INJECTION_POINT("vacuum-index-cleanup-disabled", NULL);
2215+
else if (params->index_cleanup == VACOPTVALUE_ENABLED)
2216+
INJECTION_POINT("vacuum-index-cleanup-enabled", NULL);
2217+
#endif
2218+
21942219
/*
21952220
* Check if the vacuum_max_eager_freeze_failure_rate table storage
21962221
* parameter was specified. This overrides the GUC value.
@@ -2221,6 +2246,15 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
22212246
params->truncate = VACOPTVALUE_DISABLED;
22222247
}
22232248

2249+
#ifdef USE_INJECTION_POINTS
2250+
if (params->truncate == VACOPTVALUE_AUTO)
2251+
INJECTION_POINT("vacuum-truncate-auto", NULL);
2252+
else if (params->truncate == VACOPTVALUE_DISABLED)
2253+
INJECTION_POINT("vacuum-truncate-disabled", NULL);
2254+
else if (params->truncate == VACOPTVALUE_ENABLED)
2255+
INJECTION_POINT("vacuum-truncate-enabled", NULL);
2256+
#endif
2257+
22242258
/*
22252259
* Remember the relation's TOAST relation for later, if the caller asked
22262260
* us to process it. In VACUUM FULL, though, the toast table is
@@ -2299,15 +2333,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
22992333
*/
23002334
if (toast_relid != InvalidOid)
23012335
{
2302-
VacuumParams toast_vacuum_params;
2303-
23042336
/*
23052337
* Force VACOPT_PROCESS_MAIN so vacuum_rel() processes it. Likewise,
23062338
* set toast_parent so that the privilege checks are done on the main
23072339
* relation. NB: This is only safe to do because we hold a session
23082340
* lock on the main relation that prevents concurrent deletion.
23092341
*/
2310-
memcpy(&toast_vacuum_params, params, sizeof(VacuumParams));
23112342
toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
23122343
toast_vacuum_params.toast_parent = relid;
23132344

‎src/test/modules/injection_points/Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ EXTENSION = injection_points
1111
DATA = injection_points--1.0.sql
1212
PGFILEDESC = "injection_points - facility for injection points"
1313

14-
REGRESS = injection_points hashagg reindex_conc
14+
REGRESS = injection_points hashagg reindex_conc vacuum
1515
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
1616

1717
ISOLATION = basic inplace syscache-update-pruned

‎src/test/modules/injection_points/expected/vacuum.out

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
-- Tests for VACUUM
2+
CREATE EXTENSION injection_points;
3+
SELECT injection_points_set_local();
4+
injection_points_set_local
5+
----------------------------
6+
7+
(1 row)
8+
9+
SELECT injection_points_attach('vacuum-index-cleanup-auto', 'notice');
10+
injection_points_attach
11+
-------------------------
12+
13+
(1 row)
14+
15+
SELECT injection_points_attach('vacuum-index-cleanup-disabled', 'notice');
16+
injection_points_attach
17+
-------------------------
18+
19+
(1 row)
20+
21+
SELECT injection_points_attach('vacuum-index-cleanup-enabled', 'notice');
22+
injection_points_attach
23+
-------------------------
24+
25+
(1 row)
26+
27+
SELECT injection_points_attach('vacuum-truncate-auto', 'notice');
28+
injection_points_attach
29+
-------------------------
30+
31+
(1 row)
32+
33+
SELECT injection_points_attach('vacuum-truncate-disabled', 'notice');
34+
injection_points_attach
35+
-------------------------
36+
37+
(1 row)
38+
39+
SELECT injection_points_attach('vacuum-truncate-enabled', 'notice');
40+
injection_points_attach
41+
-------------------------
42+
43+
(1 row)
44+
45+
-- Check state of index_cleanup and truncate in VACUUM.
46+
CREATE TABLE vac_tab_on_toast_off(i int, j text) WITH
47+
(autovacuum_enabled=false,
48+
vacuum_index_cleanup=true, toast.vacuum_index_cleanup=false,
49+
vacuum_truncate=true, toast.vacuum_truncate=false);
50+
CREATE TABLE vac_tab_off_toast_on(i int, j text) WITH
51+
(autovacuum_enabled=false,
52+
vacuum_index_cleanup=false, toast.vacuum_index_cleanup=true,
53+
vacuum_truncate=false, toast.vacuum_truncate=true);
54+
-- Multiple relations should use their options in isolation.
55+
VACUUM vac_tab_on_toast_off, vac_tab_off_toast_on;
56+
NOTICE: notice triggered for injection point vacuum-index-cleanup-enabled
57+
NOTICE: notice triggered for injection point vacuum-truncate-enabled
58+
NOTICE: notice triggered for injection point vacuum-index-cleanup-disabled
59+
NOTICE: notice triggered for injection point vacuum-truncate-disabled
60+
NOTICE: notice triggered for injection point vacuum-index-cleanup-disabled
61+
NOTICE: notice triggered for injection point vacuum-truncate-disabled
62+
NOTICE: notice triggered for injection point vacuum-index-cleanup-enabled
63+
NOTICE: notice triggered for injection point vacuum-truncate-enabled
64+
-- Check "auto" case of index_cleanup and "truncate" controlled by
65+
-- its GUC.
66+
CREATE TABLE vac_tab_auto(i int, j text) WITH
67+
(autovacuum_enabled=false,
68+
vacuum_index_cleanup=auto, toast.vacuum_index_cleanup=auto);
69+
SET vacuum_truncate = false;
70+
VACUUM vac_tab_auto;
71+
NOTICE: notice triggered for injection point vacuum-index-cleanup-auto
72+
NOTICE: notice triggered for injection point vacuum-truncate-disabled
73+
NOTICE: notice triggered for injection point vacuum-index-cleanup-auto
74+
NOTICE: notice triggered for injection point vacuum-truncate-disabled
75+
SET vacuum_truncate = true;
76+
VACUUM vac_tab_auto;
77+
NOTICE: notice triggered for injection point vacuum-index-cleanup-auto
78+
NOTICE: notice triggered for injection point vacuum-truncate-enabled
79+
NOTICE: notice triggered for injection point vacuum-index-cleanup-auto
80+
NOTICE: notice triggered for injection point vacuum-truncate-enabled
81+
RESET vacuum_truncate;
82+
DROP TABLE vac_tab_auto;
83+
DROP TABLE vac_tab_on_toast_off;
84+
DROP TABLE vac_tab_off_toast_on;
85+
-- Cleanup
86+
SELECT injection_points_detach('vacuum-index-cleanup-auto');
87+
injection_points_detach
88+
-------------------------
89+
90+
(1 row)
91+
92+
SELECT injection_points_detach('vacuum-index-cleanup-disabled');
93+
injection_points_detach
94+
-------------------------
95+
96+
(1 row)
97+
98+
SELECT injection_points_detach('vacuum-index-cleanup-enabled');
99+
injection_points_detach
100+
-------------------------
101+
102+
(1 row)
103+
104+
SELECT injection_points_detach('vacuum-truncate-auto');
105+
injection_points_detach
106+
-------------------------
107+
108+
(1 row)
109+
110+
SELECT injection_points_detach('vacuum-truncate-disabled');
111+
injection_points_detach
112+
-------------------------
113+
114+
(1 row)
115+
116+
SELECT injection_points_detach('vacuum-truncate-enabled');
117+
injection_points_detach
118+
-------------------------
119+
120+
(1 row)
121+
122+
DROP EXTENSION injection_points;

‎src/test/modules/injection_points/meson.build

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ tests += {
3737
'injection_points',
3838
'hashagg',
3939
'reindex_conc',
40+
'vacuum',
4041
],
4142
'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
4243
# The injection points are cluster-wide, so disable installcheck

‎src/test/modules/injection_points/sql/vacuum.sql

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
-- Tests for VACUUM
2+
3+
CREATE EXTENSION injection_points;
4+
5+
SELECT injection_points_set_local();
6+
SELECT injection_points_attach('vacuum-index-cleanup-auto', 'notice');
7+
SELECT injection_points_attach('vacuum-index-cleanup-disabled', 'notice');
8+
SELECT injection_points_attach('vacuum-index-cleanup-enabled', 'notice');
9+
SELECT injection_points_attach('vacuum-truncate-auto', 'notice');
10+
SELECT injection_points_attach('vacuum-truncate-disabled', 'notice');
11+
SELECT injection_points_attach('vacuum-truncate-enabled', 'notice');
12+
13+
-- Check state of index_cleanup and truncate in VACUUM.
14+
CREATE TABLE vac_tab_on_toast_off(i int, j text) WITH
15+
(autovacuum_enabled=false,
16+
vacuum_index_cleanup=true, toast.vacuum_index_cleanup=false,
17+
vacuum_truncate=true, toast.vacuum_truncate=false);
18+
CREATE TABLE vac_tab_off_toast_on(i int, j text) WITH
19+
(autovacuum_enabled=false,
20+
vacuum_index_cleanup=false, toast.vacuum_index_cleanup=true,
21+
vacuum_truncate=false, toast.vacuum_truncate=true);
22+
-- Multiple relations should use their options in isolation.
23+
VACUUM vac_tab_on_toast_off, vac_tab_off_toast_on;
24+
25+
-- Check "auto" case of index_cleanup and "truncate" controlled by
26+
-- its GUC.
27+
CREATE TABLE vac_tab_auto(i int, j text) WITH
28+
(autovacuum_enabled=false,
29+
vacuum_index_cleanup=auto, toast.vacuum_index_cleanup=auto);
30+
SET vacuum_truncate = false;
31+
VACUUM vac_tab_auto;
32+
SET vacuum_truncate = true;
33+
VACUUM vac_tab_auto;
34+
RESET vacuum_truncate;
35+
36+
DROP TABLE vac_tab_auto;
37+
DROP TABLE vac_tab_on_toast_off;
38+
DROP TABLE vac_tab_off_toast_on;
39+
40+
-- Cleanup
41+
SELECT injection_points_detach('vacuum-index-cleanup-auto');
42+
SELECT injection_points_detach('vacuum-index-cleanup-disabled');
43+
SELECT injection_points_detach('vacuum-index-cleanup-enabled');
44+
SELECT injection_points_detach('vacuum-truncate-auto');
45+
SELECT injection_points_detach('vacuum-truncate-disabled');
46+
SELECT injection_points_detach('vacuum-truncate-enabled');
47+
DROP EXTENSION injection_points;

0 commit comments

Comments
 (0)