Subject: Fixes inconsistent behavior in vacuum when it processes multiple relations

From: shihao zhong <[email-masked]>
Date: Wed, 18 Jun 2025 11:15:31 -0400

Hi team,

One of our customers recently encountered an issue with PostgreSQL's
pg_cron and VACUUM ANALYZE. They had configured a table with
vacuum_truncate=false to prevent exclusive lock contention, assuming
this would apply globally. However, VACUUM ANALYZE executed across the
entire database doesn't honor this table-specific setting, though
autovacuum does.

I investigated the code and found a small bug with how we're passing
the VacuumParams pointer.

The call flow is
ExecVacuum -> vacuum -> vacuum_rel

The initial VaccumParams pointer is set in ExecVacuum
In vacuum_rel, this pointer might change because it needs to determine
whether to truncate and perform index_cleanup.

Vacuuming a single table works as expected because the options are
applied at the table level. However, when vacuuming multiple tables,
the second table reuses the modified parameters set by the first
table's vacuum_rel.

We can easy repro that with following SQL and same with index_cleanup:
create table test(id int);
create table test_1(id int) with (vacuum_truncate=false);
insert into test select generate_series(1,1000);
insert into test_1 select generate_series(1,1000);
delete from test;
delete from test_1;
vacuum (analyze) test_1, test;
postgres=# \dt+
                                   List of relations
 Schema |  Name  | Type  |  Owner   | Persistence | Access method |
Size  | Description
--------+--------+-------+----------+-------------+---------------+-------+-------------
 public | test   | table | postgres | permanent   | heap          | 72 kB |
 public | test_1 | table | postgres | permanent   | heap          | 72 kB |
(2 rows)

I've implemented a fix and included a regression test in the patch.

Thanks,
Shihao

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 18 Jun 2025 11:31:50 -0500

> whether to truncate and perform index_cleanup.

Nice find!

My first reaction is to wonder whether we should 1) also make a similar
change to vacuum() for some future-proofing or 2) just teach vacuum_rel()
to make a local copy of the parameters that it can scribble on.  In the
latter case, we might want to assert that the parameters don't change after
calls to vacuum() and vacuum_rel() to prevent this problem from recurring.
That leads me to think (1) might be the better option, although I'm not too
wild about the subtlety of the fix.

-- 
nathan

---

From: shihao zhong <[email-masked]>
Date: Wed, 18 Jun 2025 14:48:16 -0400

>> wild about the subtlety of the fix.

Thanks for your feedback. New patch is attached. I also updated the
signature of do_analyze_rel for the same reason.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 19 Jun 2025 15:30:26 -0500

> signature of do_analyze_rel for the same reason.

After thinking about this some more, I'm wondering if it would be better to
pursue option (2) because it's a little less invasive (which is important
because this will need to be back-patched).  In any case, we have a similar
problem when recursing to the TOAST table, which can be fixed by copying
the params at the top of vacuum_rel().

While testing out the attached patch, I noticed a couple of other
interesting problems in this area [0].

[0] https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan

-- 
nathan
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 33a33bf6b1c..a43f090ee17 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -634,7 +634,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;
 			}
 
@@ -2008,9 +2016,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();
 
@@ -2299,15 +2314,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;

---

From: Michael Paquier <[email-masked]>
Date: Fri, 20 Jun 2025 10:34:19 +0900

> [0] https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan

Hmm.  I like the simplicity of option 2) for the purpose the back
branches and the post-feature-freeze v18.

However, Option 1) would be my go-to option for HEAD (as of v19
opening for business), but I think that we should harden the code more
than suggested and treat all VacuumParams as purely input arguments
rather keeping some pointers to it depending on the code path we are
dealing with, so as no callers of these inner routines is surprised by
changes that may happen internally.  Hence, reading the code of v2,
I'd suggest to apply the same rule to vacuum_get_cutoffs(),
do_analyze_rel() and heap_vacuum_rel().  Except if I am missing
something, it looks like all these calls should be OK with this new
policy.  This implies also changing relation_vacuum() in tableam.h,
which can be a HEAD-only change anyway.
--
Michael

---

From: shihao zhong <[email-masked]>
Date: Fri, 20 Jun 2025 10:13:38 -0400

>> However, Option 1) would be my go-to option for HEAD ...

Updated my patch to apply the same rules to all VacuumParams. Also I
am seeing clusterParams as a pass reference, not sure if we should
also change that to prevent future issues. But that should be another
patch.

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 20 Jun 2025 22:40:07 +0800

On Fri, Jun 20, 2025 at 9:34 AM Michael Paquier <[email-masked]> wrote:
> changes that may happen internally.

I'd suggest just marking the VacuumParams *params with const, so
that the user can not change its content, or the compiler will error
out, it will force the user to make a copy if changes are needed.
I see vacuum_get_cutoffs already has the const signature.

Passing by const pointer is more efficient than passing by structure value.

> Hence, reading the code of v2,

-- 
Regards
Junwang Zhao

---

From: Junwang Zhao <[email-masked]>
Date: Fri, 20 Jun 2025 22:45:44 +0800

> also change that to prevent future issues. But that should be another

 static inline void
-table_relation_vacuum(Relation rel, struct VacuumParams *params,
+table_relation_vacuum(Relation rel, struct VacuumParams params,
    BufferAccessStrategy bstrategy)
 {
  rel->rd_tableam->relation_vacuum(rel, params, bstrategy);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bc37a80dc74..9a8c63352da 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -336,7 +336,7 @@ extern PGDLLIMPORT int64 parallel_vacuum_worker_delay_ns;

 /* in commands/vacuum.c */
 extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool
isTopLevel);
-extern void vacuum(List *relations, VacuumParams *params,
+extern void vacuum(List *relations, VacuumParams params,
     BufferAccessStrategy bstrategy, MemoryContext vac_context,
     bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
@@ -357,7 +357,7 @@ extern void vac_update_relstats(Relation relation,
  bool *frozenxid_updated,
  bool *minmulti_updated,
  bool in_outer_xact);
-extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
+extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams params,
     struct VacuumCutoffs *cutoffs);
 extern bool vacuum_xid_failsafe_check(const struct VacuumCutoffs *cutoffs);
 extern void vac_update_datfrozenxid(void);
@@ -398,7 +398,7 @@ extern void parallel_vacuum_main(dsm_segment *seg,
shm_toc *toc);

 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, RangeVar *relation,
- VacuumParams *params, List *va_cols, bool in_outer_xact,
+ VacuumParams params, List *va_cols, bool in_outer_xact,

It's a bit odd that we have both `VacuumParams *params` and
`struct VacuumParams *params`. Perhaps you could remove
the struct keyword in this patch to make it consistent.

-- 
Regards
Junwang Zhao

---

From: shihao zhong <[email-masked]>
Date: Fri, 20 Jun 2025 11:54:07 -0400

>> `struct VacuumParams *params`.

Thanks for pointing that out, the attached new version fixed that.

>> I'd suggest just marking the VacuumParams *params with const...

I initially considered that approach. However, the current code path
for vacuum_rel handles table option re-consolidation, rather than the
ExecVacuum caller. This would require moving all parameter checks into
ExecVacuum, which I'm not sure is ideal. For this patch, let's focus
on resolving the inconsistent behavior. We can discuss parameter
re-consolidation further in this thread:
https://postgr.es/m/aFRxC1W_kZU9OjJ9%40nathan

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 20 Jun 2025 14:16:46 -0500

> branches and the post-feature-freeze v18.

Yeah, let's do this for now.

> which can be a HEAD-only change anyway.

Sounds reasonable to me.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Mon, 23 Jun 2025 08:48:35 +0900

>> which can be a HEAD-only change anyway.

Looking at the git history, the decision of being able to change
VacuumParams from the reloptions in vacuum_rel() for the index cleanup
dates back to a96c41feec6b (April 2019).  It has been copied a couple
of days later for truncate in b84dbc8eb80b (8th of May 2019).  The
introduction of VacuumParams comes from 0d831389749a back in 2015,
where more pointers to VacuumParams have been introduced for something
I've authored.  Perhaps we should have documented better the fact that
this structure should never be manipulated with const markers back
then, but what's done is done.  So I'm at the origin of the design
problem: even if the original refactoring of 0d831389749a did not
break things, it has encouraged the pattern we have to deal with today
because vacuum_rel() has begun this practice.

Anyway, here is an attempt at leaning all that.  I am really tempted
to add a couple of const markers to force VacuumParams to be in
read-only mode, even if it means doing so for vacuum() but not touch
at vacuum_rel() where we double-check the reloptions for the truncate
and index cleanup options.  That would be of course v19-only material.
Thoughts or opinions?
--
Michael
From 1e2f7cd189c5e7b2dad1207938d2f9a88667dcfa Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Mon, 23 Jun 2025 08:46:35 +0900
Subject: [PATCH v4] Make leaner the use of VacuumParams in the backend

The structure is marked as const where it can be, so as we force the
policy that callers should not change what has been given to them.
vacuum_rel() stands as an exception, as it manipulates truncate and
index_cleanup based on a reloptions lookup.
---
 src/include/access/heapam.h                  |  4 +-
 src/include/access/tableam.h                 |  6 +-
 src/include/commands/vacuum.h                |  6 +-
 src/backend/access/heap/vacuumlazy.c         | 44 ++++-----
 src/backend/commands/analyze.c               | 26 +++---
 src/backend/commands/cluster.c               |  2 +-
 src/backend/commands/vacuum.c                | 96 ++++++++++----------
 src/backend/postmaster/autovacuum.c          |  2 +-
 src/test/regress/expected/vacuum.out         | 28 ++++++
 src/test/regress/sql/vacuum.sql              | 14 +++
 contrib/pgstattuple/expected/pgstattuple.out | 16 ++++
 contrib/pgstattuple/sql/pgstattuple.sql      | 12 +++
 12 files changed, 161 insertions(+), 95 deletions(-)

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 3a9424c19c9a..a2bd5a897f87 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -21,6 +21,7 @@
 #include "access/skey.h"
 #include "access/table.h"		/* for backward compatibility */
 #include "access/tableam.h"
+#include "commands/vacuum.h"
 #include "nodes/lockoptions.h"
 #include "nodes/primnodes.h"
 #include "storage/bufpage.h"
@@ -396,9 +397,8 @@ extern void log_heap_prune_and_freeze(Relation relation, Buffer buffer,
 									  OffsetNumber *unused, int nunused);
 
 /* in heap/vacuumlazy.c */
-struct VacuumParams;
 extern void heap_vacuum_rel(Relation rel,
-							struct VacuumParams *params, BufferAccessStrategy bstrategy);
+							const VacuumParams params, BufferAccessStrategy bstrategy);
 
 /* in heap/heapam_visibility.c */
 extern bool HeapTupleSatisfiesVisibility(HeapTuple htup, Snapshot snapshot,
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 8713e12cbfb9..1c9e802a6b12 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -20,6 +20,7 @@
 #include "access/relscan.h"
 #include "access/sdir.h"
 #include "access/xact.h"
+#include "commands/vacuum.h"
 #include "executor/tuptable.h"
 #include "storage/read_stream.h"
 #include "utils/rel.h"
@@ -36,7 +37,6 @@ extern PGDLLIMPORT bool synchronize_seqscans;
 struct BulkInsertStateData;
 struct IndexInfo;
 struct SampleScanState;
-struct VacuumParams;
 struct ValidateIndexState;
 
 /*
@@ -645,7 +645,7 @@ typedef struct TableAmRoutine
 	 * integrate with autovacuum's scheduling.
 	 */
 	void		(*relation_vacuum) (Relation rel,
-									struct VacuumParams *params,
+									const VacuumParams params,
 									BufferAccessStrategy bstrategy);
 
 	/*
@@ -1664,7 +1664,7 @@ table_relation_copy_for_cluster(Relation OldTable, Relation NewTable,
  * routine, even if (for ANALYZE) it is part of the same VACUUM command.
  */
 static inline void
-table_relation_vacuum(Relation rel, struct VacuumParams *params,
+table_relation_vacuum(Relation rel, const VacuumParams params,
 					  BufferAccessStrategy bstrategy)
 {
 	rel->rd_tableam->relation_vacuum(rel, params, bstrategy);
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bc37a80dc74f..14eeccbd7185 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -336,7 +336,7 @@ extern PGDLLIMPORT int64 parallel_vacuum_worker_delay_ns;
 
 /* in commands/vacuum.c */
 extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel);
-extern void vacuum(List *relations, VacuumParams *params,
+extern void vacuum(List *relations, const VacuumParams params,
 				   BufferAccessStrategy bstrategy, MemoryContext vac_context,
 				   bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
@@ -357,7 +357,7 @@ extern void vac_update_relstats(Relation relation,
 								bool *frozenxid_updated,
 								bool *minmulti_updated,
 								bool in_outer_xact);
-extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
+extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams params,
 							   struct VacuumCutoffs *cutoffs);
 extern bool vacuum_xid_failsafe_check(const struct VacuumCutoffs *cutoffs);
 extern void vac_update_datfrozenxid(void);
@@ -398,7 +398,7 @@ extern void parallel_vacuum_main(dsm_segment *seg, shm_toc *toc);
 
 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, RangeVar *relation,
-						VacuumParams *params, List *va_cols, bool in_outer_xact,
+						const VacuumParams params, List *va_cols, bool in_outer_xact,
 						BufferAccessStrategy bstrategy);
 extern bool std_typanalyze(VacAttrStats *stats);
 
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 09416450af96..56d456736da0 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -423,7 +423,7 @@ typedef struct LVSavedErrInfo
 /* non-export function prototypes */
 static void lazy_scan_heap(LVRelState *vacrel);
 static void heap_vacuum_eager_scan_setup(LVRelState *vacrel,
-										 VacuumParams *params);
+										 const VacuumParams params);
 static BlockNumber heap_vac_scan_next_block(ReadStream *stream,
 											void *callback_private_data,
 											void *per_buffer_data);
@@ -485,7 +485,7 @@ static void restore_vacuum_error_info(LVRelState *vacrel,
  * vacuum options or for relfrozenxid/relminmxid advancement.
  */
 static void
-heap_vacuum_eager_scan_setup(LVRelState *vacrel, VacuumParams *params)
+heap_vacuum_eager_scan_setup(LVRelState *vacrel, const VacuumParams params)
 {
 	uint32		randseed;
 	BlockNumber allvisible;
@@ -504,7 +504,7 @@ heap_vacuum_eager_scan_setup(LVRelState *vacrel, VacuumParams *params)
 	vacrel->eager_scan_remaining_successes = 0;
 
 	/* If eager scanning is explicitly disabled, just return. */
-	if (params->max_eager_freeze_failure_rate == 0)
+	if (params.max_eager_freeze_failure_rate == 0)
 		return;
 
 	/*
@@ -581,11 +581,11 @@ heap_vacuum_eager_scan_setup(LVRelState *vacrel, VacuumParams *params)
 
 	vacrel->next_eager_scan_region_start = randseed % EAGER_SCAN_REGION_SIZE;
 
-	Assert(params->max_eager_freeze_failure_rate > 0 &&
-		   params->max_eager_freeze_failure_rate <= 1);
+	Assert(params.max_eager_freeze_failure_rate > 0 &&
+		   params.max_eager_freeze_failure_rate <= 1);
 
 	vacrel->eager_scan_max_fails_per_region =
-		params->max_eager_freeze_failure_rate *
+		params.max_eager_freeze_failure_rate *
 		EAGER_SCAN_REGION_SIZE;
 
 	/*
@@ -612,7 +612,7 @@ heap_vacuum_eager_scan_setup(LVRelState *vacrel, VacuumParams *params)
  *		and locked the relation.
  */
 void
-heap_vacuum_rel(Relation rel, VacuumParams *params,
+heap_vacuum_rel(Relation rel, const VacuumParams params,
 				BufferAccessStrategy bstrategy)
 {
 	LVRelState *vacrel;
@@ -634,9 +634,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	ErrorContextCallback errcallback;
 	char	  **indnames = NULL;
 
-	verbose = (params->options & VACOPT_VERBOSE) != 0;
+	verbose = (params.options & VACOPT_VERBOSE) != 0;
 	instrument = (verbose || (AmAutoVacuumWorkerProcess() &&
-							  params->log_min_duration >= 0));
+							  params.log_min_duration >= 0));
 	if (instrument)
 	{
 		pg_rusage_init(&ru0);
@@ -699,9 +699,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	 * The truncate param allows user to avoid attempting relation truncation,
 	 * though it can't force truncation to happen.
 	 */
-	Assert(params->index_cleanup != VACOPTVALUE_UNSPECIFIED);
-	Assert(params->truncate != VACOPTVALUE_UNSPECIFIED &&
-		   params->truncate != VACOPTVALUE_AUTO);
+	Assert(params.index_cleanup != VACOPTVALUE_UNSPECIFIED);
+	Assert(params.truncate != VACOPTVALUE_UNSPECIFIED &&
+		   params.truncate != VACOPTVALUE_AUTO);
 
 	/*
 	 * While VacuumFailSafeActive is reset to false before calling this, we
@@ -711,14 +711,14 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	vacrel->consider_bypass_optimization = true;
 	vacrel->do_index_vacuuming = true;
 	vacrel->do_index_cleanup = true;
-	vacrel->do_rel_truncate = (params->truncate != VACOPTVALUE_DISABLED);
-	if (params->index_cleanup == VACOPTVALUE_DISABLED)
+	vacrel->do_rel_truncate = (params.truncate != VACOPTVALUE_DISABLED);
+	if (params.index_cleanup == VACOPTVALUE_DISABLED)
 	{
 		/* Force disable index vacuuming up-front */
 		vacrel->do_index_vacuuming = false;
 		vacrel->do_index_cleanup = false;
 	}
-	else if (params->index_cleanup == VACOPTVALUE_ENABLED)
+	else if (params.index_cleanup == VACOPTVALUE_ENABLED)
 	{
 		/* Force index vacuuming.  Note that failsafe can still bypass. */
 		vacrel->consider_bypass_optimization = false;
@@ -726,7 +726,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	else
 	{
 		/* Default/auto, make all decisions dynamically */
-		Assert(params->index_cleanup == VACOPTVALUE_AUTO);
+		Assert(params.index_cleanup == VACOPTVALUE_AUTO);
 	}
 
 	/* Initialize page counters explicitly (be tidy) */
@@ -789,7 +789,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	 */
 	vacrel->skippedallvis = false;
 	skipwithvm = true;
-	if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
+	if (params.options & VACOPT_DISABLE_PAGE_SKIPPING)
 	{
 		/*
 		 * Force aggressive mode, and disable skipping blocks using the
@@ -830,7 +830,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	 * is already dangerously old.)
 	 */
 	lazy_check_wraparound_failsafe(vacrel);
-	dead_items_alloc(vacrel, params->nworkers);
+	dead_items_alloc(vacrel, params.nworkers);
 
 	/*
 	 * Call lazy_scan_heap to perform all required heap pruning, index
@@ -947,9 +947,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 	{
 		TimestampTz endtime = GetCurrentTimestamp();
 
-		if (verbose || params->log_min_duration == 0 ||
+		if (verbose || params.log_min_duration == 0 ||
 			TimestampDifferenceExceeds(starttime, endtime,
-									   params->log_min_duration))
+									   params.log_min_duration))
 		{
 			long		secs_dur;
 			int			usecs_dur;
@@ -984,10 +984,10 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
 				 * Aggressiveness already reported earlier, in dedicated
 				 * VACUUM VERBOSE ereport
 				 */
-				Assert(!params->is_wraparound);
+				Assert(!params.is_wraparound);
 				msgfmt = _("finished vacuuming \"%s.%s.%s\": index scans: %d\n");
 			}
-			else if (params->is_wraparound)
+			else if (params.is_wraparound)
 			{
 				/*
 				 * While it's possible for a VACUUM to be both is_wraparound
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index 4fffb76e5573..7111d5d5334f 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -76,7 +76,7 @@ static BufferAccessStrategy vac_strategy;

 static void do_analyze_rel(Relation onerel,
-						   VacuumParams *params, List *va_cols,
+						   const VacuumParams params, List *va_cols,
 						   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 						   bool inh, bool in_outer_xact, int elevel);
 static void compute_index_stats(Relation onerel, double totalrows,
@@ -107,7 +107,7 @@ static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
  */
 void
 analyze_rel(Oid relid, RangeVar *relation,
-			VacuumParams *params, List *va_cols, bool in_outer_xact,
+			const VacuumParams params, List *va_cols, bool in_outer_xact,
 			BufferAccessStrategy bstrategy)
 {
 	Relation	onerel;
@@ -116,7 +116,7 @@ analyze_rel(Oid relid, RangeVar *relation,
 	BlockNumber relpages = 0;
 
 	/* Select logging level */
-	if (params->options & VACOPT_VERBOSE)
+	if (params.options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -138,8 +138,8 @@ analyze_rel(Oid relid, RangeVar *relation,
 	 *
 	 * Make sure to generate only logs for ANALYZE in this case.
 	 */
-	onerel = vacuum_open_relation(relid, relation, params->options & ~(VACOPT_VACUUM),
-								  params->log_min_duration >= 0,
+	onerel = vacuum_open_relation(relid, relation, params.options & ~(VACOPT_VACUUM),
+								  params.log_min_duration >= 0,
 								  ShareUpdateExclusiveLock);
 
 	/* leave if relation could not be opened or locked */
@@ -155,7 +155,7 @@ analyze_rel(Oid relid, RangeVar *relation,
 	 */
 	if (!vacuum_is_permitted_for_relation(RelationGetRelid(onerel),
 										  onerel->rd_rel,
-										  params->options & ~VACOPT_VACUUM))
+										  params.options & ~VACOPT_VACUUM))
 	{
 		relation_close(onerel, ShareUpdateExclusiveLock);
 		return;
@@ -227,7 +227,7 @@ analyze_rel(Oid relid, RangeVar *relation,
 	else
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(params->options & VACOPT_VACUUM))
+		if (!(params.options & VACOPT_VACUUM))
 			ereport(WARNING,
 					(errmsg("skipping \"%s\" --- cannot analyze non-tables or special system tables",
 							RelationGetRelationName(onerel))));
@@ -275,7 +275,7 @@ analyze_rel(Oid relid, RangeVar *relation,
  * appropriate acquirefunc for each child table.
  */
 static void
-do_analyze_rel(Relation onerel, VacuumParams *params,
+do_analyze_rel(Relation onerel, const VacuumParams params,
 			   List *va_cols, AcquireSampleRowsFunc acquirefunc,
 			   BlockNumber relpages, bool inh, bool in_outer_xact,
 			   int elevel)
@@ -309,9 +309,9 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	PgStat_Counter startreadtime = 0;
 	PgStat_Counter startwritetime = 0;
 
-	verbose = (params->options & VACOPT_VERBOSE) != 0;
+	verbose = (params.options & VACOPT_VERBOSE) != 0;
 	instrument = (verbose || (AmAutoVacuumWorkerProcess() &&
-							  params->log_min_duration >= 0));
+							  params.log_min_duration >= 0));
 	if (inh)
 		ereport(elevel,
 				(errmsg("analyzing \"%s.%s\" inheritance tree",
@@ -706,7 +706,7 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	 * amvacuumcleanup() when called in ANALYZE-only mode.  The only exception
 	 * among core index AMs is GIN/ginvacuumcleanup().
 	 */
-	if (!(params->options & VACOPT_VACUUM))
+	if (!(params.options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
@@ -736,9 +736,9 @@ do_analyze_rel(Relation onerel, VacuumParams *params,
 	{
 		TimestampTz endtime = GetCurrentTimestamp();
 
-		if (verbose || params->log_min_duration == 0 ||
+		if (verbose || params.log_min_duration == 0 ||
 			TimestampDifferenceExceeds(starttime, endtime,
-									   params->log_min_duration))
+									   params.log_min_duration))
 		{
 			long		delay_in_ms;
 			WalUsage	walusage;
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 54a08e4102e1..b55221d44cd0 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -917,7 +917,7 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, bool verb
 	 * not to be aggressive about this.
 	 */
 	memset(&params, 0, sizeof(VacuumParams));
-	vacuum_get_cutoffs(OldHeap, &params, &cutoffs);
+	vacuum_get_cutoffs(OldHeap, params, &cutoffs);
 
 	/*
 	 * FreezeXid will become the table's new relfrozenxid, and that mustn't go
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 33a33bf6b1cf..c9a70a48be47 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -123,7 +123,7 @@ static void vac_truncate_clog(TransactionId frozenXID,
 							  MultiXactId minMulti,
 							  TransactionId lastSaneFrozenXid,
 							  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
 					   BufferAccessStrategy bstrategy);
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
@@ -464,7 +464,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	}
 
 	/* Now go through the common routine */
-	vacuum(vacstmt->rels, &params, bstrategy, vac_context, isTopLevel);
+	vacuum(vacstmt->rels, params, bstrategy, vac_context, isTopLevel);
 
 	/* Finally, clean up the vacuum memory context */
 	MemoryContextDelete(vac_context);
@@ -493,7 +493,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
  * memory context that will not disappear at transaction commit.
  */
 void
-vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
+vacuum(List *relations, const VacuumParams params, BufferAccessStrategy bstrategy,
 	   MemoryContext vac_context, bool isTopLevel)
 {
 	static bool in_vacuum = false;
@@ -502,9 +502,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 	volatile bool in_outer_xact,
 				use_own_xacts;
 
-	Assert(params != NULL);
-
-	stmttype = (params->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
+	stmttype = (params.options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
 	/*
 	 * We cannot run VACUUM inside a user transaction block; if we were inside
@@ -514,7 +512,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (params->options & VACOPT_VACUUM)
+	if (params.options & VACOPT_VACUUM)
 	{
 		PreventInTransactionBlock(isTopLevel, stmttype);
 		in_outer_xact = false;
@@ -537,7 +535,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 	 * Build list of relation(s) to process, putting any new data in
 	 * vac_context for safekeeping.
 	 */
-	if (params->options & VACOPT_ONLY_DATABASE_STATS)
+	if (params.options & VACOPT_ONLY_DATABASE_STATS)
 	{
 		/* We don't process any tables in this case */
 		Assert(relations == NIL);
@@ -553,7 +551,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 			List	   *sublist;
 			MemoryContext old_context;
 
-			sublist = expand_vacuum_rel(vrel, vac_context, params->options);
+			sublist = expand_vacuum_rel(vrel, vac_context, params.options);
 			old_context = MemoryContextSwitchTo(vac_context);
 			newrels = list_concat(newrels, sublist);
 			MemoryContextSwitchTo(old_context);
@@ -561,7 +559,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 		relations = newrels;
 	}
 	else
-		relations = get_all_vacuum_rels(vac_context, params->options);
+		relations = get_all_vacuum_rels(vac_context, params.options);
 
 	/*
 	 * Decide whether we need to start/commit our own transactions.
@@ -577,11 +575,11 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 	 * transaction block, and also in an autovacuum worker, use own
 	 * transactions so we can release locks sooner.
 	 */
-	if (params->options & VACOPT_VACUUM)
+	if (params.options & VACOPT_VACUUM)
 		use_own_xacts = true;
 	else
 	{
-		Assert(params->options & VACOPT_ANALYZE);
+		Assert(params.options & VACOPT_ANALYZE);
 		if (AmAutoVacuumWorkerProcess())
 			use_own_xacts = true;
 		else if (in_outer_xact)
@@ -632,13 +630,13 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 		{
 			VacuumRelation *vrel = lfirst_node(VacuumRelation, cur);
 
-			if (params->options & VACOPT_VACUUM)
+			if (params.options & VACOPT_VACUUM)
 			{
 				if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy))
 					continue;
 			}
 
-			if (params->options & VACOPT_ANALYZE)
+			if (params.options & VACOPT_ANALYZE)
 			{
 				/*
 				 * If using separate xacts, start one for analyze. Otherwise,
@@ -702,8 +700,8 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 		StartTransactionCommand();
 	}
 
-	if ((params->options & VACOPT_VACUUM) &&
-		!(params->options & VACOPT_SKIP_DATABASE_STATS))
+	if ((params.options & VACOPT_VACUUM) &&
+		!(params.options & VACOPT_SKIP_DATABASE_STATS))
 	{
 		/*
 		 * Update pg_database.datfrozenxid, and truncate pg_xact if possible.
@@ -1101,7 +1099,7 @@ get_all_vacuum_rels(MemoryContext vac_context, int options)
  * minimum).
  */
 bool
-vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
+vacuum_get_cutoffs(Relation rel, const VacuumParams params,
 				   struct VacuumCutoffs *cutoffs)
 {
 	int			freeze_min_age,
@@ -1117,10 +1115,10 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
 				aggressiveMXIDCutoff;
 
 	/* Use mutable copies of freeze age parameters */
-	freeze_min_age = params->freeze_min_age;
-	multixact_freeze_min_age = params->multixact_freeze_min_age;
-	freeze_table_age = params->freeze_table_age;
-	multixact_freeze_table_age = params->multixact_freeze_table_age;
+	freeze_min_age = params.freeze_min_age;
+	multixact_freeze_min_age = params.multixact_freeze_min_age;
+	freeze_table_age = params.freeze_table_age;
+	multixact_freeze_table_age = params.multixact_freeze_table_age;
 
 	/* Set pg_class fields in cutoffs */
 	cutoffs->relfrozenxid = rel->rd_rel->relfrozenxid;
@@ -1997,7 +1995,7 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
 		   BufferAccessStrategy bstrategy)
 {
 	LOCKMODE	lmode;
@@ -2009,12 +2007,10 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	int			save_sec_context;
 	int			save_nestlevel;
 
-	Assert(params != NULL);
-
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
 
-	if (!(params->options & VACOPT_FULL))
+	if (!(params.options & VACOPT_FULL))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -2040,7 +2036,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 		 */
 		LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
 		MyProc->statusFlags |= PROC_IN_VACUUM;
-		if (params->is_wraparound)
+		if (params.is_wraparound)
 			MyProc->statusFlags |= PROC_VACUUM_FOR_WRAPAROUND;
 		ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 		LWLockRelease(ProcArrayLock);
@@ -2064,12 +2060,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 * vacuum, but just ShareUpdateExclusiveLock for concurrent vacuum. Either
 	 * way, we can be sure that no other backend is vacuuming the same table.
 	 */
-	lmode = (params->options & VACOPT_FULL) ?
+	lmode = (params.options & VACOPT_FULL) ?
 		AccessExclusiveLock : ShareUpdateExclusiveLock;
 
 	/* open the relation and get the appropriate lock on it */
-	rel = vacuum_open_relation(relid, relation, params->options,
-							   params->log_min_duration >= 0, lmode);
+	rel = vacuum_open_relation(relid, relation, params.options,
+							   params.log_min_duration >= 0, lmode);
 
 	/* leave if relation could not be opened or locked */
 	if (!rel)
@@ -2084,8 +2080,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 * This is only safe to do because we hold a session lock on the main
 	 * relation that prevents concurrent deletion.
 	 */
-	if (OidIsValid(params->toast_parent))
-		priv_relid = params->toast_parent;
+	if (OidIsValid(params.toast_parent))
+		priv_relid = params.toast_parent;
 	else
 		priv_relid = RelationGetRelid(rel);
 
@@ -2098,7 +2094,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
 	if (!vacuum_is_permitted_for_relation(priv_relid,
 										  rel->rd_rel,
-										  params->options & ~VACOPT_ANALYZE))
+										  params.options & ~VACOPT_ANALYZE))
 	{
 		relation_close(rel, lmode);
 		PopActiveSnapshot();
@@ -2169,7 +2165,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 * Set index_cleanup option based on index_cleanup reloption if it wasn't
 	 * specified in VACUUM command, or when running in an autovacuum worker
 	 */
-	if (params->index_cleanup == VACOPTVALUE_UNSPECIFIED)
+	if (params.index_cleanup == VACOPTVALUE_UNSPECIFIED)
 	{
 		StdRdOptIndexCleanup vacuum_index_cleanup;
 
@@ -2180,14 +2176,14 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 				((StdRdOptions *) rel->rd_options)->vacuum_index_cleanup;
 
 		if (vacuum_index_cleanup == STDRD_OPTION_VACUUM_INDEX_CLEANUP_AUTO)
-			params->index_cleanup = VACOPTVALUE_AUTO;
+			params.index_cleanup = VACOPTVALUE_AUTO;
 		else if (vacuum_index_cleanup == STDRD_OPTION_VACUUM_INDEX_CLEANUP_ON)
-			params->index_cleanup = VACOPTVALUE_ENABLED;
+			params.index_cleanup = VACOPTVALUE_ENABLED;
 		else
 		{
 			Assert(vacuum_index_cleanup ==
 				   STDRD_OPTION_VACUUM_INDEX_CLEANUP_OFF);
-			params->index_cleanup = VACOPTVALUE_DISABLED;
+			params.index_cleanup = VACOPTVALUE_DISABLED;
 		}
 	}
 
@@ -2197,28 +2193,28 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
 	if (rel->rd_options != NULL &&
 		((StdRdOptions *) rel->rd_options)->vacuum_max_eager_freeze_failure_rate >= 0)
-		params->max_eager_freeze_failure_rate =
+		params.max_eager_freeze_failure_rate =
 			((StdRdOptions *) rel->rd_options)->vacuum_max_eager_freeze_failure_rate;
 
 	/*
 	 * Set truncate option based on truncate reloption or GUC if it wasn't
 	 * specified in VACUUM command, or when running in an autovacuum worker
 	 */
-	if (params->truncate == VACOPTVALUE_UNSPECIFIED)
+	if (params.truncate == VACOPTVALUE_UNSPECIFIED)
 	{
 		StdRdOptions *opts = (StdRdOptions *) rel->rd_options;
 
 		if (opts && opts->vacuum_truncate_set)
 		{
 			if (opts->vacuum_truncate)
-				params->truncate = VACOPTVALUE_ENABLED;
+				params.truncate = VACOPTVALUE_ENABLED;
 			else
-				params->truncate = VACOPTVALUE_DISABLED;
+				params.truncate = VACOPTVALUE_DISABLED;
 		}
 		else if (vacuum_truncate)
-			params->truncate = VACOPTVALUE_ENABLED;
+			params.truncate = VACOPTVALUE_ENABLED;
 		else
-			params->truncate = VACOPTVALUE_DISABLED;
+			params.truncate = VACOPTVALUE_DISABLED;
 	}
 
 	/*
@@ -2227,9 +2223,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 * automatically rebuilt by cluster_rel so we shouldn't recurse to it,
 	 * unless PROCESS_MAIN is disabled.
 	 */
-	if ((params->options & VACOPT_PROCESS_TOAST) != 0 &&
-		((params->options & VACOPT_FULL) == 0 ||
-		 (params->options & VACOPT_PROCESS_MAIN) == 0))
+	if ((params.options & VACOPT_PROCESS_TOAST) != 0 &&
+		((params.options & VACOPT_FULL) == 0 ||
+		 (params.options & VACOPT_PROCESS_MAIN) == 0))
 		toast_relid = rel->rd_rel->reltoastrelid;
 	else
 		toast_relid = InvalidOid;
@@ -2252,16 +2248,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 * table is required (e.g., PROCESS_TOAST is set), we force PROCESS_MAIN
 	 * to be set when we recurse to the TOAST table.
 	 */
-	if (params->options & VACOPT_PROCESS_MAIN)
+	if (params.options & VACOPT_PROCESS_MAIN)
 	{
 		/*
 		 * Do the actual work --- either FULL or "lazy" vacuum
 		 */
-		if (params->options & VACOPT_FULL)
+		if (params.options & VACOPT_FULL)
 		{
 			ClusterParams cluster_params = {0};
 
-			if ((params->options & VACOPT_VERBOSE) != 0)
+			if ((params.options & VACOPT_VERBOSE) != 0)
 				cluster_params.options |= CLUOPT_VERBOSE;
 
 			/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
@@ -2307,11 +2303,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 		 * 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));
+		memcpy(&toast_vacuum_params, &params, sizeof(VacuumParams));
 		toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
 		toast_vacuum_params.toast_parent = relid;
 
-		vacuum_rel(toast_relid, NULL, &toast_vacuum_params, bstrategy);
+		vacuum_rel(toast_relid, NULL, toast_vacuum_params, bstrategy);
 	}
 
 	/*
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 451fb90a610a..9474095f271a 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -3190,7 +3190,7 @@ autovacuum_do_vac_analyze(autovac_table *tab, BufferAccessStrategy bstrategy)
 	rel_list = list_make1(rel);
 	MemoryContextSwitchTo(old_context);
 
-	vacuum(rel_list, &tab->at_params, bstrategy, vac_context, true);
+	vacuum(rel_list, tab->at_params, bstrategy, vac_context, true);
 
 	MemoryContextDelete(vac_context);
 }
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 0abcc99989e0..284b5391b48b 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -686,3 +686,31 @@ RESET ROLE;
 DROP TABLE vacowned;
 DROP TABLE vacowned_parted;
 DROP ROLE regress_vacuum;
+-- TRUNCATE option with vacuum more than 1 relation
+CREATE TABLE vac_truncate_on(i INT) WITH (autovacuum_enabled=false, vacuum_truncate=true);
+INSERT INTO vac_truncate_on SELECT generate_series(1, 1000);
+CREATE TABLE vac_truncate_off(i INT) WITH (autovacuum_enabled=false, vacuum_truncate=false);
+INSERT INTO vac_truncate_off SELECT generate_series(1, 1000);
+SELECT pg_relation_size('vac_truncate_on') > 0;
+ ?column? 
+----------
+ t
+(1 row)
+
+SELECT pg_relation_size('vac_truncate_off') > 0;
+ ?column? 
+----------
+ t
+(1 row)
+
+DELETE FROM vac_truncate_on;
+DELETE FROM vac_truncate_off;
+VACUUM (ANALYZE) vac_truncate_on, vac_truncate_off;
+SELECT pg_relation_size('vac_truncate_off') > 0;
+ ?column? 
+----------
+ t
+(1 row)
+
+DROP TABLE vac_truncate_on;
+DROP TABLE vac_truncate_off;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index a72bdb5b619d..95330a5d44a4 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -495,3 +495,17 @@ RESET ROLE;
 DROP TABLE vacowned;
 DROP TABLE vacowned_parted;
 DROP ROLE regress_vacuum;
+
+-- TRUNCATE option with vacuum more than 1 relation
+CREATE TABLE vac_truncate_on(i INT) WITH (autovacuum_enabled=false, vacuum_truncate=true);
+INSERT INTO vac_truncate_on SELECT generate_series(1, 1000);
+CREATE TABLE vac_truncate_off(i INT) WITH (autovacuum_enabled=false, vacuum_truncate=false);
+INSERT INTO vac_truncate_off SELECT generate_series(1, 1000);
+SELECT pg_relation_size('vac_truncate_on') > 0;
+SELECT pg_relation_size('vac_truncate_off') > 0;
+DELETE FROM vac_truncate_on;
+DELETE FROM vac_truncate_off;
+VACUUM (ANALYZE) vac_truncate_on, vac_truncate_off;
+SELECT pg_relation_size('vac_truncate_off') > 0;
+DROP TABLE vac_truncate_on;
+DROP TABLE vac_truncate_off;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 9176dc98b6a9..646c8c11e6e9 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -303,3 +303,19 @@ drop view test_view;
 drop foreign table test_foreign_table;
 drop server dummy_server;
 drop foreign data wrapper dummy;
+-- INDEX_CLEANUP option with vacuum more than 1 relation
+CREATE TABLE vac_index_cleanup_on(i INT PRIMARY KEY) WITH (autovacuum_enabled=false, vacuum_index_cleanup=true);
+INSERT INTO vac_index_cleanup_on SELECT generate_series(1, 1000);
+CREATE TABLE vac_index_cleanup_off(i INT PRIMARY KEY) WITH (autovacuum_enabled=false, vacuum_index_cleanup=false);
+INSERT INTO vac_index_cleanup_off SELECT generate_series(1, 1000);
+DELETE FROM vac_index_cleanup_on;
+DELETE FROM vac_index_cleanup_off;
+VACUUM (ANALYZE) vac_index_cleanup_on, vac_index_cleanup_off;
+SELECT deleted_pages = 0 FROM pgstatindex('vac_index_cleanup_off_pkey');
+ ?column? 
+----------
+ t
+(1 row)
+
+DROP TABLE vac_index_cleanup_on;
+DROP TABLE vac_index_cleanup_off;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 7e72c567a064..660280ee2474 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -136,3 +136,15 @@ drop view test_view;
 drop foreign table test_foreign_table;
 drop server dummy_server;
 drop foreign data wrapper dummy;
+
+-- INDEX_CLEANUP option with vacuum more than 1 relation
+CREATE TABLE vac_index_cleanup_on(i INT PRIMARY KEY) WITH (autovacuum_enabled=false, vacuum_index_cleanup=true);
+INSERT INTO vac_index_cleanup_on SELECT generate_series(1, 1000);
+CREATE TABLE vac_index_cleanup_off(i INT PRIMARY KEY) WITH (autovacuum_enabled=false, vacuum_index_cleanup=false);
+INSERT INTO vac_index_cleanup_off SELECT generate_series(1, 1000);
+DELETE FROM vac_index_cleanup_on;
+DELETE FROM vac_index_cleanup_off;
+VACUUM (ANALYZE) vac_index_cleanup_on, vac_index_cleanup_off;
+SELECT deleted_pages = 0 FROM pgstatindex('vac_index_cleanup_off_pkey');
+DROP TABLE vac_index_cleanup_on;
+DROP TABLE vac_index_cleanup_off;
-- 
2.50.0

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 23 Jun 2025 10:38:40 -0500

> and index cleanup options.  That would be of course v19-only material.

Is the idea to do something like this for v19 and this [0] for the
back-branches?

I think the recurse-to-TOAST-table case is still broken with your patch.
We should probably move the memcpy() for toast_vacuum_params to the very
top of vacuum_rel().  Otherwise, your patch looks generally reasonable to
me.

[0] https://postgr.es/m/aFRzYhOTZcRgKPLu%40nathan

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Tue, 24 Jun 2025 10:18:18 +0900

> Is the idea to do something like this for v19 and this [0] for the

Yes, that would be the idea.  Let's do the bug fix first on all the
branches, then do the refactoring.  Knowing that I'm indirectly
responsible for this mess, I would like to take care of that myself.
Would that be OK for you?

> top of vacuum_rel().  Otherwise, your patch looks generally reasonable to

Ah, right, I have managed to mess up this part.  Sounds to me that we
should provide more coverage for both the truncate and index cleanup
cases, as well.

The updated version attached includes tests that were enough to make
the recursive TOAST table case fail for TRUNCATE and INDEX_CLEANUP,
where I have relied on an EXTERNAL column to fill in the TOAST
relation with data cheaply.  The pgstattuple case is less cheap but
I've minimized the load to be able to trigger the index cleanup.  It
was tricky to get the threshold right for INDEX_CLEANUP in the case of
TOAST table, but that's small enough to have a short run time while it
should be large enough to trigger a cleanup of the TOAST indexes (my
CI quota has been reached this month, so I can only rely on the CF bot
for some pre-validation job).  If the thresholds are too low, we may
need to bump the number of tuples.  I'd be OK to remove the TOAST
parts if these prove to be unstable, but let's see.  At least these
tests are validating the contents of the patch for the TOAST options,
and they were looking stable on my machine.

Another approach that we could use is an injection point with some
LOGs that show some information based on what VacuumParams holds.
That would be the cheapest method (no need for any data), and entirely
stable as we would look at the stack.  Perhaps going down to that is
not really necessary for the sake of this thread.

Attached are two patches to have a clean git history:
- 0001 is the basic fix, to-be-applied first on all the branches.
- 0002 is the refactoring, for v19 once it opens up.

What do you think?
--
Michael
From 2f2390c05725695d2e2d60845e5146710926fbbc Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 24 Jun 2025 10:06:09 +0900
Subject: [PATCH v4 1/2] Avoid scribbling VACUUM options

Blah, this is the basic patch that goes through all the back branches,
with tests.

Author: Nathan B.
Backpatch-through: 13
---
 src/backend/commands/vacuum.c                | 20 ++++--
 src/test/regress/expected/vacuum.out         | 71 ++++++++++++++++++++
 src/test/regress/sql/vacuum.sql              | 32 +++++++++
 contrib/pgstattuple/expected/pgstattuple.out | 47 +++++++++++++
 contrib/pgstattuple/sql/pgstattuple.sql      | 32 +++++++++
 5 files changed, 198 insertions(+), 4 deletions(-)

index 33a33bf6b1cf..a43f090ee178 100644

 
 	Assert(params != NULL);
 
+	/*

diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index 0abcc99989e0..f403ac190601 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -686,3 +686,74 @@ RESET ROLE;
 DROP TABLE vacowned;
 DROP TABLE vacowned_parted;
 DROP ROLE regress_vacuum;
+-- TRUNCATE option with VACUUM of more than 1 relation.
+CREATE TABLE vac_truncate_on_toast_off(i int, j text) WITH
+  (autovacuum_enabled=false, vacuum_truncate=true, toast.vacuum_truncate=false);
+CREATE TABLE vac_truncate_off_toast_on(i int, j text) WITH
+  (autovacuum_enabled=false, vacuum_truncate=false, toast.vacuum_truncate=true);
+-- EXTERNAL to force data on TOAST table, uncompressed.
+ALTER TABLE vac_truncate_on_toast_off ALTER COLUMN j SET STORAGE EXTERNAL;
+ALTER TABLE vac_truncate_off_toast_on ALTER COLUMN j SET STORAGE EXTERNAL;
+INSERT INTO vac_truncate_on_toast_off SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_truncate_on_toast_off SELECT generate_series(1000, 1010), repeat('1234567890', 1000);
+INSERT INTO vac_truncate_off_toast_on SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_truncate_off_toast_on SELECT generate_series(1000, 1010), repeat('1234567890', 1000);
+SELECT pg_relation_size('vac_truncate_on_toast_off') > 0 AS has_data;
+ has_data 
+----------
+ t
+(1 row)
+
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_on_toast_off';
+ has_data 
+----------
+ t
+(1 row)
+
+SELECT pg_relation_size('vac_truncate_off_toast_on') > 0 AS has_data;
+ has_data 
+----------
+ t
+(1 row)
+
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_off_toast_on';
+ has_data 
+----------
+ t
+(1 row)
+
+DELETE FROM vac_truncate_on_toast_off;
+DELETE FROM vac_truncate_off_toast_on;
+-- TRUNCATE options are retrieved from their respective relations.
+-- Do an aggressive VACUUM to prevent page-skipping.
+VACUUM (FREEZE, ANALYZE) vac_truncate_on_toast_off, vac_truncate_off_toast_on;
+SELECT pg_relation_size('vac_truncate_on_toast_off') > 0 AS has_data;
+ has_data 
+----------
+ f
+(1 row)
+
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_on_toast_off';
+ has_data 
+----------
+ t
+(1 row)
+
+SELECT pg_relation_size('vac_truncate_off_toast_on') > 0 AS has_data;
+ has_data 
+----------
+ t
+(1 row)
+
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_off_toast_on';
+ has_data 
+----------
+ f
+(1 row)
+
+DROP TABLE vac_truncate_on_toast_off;
+DROP TABLE vac_truncate_off_toast_on;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index a72bdb5b619d..234753f4e1a1 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -495,3 +495,35 @@ RESET ROLE;
 DROP TABLE vacowned;
 DROP TABLE vacowned_parted;
 DROP ROLE regress_vacuum;
+
+-- TRUNCATE option with VACUUM of more than 1 relation.
+CREATE TABLE vac_truncate_on_toast_off(i int, j text) WITH
+  (autovacuum_enabled=false, vacuum_truncate=true, toast.vacuum_truncate=false);
+CREATE TABLE vac_truncate_off_toast_on(i int, j text) WITH
+  (autovacuum_enabled=false, vacuum_truncate=false, toast.vacuum_truncate=true);
+-- EXTERNAL to force data on TOAST table, uncompressed.
+ALTER TABLE vac_truncate_on_toast_off ALTER COLUMN j SET STORAGE EXTERNAL;
+ALTER TABLE vac_truncate_off_toast_on ALTER COLUMN j SET STORAGE EXTERNAL;
+INSERT INTO vac_truncate_on_toast_off SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_truncate_on_toast_off SELECT generate_series(1000, 1010), repeat('1234567890', 1000);
+INSERT INTO vac_truncate_off_toast_on SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_truncate_off_toast_on SELECT generate_series(1000, 1010), repeat('1234567890', 1000);
+SELECT pg_relation_size('vac_truncate_on_toast_off') > 0 AS has_data;
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_on_toast_off';
+SELECT pg_relation_size('vac_truncate_off_toast_on') > 0 AS has_data;
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_off_toast_on';
+DELETE FROM vac_truncate_on_toast_off;
+DELETE FROM vac_truncate_off_toast_on;
+-- TRUNCATE options are retrieved from their respective relations.
+-- Do an aggressive VACUUM to prevent page-skipping.
+VACUUM (FREEZE, ANALYZE) vac_truncate_on_toast_off, vac_truncate_off_toast_on;
+SELECT pg_relation_size('vac_truncate_on_toast_off') > 0 AS has_data;
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_on_toast_off';
+SELECT pg_relation_size('vac_truncate_off_toast_on') > 0 AS has_data;
+SELECT pg_relation_size(reltoastrelid) > 0 AS has_data FROM pg_class
+  WHERE relname = 'vac_truncate_off_toast_on';
+DROP TABLE vac_truncate_on_toast_off;
+DROP TABLE vac_truncate_off_toast_on;
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 9176dc98b6a9..f0bdf7717e30 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -303,3 +303,50 @@ drop view test_view;
 drop foreign table test_foreign_table;
 drop server dummy_server;
 drop foreign data wrapper dummy;
+-- INDEX_CLEANUP option with VACUUM of more than 1 relation.
+CREATE TABLE vac_index_cleanup_on_toast_off(i int primary key, j text) WITH
+  (autovacuum_enabled=false, vacuum_index_cleanup=true, toast.vacuum_index_cleanup=false);
+CREATE TABLE vac_index_cleanup_off_toast_on(i int primary key, j text)
+  WITH (autovacuum_enabled=false, vacuum_index_cleanup=false, toast.vacuum_index_cleanup=true);
+-- EXTERNAL to force data on TOAST table, uncompressed.
+ALTER TABLE vac_index_cleanup_on_toast_off ALTER COLUMN j SET STORAGE EXTERNAL;
+ALTER TABLE vac_index_cleanup_off_toast_on ALTER COLUMN j SET STORAGE EXTERNAL;
+INSERT INTO vac_index_cleanup_on_toast_off SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_index_cleanup_on_toast_off SELECT generate_series(1001, 1010), repeat('1234567890', 10000);
+INSERT INTO vac_index_cleanup_off_toast_on SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_index_cleanup_off_toast_on SELECT generate_series(1001, 1010), repeat('1234567890', 10000);
+DELETE FROM vac_index_cleanup_on_toast_off;
+DELETE FROM vac_index_cleanup_off_toast_on;
+-- Do an aggressive VACUUM to prevent page-skipping
+VACUUM (FREEZE, ANALYZE) vac_index_cleanup_on_toast_off, vac_index_cleanup_off_toast_on;
+-- Check cleanup state of main table indexes
+SELECT deleted_pages > 0 AS has_del_pages FROM pgstatindex('vac_index_cleanup_off_toast_on_pkey');
+ has_del_pages 
+---------------
+ f
+(1 row)
+
+SELECT deleted_pages > 0 AS has_del_pages FROM pgstatindex('vac_index_cleanup_on_toast_off_pkey');
+ has_del_pages 
+---------------
+ t
+(1 row)
+
+-- Check cleanup state of TOAST indexes
+WITH index_data AS (
+  SELECT indexrelid::regclass AS indname, c.relname AS tabname FROM pg_class AS c, pg_index AS i
+    WHERE c.reltoastrelid = i.indrelid AND
+          c.relname IN ('vac_index_cleanup_off_toast_on',
+                        'vac_index_cleanup_on_toast_off')
+)
+SELECT tabname, deleted_pages > 0 AS has_del_pages
+  FROM index_data, pgstatindex(index_data.indname)
+  ORDER BY tabname COLLATE "C";
+            tabname             | has_del_pages 
+--------------------------------+---------------
+ vac_index_cleanup_off_toast_on | t
+ vac_index_cleanup_on_toast_off | f
+(2 rows)
+
+DROP TABLE vac_index_cleanup_on_toast_off;
+DROP TABLE vac_index_cleanup_off_toast_on;
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 7e72c567a064..9a0d284f9b14 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -136,3 +136,35 @@ drop view test_view;
 drop foreign table test_foreign_table;
 drop server dummy_server;
 drop foreign data wrapper dummy;
+
+-- INDEX_CLEANUP option with VACUUM of more than 1 relation.
+CREATE TABLE vac_index_cleanup_on_toast_off(i int primary key, j text) WITH
+  (autovacuum_enabled=false, vacuum_index_cleanup=true, toast.vacuum_index_cleanup=false);
+CREATE TABLE vac_index_cleanup_off_toast_on(i int primary key, j text)
+  WITH (autovacuum_enabled=false, vacuum_index_cleanup=false, toast.vacuum_index_cleanup=true);
+-- EXTERNAL to force data on TOAST table, uncompressed.
+ALTER TABLE vac_index_cleanup_on_toast_off ALTER COLUMN j SET STORAGE EXTERNAL;
+ALTER TABLE vac_index_cleanup_off_toast_on ALTER COLUMN j SET STORAGE EXTERNAL;
+INSERT INTO vac_index_cleanup_on_toast_off SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_index_cleanup_on_toast_off SELECT generate_series(1001, 1010), repeat('1234567890', 10000);
+INSERT INTO vac_index_cleanup_off_toast_on SELECT generate_series(1, 1000), NULL;
+INSERT INTO vac_index_cleanup_off_toast_on SELECT generate_series(1001, 1010), repeat('1234567890', 10000);
+DELETE FROM vac_index_cleanup_on_toast_off;
+DELETE FROM vac_index_cleanup_off_toast_on;
+-- Do an aggressive VACUUM to prevent page-skipping
+VACUUM (FREEZE, ANALYZE) vac_index_cleanup_on_toast_off, vac_index_cleanup_off_toast_on;
+-- Check cleanup state of main table indexes
+SELECT deleted_pages > 0 AS has_del_pages FROM pgstatindex('vac_index_cleanup_off_toast_on_pkey');
+SELECT deleted_pages > 0 AS has_del_pages FROM pgstatindex('vac_index_cleanup_on_toast_off_pkey');
+-- Check cleanup state of TOAST indexes
+WITH index_data AS (
+  SELECT indexrelid::regclass AS indname, c.relname AS tabname FROM pg_class AS c, pg_index AS i
+    WHERE c.reltoastrelid = i.indrelid AND
+          c.relname IN ('vac_index_cleanup_off_toast_on',
+                        'vac_index_cleanup_on_toast_off')
+)
+SELECT tabname, deleted_pages > 0 AS has_del_pages
+  FROM index_data, pgstatindex(index_data.indname)
+  ORDER BY tabname COLLATE "C";
+DROP TABLE vac_index_cleanup_on_toast_off;
+DROP TABLE vac_index_cleanup_off_toast_on;
-- 
2.50.0

From cd1273aa60f41c02baef9807c9b7d51dd66cbec7 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Tue, 24 Jun 2025 09:43:38 +0900
Subject: [PATCH v4 2/2] Refactor handling of VacuumParams

This adds a couple of const markers, while removing pointer uses of
VacuumParams to force the following policy among all the VACUUM-related
calls: do not touch the values given by the caller.

This patch is for HEAD.

Author: Michael P.
---
 src/include/access/heapam.h          |   4 +-
 src/include/access/tableam.h         |   6 +-
 src/include/commands/vacuum.h        |   6 +-
 src/backend/access/heap/vacuumlazy.c |  44 +++++------
 src/backend/commands/analyze.c       |  26 +++----
 src/backend/commands/cluster.c       |   2 +-
 src/backend/commands/vacuum.c        | 106 ++++++++++++---------------
 src/backend/postmaster/autovacuum.c  |   2 +-
 8 files changed, 92 insertions(+), 104 deletions(-)

 
 
 /*
 
 	/*

 

 	/*

 
 
 	/*

 	/*

 	/*

 

 
 
 	/*
index a43f090ee178..011239588c7c 100644

 
 
 	/*

 	/*
@@ -632,21 +630,13 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
-			if (params->options & VACOPT_VACUUM)
+			if (params.options & VACOPT_VACUUM)
 			{
-				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))
+				if (!vacuum_rel(vrel->oid, vrel->relation, params, bstrategy))
 					continue;
 			}
 
@@ -710,8 +700,8 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
@@ -1109,7 +1099,7 @@ get_all_vacuum_rels(MemoryContext vac_context, int options)
@@ -1125,10 +1115,10 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
 				aggressiveMXIDCutoff;

 	/* Set pg_class fields in cutoffs */
 	cutoffs->relfrozenxid = rel->rd_rel->relfrozenxid;
@@ -2005,7 +1995,7 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
 		   BufferAccessStrategy bstrategy)
 {
 	LOCKMODE	lmode;
@@ -2018,18 +2008,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	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));
+	memcpy(&toast_vacuum_params, &params, sizeof(VacuumParams));

@@ -2055,7 +2043,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
@@ -2079,12 +2067,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,

@@ -2099,8 +2087,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 
@@ -2113,7 +2101,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
@@ -2184,7 +2172,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 
@@ -2195,14 +2183,14 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 				((StdRdOptions *) rel->rd_options)->vacuum_index_cleanup;

@@ -2212,28 +2200,28 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
 
 	/*

 	/*
@@ -2242,9 +2230,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
@@ -2267,16 +2255,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,

 			/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
@@ -2323,7 +2311,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 		toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
 		toast_vacuum_params.toast_parent = relid;

 	/*

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 24 Jun 2025 11:30:13 -0500

On Tue, Jun 24, 2025 at 10:18:18AM +0900, Michael Paquier wrote:
> Knowing that I'm indirectly responsible for this mess, I would like to
> take care of that myself.  Would that be OK for you?

I would be grateful if you took care of it.

> not really necessary for the sake of this thread.

+1 for this.  I did something similar to verify the other bugs I reported,
and this seems far easier to maintain than potentially-unstable tests that
require lots of setup and that depend on secondary effects.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 25 Jun 2025 10:31:35 +0900

> I would be grateful if you took care of it.

Okay, applied the main fix down to v13 then.

> require lots of setup and that depend on secondary effects.

After sleeping on this point, I have finished with this idea for tests
down to v17, adding more coverage for v18 with the GUC vacuum_truncate
and the "auto" mode of index_cleanup, while on it.  v16 and older
branches don't have tests, but I have checked the sanity of the two
code paths using the tests attached for vacuum.sql and
pgstattuple.sql, at least.  Two truncation tests for the "false" cases
were actually unstable.  Sometimes the truncation may not happen even
with an aggressive VACUUM.  Anyway, it is enough to check the true
case for the two code paths patched, even if it may not happen at
100%.  I did not notice a problem with the index_cleanup case as long
as we had enough pages in the TOAST btree index, requiring some
runtime to load the entries, so the cost was annoying.

Attached is the remaining patch for HEAD, planned once v19 opens, and
the tests I have used on the back-branches as a txt to not confuse the
CI, for reference.
--
Michael
From 32b561d88e7480cd0ab9f79443fbacd4f31dc53c Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Wed, 25 Jun 2025 10:22:08 +0900
Subject: [PATCH v5] Refactor handling of VacuumParams

This patch is for HEAD.

Author: Michael P.
---
 src/include/access/heapam.h          |   4 +-
 src/include/access/tableam.h         |   6 +-
 src/include/commands/vacuum.h        |   6 +-
 src/backend/access/heap/vacuumlazy.c |  44 +++++-----
 src/backend/commands/analyze.c       |  26 +++---
 src/backend/commands/cluster.c       |   2 +-
 src/backend/commands/vacuum.c        | 118 ++++++++++++---------------
 src/backend/postmaster/autovacuum.c  |   2 +-
 8 files changed, 98 insertions(+), 110 deletions(-)

 
 
 /*
 
 	/*

 

 	/*

 
 
 	/*

 	/*

 	/*

 

 
 
 	/*
 	 * FreezeXid will become the table's new relfrozenxid, and that mustn't go
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 02993d320daf..733ef40ae7c5 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -124,7 +124,7 @@ static void vac_truncate_clog(TransactionId frozenXID,
 							  MultiXactId minMulti,
 							  TransactionId lastSaneFrozenXid,
 							  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
 					   BufferAccessStrategy bstrategy);
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
@@ -465,7 +465,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	}

@@ -494,7 +494,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
@@ -503,9 +503,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,

 	/*
@@ -515,7 +513,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
@@ -538,7 +536,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
@@ -554,7 +552,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
@@ -562,7 +560,7 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
 	/*
@@ -578,11 +576,11 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
@@ -633,21 +631,13 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,

@@ -711,8 +701,8 @@ vacuum(List *relations, VacuumParams *params, BufferAccessStrategy bstrategy,
 
@@ -1110,7 +1100,7 @@ get_all_vacuum_rels(MemoryContext vac_context, int options)
@@ -1126,10 +1116,10 @@ vacuum_get_cutoffs(Relation rel, const VacuumParams *params,
 				aggressiveMXIDCutoff;

 	/* Set pg_class fields in cutoffs */
 	cutoffs->relfrozenxid = rel->rd_rel->relfrozenxid;
@@ -2006,7 +1996,7 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
+vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
 		   BufferAccessStrategy bstrategy)
 {
 	LOCKMODE	lmode;
@@ -2019,18 +2009,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	int			save_nestlevel;
 	VacuumParams toast_vacuum_params;

 
@@ -2056,7 +2044,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
@@ -2080,12 +2068,12 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,

@@ -2100,8 +2088,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 
@@ -2114,7 +2102,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
@@ -2185,7 +2173,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 
@@ -2196,23 +2184,23 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 				((StdRdOptions *) rel->rd_options)->vacuum_index_cleanup;

 #ifdef USE_INJECTION_POINTS
-	if (params->index_cleanup == VACOPTVALUE_AUTO)
+	if (params.index_cleanup == VACOPTVALUE_AUTO)
 		INJECTION_POINT("vacuum-index-cleanup-auto", NULL);
-	else if (params->index_cleanup == VACOPTVALUE_DISABLED)
+	else if (params.index_cleanup == VACOPTVALUE_DISABLED)
 		INJECTION_POINT("vacuum-index-cleanup-disabled", NULL);
-	else if (params->index_cleanup == VACOPTVALUE_ENABLED)
+	else if (params.index_cleanup == VACOPTVALUE_ENABLED)
 		INJECTION_POINT("vacuum-index-cleanup-enabled", NULL);
 #endif
 
@@ -2222,36 +2210,36 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 	 */
 
 	/*

 #ifdef USE_INJECTION_POINTS
-	if (params->truncate == VACOPTVALUE_AUTO)
+	if (params.truncate == VACOPTVALUE_AUTO)
 		INJECTION_POINT("vacuum-truncate-auto", NULL);
-	else if (params->truncate == VACOPTVALUE_DISABLED)
+	else if (params.truncate == VACOPTVALUE_DISABLED)
 		INJECTION_POINT("vacuum-truncate-disabled", NULL);
-	else if (params->truncate == VACOPTVALUE_ENABLED)
+	else if (params.truncate == VACOPTVALUE_ENABLED)
 		INJECTION_POINT("vacuum-truncate-enabled", NULL);
 #endif
 
@@ -2261,9 +2249,9 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
@@ -2286,16 +2274,16 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,

 			/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
@@ -2342,7 +2330,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 		toast_vacuum_params.options |= VACOPT_PROCESS_MAIN;
 		toast_vacuum_params.toast_parent = relid;

 	/*

index 0abcc99989e0..a4d9c5510cfc 100644
@@ -686,3 +686,61 @@ RESET ROLE;
index a72bdb5b619d..865582f93d17 100644
@@ -495,3 +495,32 @@ RESET ROLE;

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 25 Jun 2025 10:21:03 -0500

> Okay, applied the main fix down to v13 then.

Thanks!

> the tests I have used on the back-branches as a txt to not confuse the

Looks reasonable to me.

-- 
nathan

---

From: shihao zhong <[email-masked]>
Date: Wed, 25 Jun 2025 13:12:02 -0400

> the tests I have used on the back-branches as a txt to not confuse the

Just want to make sure, are we not going to include my original test
to catch the future regression? Also, could someone please let me know
how to check if the test is stable or not?

Thanks

---

From: Michael Paquier <[email-masked]>
Date: Thu, 26 Jun 2025 07:47:22 +0900

> how to check if the test is stable or not?

On stable branches, you could reuse the patch I have posted upthread,
even if it is not included in the tree.  On HEAD and v17, you can run
the tests of src/test/modules/injection_points/ with
--enable-injection-points set.

Making the index cleanup stable takes a certain amount of cycles
because it requires a minimum amount of data, particularly for the
btree of a TOAST index.  The truncation check is halfly stable: it is
possible to check that a truncation does not happen, but it can still
be slightly unstable because the option may not trigger all the time.
So, while the test success rate is perhaps close to 90%, with this
rate going down on slower machines, it is just cheaper and more
reliable to check directly the contents of VacuumParams.  Note that
your original set of tests only covered the case of multiple
relations, and it missed coverage for the TOAST relation vacuumed
after its main relation.
--
Michael

---

From: shihao zhong <[email-masked]>
Date: Fri, 27 Jun 2025 11:26:54 -0400

>> Making the index cleanup stable takes a certain amount of cycles,,

Thanks for your explanation!

---

From: Michael Paquier <[email-masked]>
Date: Mon, 30 Jun 2025 15:49:49 +0900

>> the tests I have used on the back-branches as a txt to not confuse the

Thanks.  Applied on HEAD, then.
--
Michael

---

From: Andres Freund <[email-masked]>
Date: Fri, 20 Mar 2026 12:27:49 -0400

Hi,

> Thanks.  Applied on HEAD, then.

I was just looking at the includes for tableam.h - a quite widely included
header - and got rather sad seeing vacuum.h in there, as it has a quite large
dependency tree and tableam.h is obviously widely included.

I was surprised that I would have added such a wide include when working on
tableam. Turns out I hadn't - it was 2252fcd4276c.

I think this change was quite ill considered. Look at the dependency tree of
vacuum.h:

https://doxygen.postgresql.org/vacuum_8h.html

That's a *lot* of headers to pull into something as widely included as
tableam.h

Why wasn't it enough to add const markers and keep passing by pointer?

Greetings,

Andres

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 20 Mar 2026 14:39:11 -0500

> Why wasn't it enough to add const markers and keep passing by pointer?

IIRC the idea was to prevent similar problems in the future.  To avoid the
extra #includes, we could instead use the back-patched version (e.g.,
commit 661643deda).

-- 
nathan

---

From: Andres Freund <[email-masked]>
Date: Fri, 20 Mar 2026 16:32:48 -0400

Hi,

On 2026-03-20 14:39:11 -0500, Nathan Bossart wrote:
> IIRC the idea was to prevent similar problems in the future.

Seems using const VacuumParams *params should suffice for that? I don't think
it's particularly likely that we'll accept code that casts the const away and
then later get hurt by that.

> To avoid the extra #includes, we could instead use the back-patched version
> (e.g., commit 661643deda).

I'd probably not go quite there, at least the params should largely be const,
with a local on-stack copy where we do need to modify.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 24 Mar 2026 16:47:30 -0500

> with a local on-stack copy where we do need to modify.

Here is a first try.

-- 
nathan

---

From: Andres Freund <[email-masked]>
Date: Tue, 24 Mar 2026 23:29:52 -0400

Hi,

Thanks!

Looks reasonable on a skim.

>  static bool
> -vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
> +vacuum_rel(Oid relid, RangeVar *relation, const VacuumParams *params,
> @@ -2014,18 +2014,21 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams params,
>  	Oid			save_userid;
>  	int			save_sec_context;
>  	int			save_nestlevel;

I'd maybe not name it _copy, but params_local or params_table or such, as
somehow it seems a bit odd to modify the copy. But I can't really explain why
it feels odd.

I wonder if more of the code in the function should be updated to use the
copy, otherwise it seems like it could more easily happen that a part of the
code not using the modified version is moved until after a modification, and
the code author assumes the modifications now have taken effect.

Greetings,

Andres Freund

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 25 Mar 2026 10:33:48 -0500

> somehow it seems a bit odd to modify the copy. But I can't really explain why

params_local WFM.

> the code author assumes the modifications now have taken effect.

Yeah, I was waffling on this.  I updated the rest of the function to use
params_local instead.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Thu, 26 Mar 2026 11:22:25 +0900

> Yeah, I was waffling on this.  I updated the rest of the function to use

Fine by me with these extra forward declarations and the pointer
manipulations across these VACUUM/ANALYZE calls.  That would be
protective enough in terms of the original intention of these const
markers added in 2252fcd4276c.

Once vacuum_rel() has its VacuumParams changed to a pointer, renaming
it to params_local and use params_local in all its code paths looks
sensible.  On top of what you are doing, I would add a big warning
comment at the top of the routine to mention that "params" should not
be used directly in the routine.  The local copies should be used.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 26 Mar 2026 11:07:52 -0500

> be used directly in the routine.  The local copies should be used.

Wait...  Can't we just pass params by value only to vacuum_rel()?  Doesn't
this accomplish what we need?

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Fri, 27 Mar 2026 08:03:30 +0900

> this accomplish what we need?

It would accomplish Andres's goal of avoiding a deeper inclusion of
vacuum.h in src/include/.

Still, I slightly prefer your v2, where the interface of vacuum_rel()
is leaner with all the other ones.  It comes at the cost of copying
the input parameters into a temporary "copy" of VacuumParams, but I
see the fact of marking the input "params" with a const as more
valuable in the long-run, with less temptation to manipulate it
directly especially it is not not marked with a const.  One small
worry with v3 is that people like copy-pasting code around, and I
suspect that v2 could discourage better the patterns that 2252fcd4276c
has tried to improve and that 661643dedad9 had to fix.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 27 Mar 2026 13:58:39 -0500

> has tried to improve and that 661643dedad9 had to fix.

I disagree with you here.  By passing the struct by-value, we are avoiding
scribbles on the original one without an explicit memcpy and without a big
comment warning folks to only use the copy (which seems like it'd be easy
to miss).  I think using a const pointer in most places makes sense, but
not if we need to immediately copy the contents to a local variable anyway.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 31 Mar 2026 09:24:09 -0500

Here is a rebased patch.  Provided cfbot is happy with it, I plan to commit
it later today.  Happy to revisit the vacuum_rel() discussion if needed.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 31 Mar 2026 12:45:55 -0500

Committed.

-- 
nathan