0001-wip-vacuum-refactor.patch

From f46fa37268b2873b4f98118ce8561fd87ef7297a Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 5 Apr 2023 17:20:41 -0400
Subject: [PATCH v1 1/2] wip vacuum refactor

---
 src/backend/commands/vacuum.c       | 243 ++++++++++++++--------------
 src/backend/postmaster/autovacuum.c |  17 +-
 src/include/commands/vacuum.h       |   3 +-
 3 files changed, 139 insertions(+), 124 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index da85330ef4..c6eb1f2fad 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -105,6 +105,7 @@ void
 ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 {
 	VacuumParams params;
+	BufferAccessStrategy bstrategy = NULL;
 	bool		verbose = false;
 	bool		skip_locked = false;
 	bool		analyze = false;
@@ -115,8 +116,13 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	bool		process_toast = true;
 	bool		skip_database_stats = false;
 	bool		only_database_stats = false;
+	MemoryContext vac_context;
 	ListCell   *lc;
 
+	const char *stmttype;
+	volatile bool in_outer_xact,
+				use_own_xacts;
+
 	/* index_cleanup and truncate values unspecified for now */
 	params.index_cleanup = VACOPTVALUE_UNSPECIFIED;
 	params.truncate = VACOPTVALUE_UNSPECIFIED;
@@ -254,6 +260,61 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 		}
 	}
 
+
+	stmttype = (params.options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
+
+	/*
+	 * We cannot run VACUUM inside a user transaction block; if we were inside
+	 * a transaction, then our commit- and start-transaction-command calls
+	 * would not have the intended effect!	There are numerous other subtle
+	 * dependencies on this, too.
+	 *
+	 * ANALYZE (without VACUUM) can run either way.
+	 */
+	if (params.options & VACOPT_VACUUM)
+	{
+		PreventInTransactionBlock(isTopLevel, stmttype);
+		in_outer_xact = false;
+	}
+	else
+		in_outer_xact = IsInTransactionBlock(isTopLevel);
+
+	/*
+	 * Sanity check DISABLE_PAGE_SKIPPING option.
+	 */
+	if ((params.options & VACOPT_FULL) != 0 &&
+		(params.options & VACOPT_DISABLE_PAGE_SKIPPING) != 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL")));
+
+	/* sanity check for PROCESS_TOAST */
+	if ((params.options & VACOPT_FULL) != 0 &&
+		(params.options & VACOPT_PROCESS_TOAST) == 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("PROCESS_TOAST required with VACUUM FULL")));
+
+	/* sanity check for ONLY_DATABASE_STATS */
+	if (params.options & VACOPT_ONLY_DATABASE_STATS)
+	{
+		Assert(params.options & VACOPT_VACUUM);
+		if (vacstmt->rels != NIL)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("ONLY_DATABASE_STATS cannot be specified with a list of tables")));
+		/* don't require people to turn off PROCESS_TOAST/MAIN explicitly */
+		if (params.options & ~(VACOPT_VACUUM |
+								VACOPT_VERBOSE |
+								VACOPT_PROCESS_MAIN |
+								VACOPT_PROCESS_TOAST |
+								VACOPT_ONLY_DATABASE_STATS))
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("ONLY_DATABASE_STATS cannot be specified with other VACUUM options")));
+	}
+
+
 	/*
 	 * All freeze ages are zero if the FREEZE option is given; otherwise pass
 	 * them as -1 which means to use the default values.
@@ -279,8 +340,67 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	/* user-invoked vacuum uses VACOPT_VERBOSE instead of log_min_duration */
 	params.log_min_duration = -1;
 
+	/*
+	 * Create special memory context for cross-transaction storage.
+	 *
+	 * Since it is a child of PortalContext, it will go away eventually even
+	 * if we suffer an error; there's no need for special abort cleanup logic.
+	 */
+	vac_context = AllocSetContextCreate(PortalContext,
+										"Vacuum",
+										ALLOCSET_DEFAULT_SIZES);
+
+	/*
+	 * We needn't bother making a Buffer Access Strategy for VACUUM (FULL)
+	 * (unless ANALYZE is also specified) or VACUUM (ONLY_DATABASE_STATS) as
+	 * they'll not make use of it.
+	 */
+	if ((params.options & VACOPT_ONLY_DATABASE_STATS) == 0 ||
+		(params.options & VACOPT_FULL && (params.options & VACOPT_ANALYZE) == 0))
+	{
+		MemoryContext old_context = MemoryContextSwitchTo(vac_context);
+
+		bstrategy = GetAccessStrategy(BAS_VACUUM);
+		MemoryContextSwitchTo(old_context);
+	}
+
+	/*
+	 * Decide whether we need to start/commit our own transactions.
+	 *
+	 * For VACUUM (with or without ANALYZE): always do so, so that we can
+	 * release locks as soon as possible.  (We could possibly use the outer
+	 * transaction for a one-table VACUUM, but handling TOAST tables would be
+	 * problematic.)
+	 *
+	 * For ANALYZE (no VACUUM): if inside a transaction block, we cannot
+	 * start/commit our own transactions.  Also, there's no need to do so if
+	 * only processing one relation.  For multiple relations when not within a
+	 * transaction block, and also in an autovacuum worker, use own
+	 * transactions so we can release locks sooner.
+	 */
+	if (params.options & VACOPT_VACUUM)
+		use_own_xacts = true;
+	else
+	{
+		Assert(params.options & VACOPT_ANALYZE);
+		if (in_outer_xact)
+			use_own_xacts = false;
+		else if (list_length(vacstmt->rels) > 1)
+			use_own_xacts = true;
+		else
+			use_own_xacts = false;
+	}
+
 	/* Now go through the common routine */
-	vacuum(vacstmt->rels, &params, NULL, isTopLevel);
+	vacuum(vacstmt->rels, &params, bstrategy, vac_context, isTopLevel,
+			use_own_xacts, in_outer_xact);
+
+	/*
+	 * Clean up working storage --- note we must do this after
+	 * StartTransactionCommand, else we might be trying to delete the active
+	 * context!
+	 */
+	MemoryContextDelete(vac_context);
 }
 
 /*
@@ -304,35 +424,16 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
  */
 void
 vacuum(List *relations, VacuumParams *params,
-	   BufferAccessStrategy bstrategy, bool isTopLevel)
+	   BufferAccessStrategy bstrategy, MemoryContext vac_context,
+	   bool isTopLevel, bool use_own_xacts, bool in_outer_xact)
 {
-	static bool in_vacuum = false;
 
-	MemoryContext vac_context;
 	const char *stmttype;
-	volatile bool in_outer_xact,
-				use_own_xacts;
-
+	static bool in_vacuum = false;
 	Assert(params != NULL);
 
 	stmttype = (params->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
-	/*
-	 * We cannot run VACUUM inside a user transaction block; if we were inside
-	 * a transaction, then our commit- and start-transaction-command calls
-	 * would not have the intended effect!	There are numerous other subtle
-	 * dependencies on this, too.
-	 *
-	 * ANALYZE (without VACUUM) can run either way.
-	 */
-	if (params->options & VACOPT_VACUUM)
-	{
-		PreventInTransactionBlock(isTopLevel, stmttype);
-		in_outer_xact = false;
-	}
-	else
-		in_outer_xact = IsInTransactionBlock(isTopLevel);
-
 	/*
 	 * Check for and disallow recursive calls.  This could happen when VACUUM
 	 * FULL or ANALYZE calls a hostile index expression that itself calls
@@ -344,67 +445,6 @@ vacuum(List *relations, VacuumParams *params,
 				 errmsg("%s cannot be executed from VACUUM or ANALYZE",
 						stmttype)));
 
-	/*
-	 * Sanity check DISABLE_PAGE_SKIPPING option.
-	 */
-	if ((params->options & VACOPT_FULL) != 0 &&
-		(params->options & VACOPT_DISABLE_PAGE_SKIPPING) != 0)
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("VACUUM option DISABLE_PAGE_SKIPPING cannot be used with FULL")));
-
-	/* sanity check for PROCESS_TOAST */
-	if ((params->options & VACOPT_FULL) != 0 &&
-		(params->options & VACOPT_PROCESS_TOAST) == 0)
-		ereport(ERROR,
-				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-				 errmsg("PROCESS_TOAST required with VACUUM FULL")));
-
-	/* sanity check for ONLY_DATABASE_STATS */
-	if (params->options & VACOPT_ONLY_DATABASE_STATS)
-	{
-		Assert(params->options & VACOPT_VACUUM);
-		if (relations != NIL)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("ONLY_DATABASE_STATS cannot be specified with a list of tables")));
-		/* don't require people to turn off PROCESS_TOAST/MAIN explicitly */
-		if (params->options & ~(VACOPT_VACUUM |
-								VACOPT_VERBOSE |
-								VACOPT_PROCESS_MAIN |
-								VACOPT_PROCESS_TOAST |
-								VACOPT_ONLY_DATABASE_STATS))
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("ONLY_DATABASE_STATS cannot be specified with other VACUUM options")));
-	}
-
-	/*
-	 * Create special memory context for cross-transaction storage.
-	 *
-	 * Since it is a child of PortalContext, it will go away eventually even
-	 * if we suffer an error; there's no need for special abort cleanup logic.
-	 */
-	vac_context = AllocSetContextCreate(PortalContext,
-										"Vacuum",
-										ALLOCSET_DEFAULT_SIZES);
-
-	/*
-	 * If caller didn't give us a buffer strategy object, make one in the
-	 * cross-transaction memory context.  We needn't bother making this for
-	 * VACUUM (FULL) or VACUUM (ONLY_DATABASE_STATS) as they'll not make use
-	 * of it.
-	 */
-	if (bstrategy == NULL &&
-		(params->options & (VACOPT_ONLY_DATABASE_STATS |
-							VACOPT_FULL)) == 0)
-	{
-		MemoryContext old_context = MemoryContextSwitchTo(vac_context);
-
-		bstrategy = GetAccessStrategy(BAS_VACUUM);
-		MemoryContextSwitchTo(old_context);
-	}
-
 	/*
 	 * Build list of relation(s) to process, putting any new data in
 	 * vac_context for safekeeping.
@@ -435,35 +475,6 @@ vacuum(List *relations, VacuumParams *params,
 	else
 		relations = get_all_vacuum_rels(vac_context, params->options);
 
-	/*
-	 * Decide whether we need to start/commit our own transactions.
-	 *
-	 * For VACUUM (with or without ANALYZE): always do so, so that we can
-	 * release locks as soon as possible.  (We could possibly use the outer
-	 * transaction for a one-table VACUUM, but handling TOAST tables would be
-	 * problematic.)
-	 *
-	 * For ANALYZE (no VACUUM): if inside a transaction block, we cannot
-	 * start/commit our own transactions.  Also, there's no need to do so if
-	 * only processing one relation.  For multiple relations when not within a
-	 * transaction block, and also in an autovacuum worker, use own
-	 * transactions so we can release locks sooner.
-	 */
-	if (params->options & VACOPT_VACUUM)
-		use_own_xacts = true;
-	else
-	{
-		Assert(params->options & VACOPT_ANALYZE);
-		if (IsAutoVacuumWorkerProcess())
-			use_own_xacts = true;
-		else if (in_outer_xact)
-			use_own_xacts = false;
-		else if (list_length(relations) > 1)
-			use_own_xacts = true;
-		else
-			use_own_xacts = false;
-	}
-
 	/*
 	 * vacuum_rel expects to be entered with no transaction active; it will
 	 * start and commit its own transaction.  But we are called by an SQL
@@ -576,12 +587,6 @@ vacuum(List *relations, VacuumParams *params,
 		vac_update_datfrozenxid();
 	}
 
-	/*
-	 * Clean up working storage --- note we must do this after
-	 * StartTransactionCommand, else we might be trying to delete the active
-	 * context!
-	 */
-	MemoryContextDelete(vac_context);
 }
 
 /*
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 585d28148c..8016a2dd47 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -338,7 +338,8 @@ static void relation_needs_vacanalyze(Oid relid, AutoVacOpts *relopts,
 									  bool *dovacuum, bool *doanalyze, bool *wraparound);
 
 static void autovacuum_do_vac_analyze(autovac_table *tab,
-									  BufferAccessStrategy bstrategy);
+									  BufferAccessStrategy bstrategy,
+									  MemoryContext portal_context);
 static AutoVacOpts *extract_autovac_opts(HeapTuple tup,
 										 TupleDesc pg_class_desc);
 static void perform_work_item(AutoVacuumWorkItem *workitem);
@@ -2470,7 +2471,7 @@ do_autovacuum(void)
 			MemoryContextSwitchTo(PortalContext);
 
 			/* have at it */
-			autovacuum_do_vac_analyze(tab, bstrategy);
+			autovacuum_do_vac_analyze(tab, bstrategy, PortalContext);
 
 			/*
 			 * Clear a possible query-cancel signal, to avoid a late reaction
@@ -3143,11 +3144,13 @@ relation_needs_vacanalyze(Oid relid,
  *		Vacuum and/or analyze the specified table
  */
 static void
-autovacuum_do_vac_analyze(autovac_table *tab, BufferAccessStrategy bstrategy)
+autovacuum_do_vac_analyze(autovac_table *tab, BufferAccessStrategy bstrategy,
+						  MemoryContext portal_context)
 {
 	RangeVar   *rangevar;
 	VacuumRelation *rel;
 	List	   *rel_list;
+	MemoryContext	vac_context;
 
 	/* Let pgstat know what we're doing */
 	autovac_report_activity(tab);
@@ -3157,7 +3160,13 @@ autovacuum_do_vac_analyze(autovac_table *tab, BufferAccessStrategy bstrategy)
 	rel = makeVacuumRelation(rangevar, tab->at_relid, NIL);
 	rel_list = list_make1(rel);
 
-	vacuum(rel_list, &tab->at_params, bstrategy, true);
+	vac_context = AllocSetContextCreate(portal_context,
+										"Vacuum",
+										ALLOCSET_DEFAULT_SIZES);
+
+	vacuum(rel_list, &tab->at_params, bstrategy, vac_context, true, true, false);
+
+	MemoryContextDelete(vac_context);
 }
 
 /*
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index bdfd96cfec..a4c928a877 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -310,7 +310,8 @@ extern PGDLLIMPORT int VacuumCostBalanceLocal;
 /* in commands/vacuum.c */
 extern void ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel);
 extern void vacuum(List *relations, VacuumParams *params,
-				   BufferAccessStrategy bstrategy, bool isTopLevel);
+				   BufferAccessStrategy bstrategy, MemoryContext vac_context,
+				   bool isTopLevel, bool use_own_xacts, bool in_outer_xact);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
 							 int *nindexes, Relation **Irel);
 extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode);
-- 
2.37.2