Allow single table VACUUM in transaction block

Started by Simon Riggsabout 3 years ago32 messages
#1Simon Riggs
simon.riggs@enterprisedb.com
1 attachment(s)

It is a common user annoyance to have a script fail because someone
added a VACUUM, especially when using --single-transaction option.
Fix, so that this works without issue:

BEGIN;
....
VACUUM (ANALYZE) vactst;
....
COMMIT;

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

When in a xact block, we do not set PROC_IN_VACUUM,
nor update datfrozenxid.

Tests, docs.

--
Simon Riggs http://www.EnterpriseDB.com/

Attachments:

single_table_vacuum.v1.patchapplication/octet-stream; name=single_table_vacuum.v1.patchDownload
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index c582021d29..6f7860316c 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -366,7 +366,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
    </para>
 
    <para>
-    <command>VACUUM</command> cannot be executed inside a transaction block.
+    <command>VACUUM</command> cannot be executed inside a transaction block,
+    unless a single table is specified and <literal>FULL</literal> is not
+    specified.  When executing inside a transaction block the vacuum scan can
+    hold back the xmin horizon and does not update the database datfrozenxid,
+    as a result this usage is not useful for database maintenance, but is provided
+    to allow vacuuming in special circumstances, such as temporary or private
+    work tables.
    </para>
 
    <para>
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7ccde07de9..95fa1a97ca 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -297,6 +297,7 @@ vacuum(List *relations, VacuumParams *params,
 	   BufferAccessStrategy bstrategy, bool isTopLevel)
 {
 	static bool in_vacuum = false;
+	bool single_table = false;
 
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -306,15 +307,20 @@ vacuum(List *relations, VacuumParams *params,
 
 	stmttype = (params->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
+	if (relations != NIL && list_length(relations) == 1)
+		single_table = true;
+
 	/*
-	 * We cannot run VACUUM inside a user transaction block; if we were inside
-	 * a transaction, then our commit- and start-transaction-command calls
+	 * Single-table VACUUM can run inside a user transaction block, but
+	 * we cannot run multiple VACUUMs 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.
+	 * dependencies on this, too, so when running in a transaction block, vacuum
+	 * will skip some of its normal actions, see later for details.
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (params->options & VACOPT_VACUUM)
+	if (params->options & VACOPT_VACUUM && !single_table)
 	{
 		PreventInTransactionBlock(isTopLevel, stmttype);
 		in_outer_xact = false;
@@ -401,9 +407,8 @@ vacuum(List *relations, VacuumParams *params,
 	 * 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.)
+	 * release locks as soon as possible, except for a single table VACUUM
+	 * when it is executed inside a transaction block.
 	 *
 	 * 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
@@ -412,7 +417,7 @@ vacuum(List *relations, VacuumParams *params,
 	 * transactions so we can release locks sooner.
 	 */
 	if (params->options & VACOPT_VACUUM)
-		use_own_xacts = true;
+		use_own_xacts = !in_outer_xact;
 	else
 	{
 		Assert(params->options & VACOPT_ANALYZE);
@@ -427,6 +432,7 @@ vacuum(List *relations, VacuumParams *params,
 	}
 
 	/*
+	 * Tell vacuum_rel whether it will need to manage its own transaction. If so,
 	 * 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
 	 * command, and so we are executing inside a transaction already. We
@@ -437,6 +443,9 @@ vacuum(List *relations, VacuumParams *params,
 	if (use_own_xacts)
 	{
 		Assert(!in_outer_xact);
+		Assert(!IsInTransactionBlock(isTopLevel));
+
+		params->use_own_xact = true;
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -445,6 +454,8 @@ vacuum(List *relations, VacuumParams *params,
 		/* matches the StartTransaction in PostgresMain() */
 		CommitTransactionCommand();
 	}
+	else
+		params->use_own_xact = false;
 
 	/* Turn vacuum cost accounting on or off, and set/clear in_vacuum */
 	PG_TRY();
@@ -528,11 +539,13 @@ vacuum(List *relations, VacuumParams *params,
 		StartTransactionCommand();
 	}
 
-	if ((params->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+	if ((params->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess() && use_own_xacts)
 	{
 		/*
 		 * Update pg_database.datfrozenxid, and truncate pg_xact if possible.
-		 * (autovacuum.c does this for itself.)
+		 * (autovacuum.c does this for itself.) unless we are in a transaction
+		 * block, since this might take a while and we're not sure whether it
+		 * is safe to allow this.
 		 */
 		vac_update_datfrozenxid();
 	}
@@ -1837,9 +1850,10 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	Assert(params != NULL);
 
 	/* Begin a transaction for vacuuming this relation */
-	StartTransactionCommand();
+	if (params->use_own_xact)
+		StartTransactionCommand();
 
-	if (!(params->options & VACOPT_FULL))
+	if (!(params->options & VACOPT_FULL) && (params->use_own_xact))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1852,6 +1866,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 		 * contents of other tables is arguably broken, but we won't break it
 		 * here by violating transaction semantics.)
 		 *
+		 * Don't set PROC_IN_VACUUM if running in a transaction block, since it
+		 * would be very bad for other users to ignore our xact in that case.
+		 * Note that setting the flag is an optional performance tweak, not
+		 * required for correct operation of VACUUM.
+		 *
 		 * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by
 		 * autovacuum; it's used to avoid canceling a vacuum that was invoked
 		 * in an emergency.
@@ -1876,7 +1895,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * cutoff xids in local memory wrapping around, and to have updated xmin
 	 * horizons.
 	 */
-	PushActiveSnapshot(GetTransactionSnapshot());
+	if (params->use_own_xact)
+		PushActiveSnapshot(GetTransactionSnapshot());
 
 	/*
 	 * Check for user-requested abort.  Note we want this to be inside a
@@ -1899,8 +1919,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/* leave if relation could not be opened or locked */
 	if (!rel)
 	{
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1917,8 +1940,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 								  params->options & VACOPT_VACUUM))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1934,8 +1960,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 				(errmsg("skipping \"%s\" --- cannot vacuum non-tables or special system tables",
 						RelationGetRelationName(rel))));
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1949,8 +1978,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (RELATION_IS_OTHER_TEMP(rel))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1962,8 +1994,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		/* It's OK to proceed with ANALYZE on this table */
 		return true;
 	}
@@ -1977,9 +2012,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * NOTE: this cannot block, even if someone else is waiting for access,
 	 * because the lock manager knows that both lock requests are from the
 	 * same process.
+	 *
+	 * If we are in a transaction block, vacuum both main table and toast
+	 * table within the existing transaction, so no session lock required.
 	 */
 	lockrelid = rel->rd_lockInfo.lockRelId;
-	LockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		LockRelationIdForSession(&lockrelid, lmode);
 
 	/*
 	 * Set index_cleanup option based on index_cleanup reloption if it wasn't
@@ -2075,8 +2114,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Complete the transaction and free all temporary memory used.
 	 */
-	PopActiveSnapshot();
-	CommitTransactionCommand();
+	if (params->use_own_xact)
+	{
+		PopActiveSnapshot();
+		CommitTransactionCommand();
+	}
 
 	/*
 	 * If the relation has a secondary toast rel, vacuum that too while we
@@ -2091,7 +2133,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Now release the session-level lock on the main table.
 	 */
-	UnlockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		UnlockRelationIdForSession(&lockrelid, lmode);
 
 	/* Report that we really did it. */
 	return true;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5d816ba7f4..35310c588b 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,6 +227,8 @@ typedef struct VacuumParams
 	VacOptValue index_cleanup;	/* Do index vacuum and cleanup */
 	VacOptValue truncate;		/* Truncate empty pages at the end */
 
+	bool		use_own_xact;	/* Use own xact in vacuum_rel? */
+
 	/*
 	 * The number of parallel vacuum workers.  0 by default which means choose
 	 * based on the number of indexes.  -1 indicates parallel vacuum is
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c63a157e5f..94cfe7755b 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -282,6 +282,16 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 ERROR:  PROCESS_TOAST required with VACUUM FULL
+-- Single table inside transaction block
+BEGIN;
+VACUUM (PROCESS_TOAST FALSE) vactst;
+COMMIT;
+BEGIN;
+VACUUM vactst;
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+COMMIT;
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 9faa8a34a6..05dea783df 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -237,6 +237,17 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 
+-- Single table inside transaction block
+BEGIN;
+VACUUM (PROCESS_TOAST FALSE) vactst;
+COMMIT;
+BEGIN;
+VACUUM vactst;
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+COMMIT;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
#2Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Simon Riggs (#1)
Re: Allow single table VACUUM in transaction block

On Thu, 27 Oct 2022 at 10:31, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Tests, docs.

The patch tester says that a pg_upgrade test is failing on Windows,
but works for me.

t/002_pg_upgrade.pl .. ok

Anybody shed any light on that, much appreciated.

--
Simon Riggs http://www.EnterpriseDB.com/

#3Bharath Rupireddy
bharath.rupireddyforpostgres@gmail.com
In reply to: Simon Riggs (#2)
Re: Allow single table VACUUM in transaction block

On Thu, Oct 27, 2022 at 9:49 PM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

On Thu, 27 Oct 2022 at 10:31, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Tests, docs.

The patch tester says that a pg_upgrade test is failing on Windows,
but works for me.

t/002_pg_upgrade.pl .. ok

Anybody shed any light on that, much appreciated.

Please see a recent thread on pg_upgrade failure -
/messages/by-id/Y04mN0ZLNzJywrad@paquier.xyz.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

#4Justin Pryzby
pryzby@telsasoft.com
In reply to: Simon Riggs (#1)
Re: Allow single table VACUUM in transaction block

On Thu, Oct 27, 2022 at 10:31:31AM +0100, Simon Riggs wrote:

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

Maybe I misunderstood what you meant: you said "not VACUUM FULL", but
with your patch, that works:

postgres=# begin; VACUUM FULL pg_class; commit;
BEGIN
VACUUM
COMMIT

Actually, I've thought before that it was bit weird that CLUSTER can be
run within a transaction, but VACUUM FULL cannot (even though it does a
CLUSTER behind the scenes). VACUUM FULL can process multiple relations,
whereas CLUSTER can't, but it seems nice to allow vacuum full for the
case of a single relation.

I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL
within a user txn.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

--
Justin

#5Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Justin Pryzby (#4)
Re: Allow single table VACUUM in transaction block

On Thu, 27 Oct 2022 at 21:07, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Thu, Oct 27, 2022 at 10:31:31AM +0100, Simon Riggs wrote:

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

Maybe I misunderstood what you meant: you said "not VACUUM FULL", but
with your patch, that works:

postgres=# begin; VACUUM FULL pg_class; commit;
BEGIN
VACUUM
COMMIT

Actually, I've thought before that it was bit weird that CLUSTER can be
run within a transaction, but VACUUM FULL cannot (even though it does a
CLUSTER behind the scenes). VACUUM FULL can process multiple relations,
whereas CLUSTER can't, but it seems nice to allow vacuum full for the
case of a single relation.

I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL
within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

--
Simon Riggs http://www.EnterpriseDB.com/

#6Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Simon Riggs (#5)
1 attachment(s)
Re: Allow single table VACUUM in transaction block

On Tue, 1 Nov 2022 at 23:56, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

I haven't checked the rest of the patch, but +1 for allowing VACUUM FULL
within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

New version attached, as described.

Other review comments and alternate opinions welcome.

--
Simon Riggs http://www.EnterpriseDB.com/

Attachments:

single_table_vacuum.v2.patchapplication/octet-stream; name=single_table_vacuum.v2.patchDownload
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index c582021d29..d2cb40a80e 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -366,7 +366,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
    </para>
 
    <para>
-    <command>VACUUM</command> cannot be executed inside a transaction block.
+    <command>VACUUM</command> cannot be executed inside a transaction block,
+    unless a single table is specified and <literal>FULL</literal> is not
+    specified.  When executing inside a transaction block the vacuum scan will
+    hold back the xmin horizon and does not update the database datfrozenxid,
+    as a result this usage is not useful for database maintenance, but is provided
+    to allow vacuuming in special circumstances, such as temporary or private
+    work tables.
    </para>
 
    <para>
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7ccde07de9..ad7c4480e6 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -85,6 +85,7 @@ pg_atomic_uint32 *VacuumActiveNWorkers = NULL;
 int			VacuumCostBalanceLocal = 0;
 
 /* non-export function prototypes */
+static void vacuum_xact_block_callback(void *arg);
 static List *expand_vacuum_rel(VacuumRelation *vrel, int options);
 static List *get_all_vacuum_rels(int options);
 static void vac_truncate_clog(TransactionId frozenXID,
@@ -273,6 +274,20 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	vacuum(vacstmt->rels, &params, NULL, isTopLevel);
 }
 
+/*
+ * Error context callback while checking PreventInTransactionBlock().
+ */
+static void
+vacuum_xact_block_callback(void *arg)
+{
+	bool *multi = (bool *) arg;
+
+	if (*multi)
+		errcontext("while attempting VACUUM of multiple relations");
+	else
+		errcontext("while attempting VACUUM FULL");
+}
+
 /*
  * Internal entry point for VACUUM and ANALYZE commands.
  *
@@ -297,6 +312,7 @@ vacuum(List *relations, VacuumParams *params,
 	   BufferAccessStrategy bstrategy, bool isTopLevel)
 {
 	static bool in_vacuum = false;
+	bool single_table = false;
 
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -306,17 +322,39 @@ vacuum(List *relations, VacuumParams *params,
 
 	stmttype = (params->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
+	if (relations != NIL && list_length(relations) == 1)
+		single_table = true;
+
 	/*
-	 * We cannot run VACUUM inside a user transaction block; if we were inside
-	 * a transaction, then our commit- and start-transaction-command calls
+	 * Single-table VACUUM can run inside a user transaction block, but
+	 * we cannot run multiple VACUUMs 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.
+	 * dependencies on this, too, so when running in a transaction block, vacuum
+	 * will skip some of its normal actions, see later for details.
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (params->options & VACOPT_VACUUM)
+	if (params->options & VACOPT_VACUUM &&
+		(!single_table ||
+		(params->options & VACOPT_FULL) != 0))
 	{
+		ErrorContextCallback errcallback;
+		bool	multi = !single_table;
+
+		/*
+		 * Setup errcontext to explain reason for disallowing vacuum, if any
+		 */
+		errcallback.callback = vacuum_xact_block_callback;
+		errcallback.previous = error_context_stack;
+		error_context_stack = &errcallback;
+		errcallback.arg = &multi;
+
 		PreventInTransactionBlock(isTopLevel, stmttype);
+
+		/* Pop error context stack back to how it was */
+		error_context_stack = errcallback.previous;
+
 		in_outer_xact = false;
 	}
 	else
@@ -401,9 +439,8 @@ vacuum(List *relations, VacuumParams *params,
 	 * 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.)
+	 * release locks as soon as possible, except for a single table VACUUM
+	 * when it is executed inside a transaction block.
 	 *
 	 * 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
@@ -412,7 +449,7 @@ vacuum(List *relations, VacuumParams *params,
 	 * transactions so we can release locks sooner.
 	 */
 	if (params->options & VACOPT_VACUUM)
-		use_own_xacts = true;
+		use_own_xacts = !in_outer_xact;
 	else
 	{
 		Assert(params->options & VACOPT_ANALYZE);
@@ -427,6 +464,7 @@ vacuum(List *relations, VacuumParams *params,
 	}
 
 	/*
+	 * Tell vacuum_rel whether it will need to manage its own transaction. If so,
 	 * 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
 	 * command, and so we are executing inside a transaction already. We
@@ -437,6 +475,9 @@ vacuum(List *relations, VacuumParams *params,
 	if (use_own_xacts)
 	{
 		Assert(!in_outer_xact);
+		Assert(!IsInTransactionBlock(isTopLevel));
+
+		params->use_own_xact = true;
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -445,6 +486,8 @@ vacuum(List *relations, VacuumParams *params,
 		/* matches the StartTransaction in PostgresMain() */
 		CommitTransactionCommand();
 	}
+	else
+		params->use_own_xact = false;
 
 	/* Turn vacuum cost accounting on or off, and set/clear in_vacuum */
 	PG_TRY();
@@ -528,11 +571,13 @@ vacuum(List *relations, VacuumParams *params,
 		StartTransactionCommand();
 	}
 
-	if ((params->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+	if ((params->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess() && use_own_xacts)
 	{
 		/*
 		 * Update pg_database.datfrozenxid, and truncate pg_xact if possible.
-		 * (autovacuum.c does this for itself.)
+		 * (autovacuum.c does this for itself.) unless we are in a transaction
+		 * block, since this might take a while and we're not sure whether it
+		 * is safe to allow this.
 		 */
 		vac_update_datfrozenxid();
 	}
@@ -1837,9 +1882,10 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	Assert(params != NULL);
 
 	/* Begin a transaction for vacuuming this relation */
-	StartTransactionCommand();
+	if (params->use_own_xact)
+		StartTransactionCommand();
 
-	if (!(params->options & VACOPT_FULL))
+	if (!(params->options & VACOPT_FULL) && (params->use_own_xact))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1852,6 +1898,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 		 * contents of other tables is arguably broken, but we won't break it
 		 * here by violating transaction semantics.)
 		 *
+		 * Don't set PROC_IN_VACUUM if running in a transaction block, since it
+		 * would be very bad for other users to ignore our xact in that case.
+		 * Note that setting the flag is an optional performance tweak, not
+		 * required for correct operation of VACUUM.
+		 *
 		 * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by
 		 * autovacuum; it's used to avoid canceling a vacuum that was invoked
 		 * in an emergency.
@@ -1876,7 +1927,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * cutoff xids in local memory wrapping around, and to have updated xmin
 	 * horizons.
 	 */
-	PushActiveSnapshot(GetTransactionSnapshot());
+	if (params->use_own_xact)
+		PushActiveSnapshot(GetTransactionSnapshot());
 
 	/*
 	 * Check for user-requested abort.  Note we want this to be inside a
@@ -1899,8 +1951,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/* leave if relation could not be opened or locked */
 	if (!rel)
 	{
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1917,8 +1972,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 								  params->options & VACOPT_VACUUM))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1934,8 +1992,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 				(errmsg("skipping \"%s\" --- cannot vacuum non-tables or special system tables",
 						RelationGetRelationName(rel))));
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1949,8 +2010,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (RELATION_IS_OTHER_TEMP(rel))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1962,8 +2026,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		/* It's OK to proceed with ANALYZE on this table */
 		return true;
 	}
@@ -1977,9 +2044,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * NOTE: this cannot block, even if someone else is waiting for access,
 	 * because the lock manager knows that both lock requests are from the
 	 * same process.
+	 *
+	 * If we are in a transaction block, vacuum both main table and toast
+	 * table within the existing transaction, so no session lock required.
 	 */
 	lockrelid = rel->rd_lockInfo.lockRelId;
-	LockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		LockRelationIdForSession(&lockrelid, lmode);
 
 	/*
 	 * Set index_cleanup option based on index_cleanup reloption if it wasn't
@@ -2075,8 +2146,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Complete the transaction and free all temporary memory used.
 	 */
-	PopActiveSnapshot();
-	CommitTransactionCommand();
+	if (params->use_own_xact)
+	{
+		PopActiveSnapshot();
+		CommitTransactionCommand();
+	}
 
 	/*
 	 * If the relation has a secondary toast rel, vacuum that too while we
@@ -2091,7 +2165,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Now release the session-level lock on the main table.
 	 */
-	UnlockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		UnlockRelationIdForSession(&lockrelid, lmode);
 
 	/* Report that we really did it. */
 	return true;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5d816ba7f4..35310c588b 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,6 +227,8 @@ typedef struct VacuumParams
 	VacOptValue index_cleanup;	/* Do index vacuum and cleanup */
 	VacOptValue truncate;		/* Truncate empty pages at the end */
 
+	bool		use_own_xact;	/* Use own xact in vacuum_rel? */
+
 	/*
 	 * The number of parallel vacuum workers.  0 by default which means choose
 	 * based on the number of indexes.  -1 indicates parallel vacuum is
diff --git a/src/test/regress/expected/transactions.out b/src/test/regress/expected/transactions.out
index 2b2cff7d91..88d5043cc6 100644
--- a/src/test/regress/expected/transactions.out
+++ b/src/test/regress/expected/transactions.out
@@ -1033,6 +1033,7 @@ SELECT 1\; VACUUM;
 (1 row)
 
 ERROR:  VACUUM cannot run inside a transaction block
+CONTEXT:  while attempting VACUUM of multiple relations
 SELECT 1\; COMMIT\; VACUUM;
 WARNING:  there is no transaction in progress
  ?column? 
@@ -1041,6 +1042,7 @@ WARNING:  there is no transaction in progress
 (1 row)
 
 ERROR:  VACUUM cannot run inside a transaction block
+CONTEXT:  while attempting VACUUM of multiple relations
 -- we disallow savepoint-related commands in implicit-transaction state
 SELECT 1\; SAVEPOINT sp;
  ?column? 
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c63a157e5f..f252620d93 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -282,6 +282,19 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 ERROR:  PROCESS_TOAST required with VACUUM FULL
+-- Single table inside transaction block
+BEGIN;
+VACUUM (PROCESS_TOAST FALSE) vactst;
+COMMIT;
+BEGIN;
+VACUUM vactst;
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+COMMIT;
+BEGIN; VACUUM FULL vactst; COMMIT;
+ERROR:  VACUUM cannot run inside a transaction block
+CONTEXT:  while attempting VACUUM FULL
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 9faa8a34a6..b9f6c0182d 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -237,6 +237,18 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 
+-- Single table inside transaction block
+BEGIN;
+VACUUM (PROCESS_TOAST FALSE) vactst;
+COMMIT;
+BEGIN;
+VACUUM vactst;
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+COMMIT;
+BEGIN; VACUUM FULL vactst; COMMIT;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
#7Rahila Syed
rahilasyed90@gmail.com
In reply to: Simon Riggs (#6)
Re: Allow single table VACUUM in transaction block

Hi Simon,

On Thu, Nov 3, 2022 at 3:53 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

On Tue, 1 Nov 2022 at 23:56, Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

I haven't checked the rest of the patch, but +1 for allowing VACUUM

FULL

within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

New version attached, as described.

Other review comments and alternate opinions welcome.

I applied and did some basic testing on the patch, it works as described.

I would like to bring up a few points that I came across while looking into
the vacuum code.

1. As a result of this change to allow VACUUM inside a user transaction, I
think there is some chance of causing
a block/delay of concurrent VACUUMs if a VACUUM is being run under a long
running transaction.
2. Also, if a user runs VACUUM in a transaction, performance optimizations
like PROC_IN_VACUUM won't work.
3. Also, if VACUUM happens towards the end of a long running transaction,
the snapshot will be old
and xmin horizon for vacuum would be somewhat old as compared to current
lazy vacuum which
acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there
should be some mention
of above caveats in documentation with the recommendation that VACUUM
should be run outside
a transaction, in general.

Thank you,
Rahila Syed

#8Rahila Syed
rahilasyed90@gmail.com
In reply to: Rahila Syed (#7)
Re: Allow single table VACUUM in transaction block

Hi Simon,

On Fri, Nov 4, 2022 at 10:15 AM Rahila Syed <rahilasyed90@gmail.com> wrote:

Hi Simon,

On Thu, Nov 3, 2022 at 3:53 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

On Tue, 1 Nov 2022 at 23:56, Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

I haven't checked the rest of the patch, but +1 for allowing VACUUM

FULL

within a user txn.

My intention was to prevent that. I am certainly quite uneasy about
changing anything related to CLUSTER/VF, since they are old, complex
and bug prone.

So for now, I will block VF, as was my original intent.

I will be guided by what others think... so you may yet get your wish.

Maybe the error message needs to be qualified "...when multiple
relations are specified".

ERROR: VACUUM cannot run inside a transaction block

Hmm, that is standard wording based on the statement type, but I can
set a CONTEXT message also. Will update accordingly.

Thanks for your input.

New version attached, as described.

Other review comments and alternate opinions welcome.

I applied and did some basic testing on the patch, it works as described.

I would like to bring up a few points that I came across while looking
into the vacuum code.

1. As a result of this change to allow VACUUM inside a user transaction,
I think there is some chance of causing
a block/delay of concurrent VACUUMs if a VACUUM is being run under a long
running transaction.
2. Also, if a user runs VACUUM in a transaction, performance optimizations
like PROC_IN_VACUUM won't work.
3. Also, if VACUUM happens towards the end of a long running transaction,
the snapshot will be old
and xmin horizon for vacuum would be somewhat old as compared to current
lazy vacuum which
acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there
should be some mention
of above caveats in documentation with the recommendation that VACUUM
should be run outside
a transaction, in general.

Sorry, I just noticed that you have already mentioned some of these in the
documentation as follows, so it seems
it is already taken care of.

+    <command>VACUUM</command> cannot be executed inside a transaction
block,
+    unless a single table is specified and <literal>FULL</literal> is not
+    specified.  When executing inside a transaction block the vacuum scan
can
+    hold back the xmin horizon and does not update the database
datfrozenxid,
+    as a result this usage is not useful for database maintenance, but is
provided
+    to allow vacuuming in special circumstances, such as temporary or
private
+    work tables.

Thank you,
Rahila Syed

#9Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Rahila Syed (#8)
Re: Allow single table VACUUM in transaction block

Hi Rahila,

Thanks for your review.

On Fri, 4 Nov 2022 at 07:37, Rahila Syed <rahilasyed90@gmail.com> wrote:

I would like to bring up a few points that I came across while looking into the vacuum code.

1. As a result of this change to allow VACUUM inside a user transaction, I think there is some chance of causing
a block/delay of concurrent VACUUMs if a VACUUM is being run under a long running transaction.
2. Also, if a user runs VACUUM in a transaction, performance optimizations like PROC_IN_VACUUM won't work.
3. Also, if VACUUM happens towards the end of a long running transaction, the snapshot will be old
and xmin horizon for vacuum would be somewhat old as compared to current lazy vacuum which
acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there should be some mention
of above caveats in documentation with the recommendation that VACUUM should be run outside
a transaction, in general.

Sorry, I just noticed that you have already mentioned some of these in the documentation as follows, so it seems
it is already taken care of.

+    <command>VACUUM</command> cannot be executed inside a transaction block,
+    unless a single table is specified and <literal>FULL</literal> is not
+    specified.  When executing inside a transaction block the vacuum scan can
+    hold back the xmin horizon and does not update the database datfrozenxid,
+    as a result this usage is not useful for database maintenance, but is provided
+    to allow vacuuming in special circumstances, such as temporary or private
+    work tables.

Yes, I wondered whether we should have a NOTICE or WARNING to remind
people of those points?

--
Simon Riggs http://www.EnterpriseDB.com/

#10Rahila Syed
rahilasyed90@gmail.com
In reply to: Simon Riggs (#9)
Re: Allow single table VACUUM in transaction block

Hi,

On Fri, Nov 4, 2022 at 2:39 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

Hi Rahila,

Thanks for your review.

On Fri, 4 Nov 2022 at 07:37, Rahila Syed <rahilasyed90@gmail.com> wrote:

I would like to bring up a few points that I came across while looking

into the vacuum code.

1. As a result of this change to allow VACUUM inside a user

transaction, I think there is some chance of causing

a block/delay of concurrent VACUUMs if a VACUUM is being run under a

long running transaction.

2. Also, if a user runs VACUUM in a transaction, performance

optimizations like PROC_IN_VACUUM won't work.

3. Also, if VACUUM happens towards the end of a long running

transaction, the snapshot will be old

and xmin horizon for vacuum would be somewhat old as compared to

current lazy vacuum which

acquires a new snapshot just before scanning the table.

So, while I understand the need of the feature, I am wondering if there

should be some mention

of above caveats in documentation with the recommendation that VACUUM

should be run outside

a transaction, in general.

Sorry, I just noticed that you have already mentioned some of these in

the documentation as follows, so it seems

it is already taken care of.

+ <command>VACUUM</command> cannot be executed inside a transaction

block,

+ unless a single table is specified and <literal>FULL</literal> is

not

+ specified. When executing inside a transaction block the vacuum

scan can

+ hold back the xmin horizon and does not update the database

datfrozenxid,

+ as a result this usage is not useful for database maintenance, but

is provided

+ to allow vacuuming in special circumstances, such as temporary or

private

+ work tables.

Yes, I wondered whether we should have a NOTICE or WARNING to remind
people of those points?

+1 . My vote for NOTICE over WARNING because I think
it is useful information for the user rather than any potential problem.

Thank you,
Rahila Syed

In reply to: Simon Riggs (#1)
Re: Allow single table VACUUM in transaction block

On Thu, Oct 27, 2022 at 2:31 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

Fix, so that this works without issue:

BEGIN;
....
VACUUM (ANALYZE) vactst;
....
COMMIT;

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

When in a xact block, we do not set PROC_IN_VACUUM,
nor update datfrozenxid.

It doesn't seem like a good idea to add various new special cases to
VACUUM just to make scripts like this work. I'm pretty sure that there
are several deep, subtle reasons why VACUUM cannot be assumed safe to
run in a user transaction.

For example, the whole way that index page deletion is decoupled from
recycling in access methods like nbtree (see "Placing deleted pages in
the FSM" from the nbtree README) rests upon delicate assumptions about
whether or not there could be an "in-flight" B-tree descent that is
at risk of landing on a deleted page as it is concurrently recycled.
In general the deleted page has to remain in place as a tombstone,
until that is definitely not possible anymore. This relies on the backend
that runs VACUUM having no references to the page pending deletion.
(Commit 9dd963ae25 added an optimization that heavily leaned on the
idea that the state within the backend running VACUUM couldn't
possibly have a live page reference that is at risk of being broken by
the optimization, though I'm pretty sure that you'd have problems even
without that commit/optimization in place.)

My guess is that there are more things like that. Possibly even things
that were never directly considered. VACUUM evolved in a world where
we absolutely took not running in a transaction for granted. Changing
that now is a pretty big deal. Maybe it would all be worth it if the end
result was a super compelling feature. But I for one don't think that
it will be.

If we absolutely have to do this, then the least worst approach might
be to make VACUUM into a no-op rather than throwing an ERROR -- demote
the ERROR into a WARNING. You could argue that we're just arbitrarily
deciding to not do a VACUUM just to be able to avoid throwing an error
if we do that. But isn't that already true with the patch that we
have? Is it really a "true VACUUM" if the operation can never advance
datfrozenxid? At least selectively demoting the ERROR to a WARNING is
"transparent" about it.

--
Peter Geoghegan

#12Tom Lane
tgl@sss.pgh.pa.us
In reply to: Peter Geoghegan (#11)
Re: Allow single table VACUUM in transaction block

Peter Geoghegan <pg@bowt.ie> writes:

My guess is that there are more things like that. Possibly even things
that were never directly considered. VACUUM evolved in a world where
we absolutely took not running in a transaction for granted. Changing
that now is a pretty big deal. Maybe it would all be worth it if the end
result was a super compelling feature. But I for one don't think that
it will be.

Yeah. To be blunt, this proposal scares the sh*t out of me. I agree
that there are tons of subtle dependencies on our current assumptions
about how VACUUM works, and I strongly suspect that we won't find all of
them until after users have lost data. I cannot believe that running
VACUUM inside transactions is valuable enough to take that risk ...
especially if it's a modified limited kind of VACUUM that doesn't
eliminate the need for periodic real VACUUMs.

In general, I do not believe in encouraging users to run VACUUM
manually in the first place. We would be far better served by
spending our effort to improve autovacuum's shortcomings. I'd
like to see some sort of direct attack on its inability to deal
with temp tables, for instance. (Force the owning backend to
do it? Temporarily change the access rules so that the data
moves to shared buffers? Dunno, but we sure haven't tried hard.)
However many bugs such a thing might have initially, at least
they'd only put temporary data at hazard.

regards, tom lane

In reply to: Tom Lane (#12)
Re: Allow single table VACUUM in transaction block

On Sun, Nov 6, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In general, I do not believe in encouraging users to run VACUUM
manually in the first place. We would be far better served by
spending our effort to improve autovacuum's shortcomings.

I couldn't agree more. A lot of problems seem related to the idea that
VACUUM is just a command that the DBA periodically runs to get a
predictable fixed result, a little like CREATE INDEX. That conceptual
model isn't exactly wrong; it just makes it much harder to apply any
kind of context about the needs of the table over time. There is a
natural cycle to how VACUUM (really autovacuum) is run, and the
details matter.

There is a significant amount of relevant context that we can't really
use right now. That wouldn't be true if VACUUM only ran within an
autovacuum worker (by definition). The VACUUM command itself would
still be available, and support the same user interface, more or less.
Under the hood the VACUUM command would work by enqueueing a VACUUM
job, to be performed asynchronously by an autovacuum worker. Perhaps
the initial enqueue operation could be transactional, fixing Simon's complaint.

"No more VACUUMs outside of autovacuum" would enable more advanced
autovacuum.c scheduling, allowing us to apply a lot more context about
the costs and benefits, without having to treat manual VACUUM as an
independent thing. We could coalesce together redundant VACUUM jobs,
suspend and resume VACUUM operations, and have more strategies to deal
with problems as they emerge.

I'd like to see some sort of direct attack on its inability to deal
with temp tables, for instance. (Force the owning backend to
do it? Temporarily change the access rules so that the data
moves to shared buffers? Dunno, but we sure haven't tried hard.)

This is a good example of the kind of thing I have in mind. Perhaps it
could work by killing the backend that owns the temp relation when
things truly get out of hand? I think that that would be a perfectly
reasonable trade-off.

Another related idea: better behavior in the event of a manually
issued VACUUM (now just an enqueued autovacuum) that cannot do useful
work due to the presence of a long running snapshot. The VACUUM
doesn't have to dutifully report "success" when there is no practical
sense in which it was successful. There could be a back and forth
conversation between autovacuum.c and vacuumlazy.c that makes sure
that something useful happens sooner or later. The passage of time
really matters here.

As a bonus, we might be able to get rid of the autovacuum GUC
variants. Plus the current autovacuum logging would just be how we'd
log every VACUUM.

--
Peter Geoghegan

#14Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Peter Geoghegan (#11)
Re: Allow single table VACUUM in transaction block

On Sun, 6 Nov 2022 at 18:50, Peter Geoghegan <pg@bowt.ie> wrote:

On Thu, Oct 27, 2022 at 2:31 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

Fix, so that this works without issue:

BEGIN;
....
VACUUM (ANALYZE) vactst;
....
COMMIT;

Allows both ANALYZE and vacuum of toast tables, but not VACUUM FULL.

When in a xact block, we do not set PROC_IN_VACUUM,
nor update datfrozenxid.

It doesn't seem like a good idea to add various new special cases to
VACUUM just to make scripts like this work.

Usability is a major concern that doesn't get a high enough priority.

I'm pretty sure that there
are several deep, subtle reasons why VACUUM cannot be assumed safe to
run in a user transaction.

I expected there were, so it's good to discuss them. Thanks for the input.

If we absolutely have to do this, then the least worst approach might
be to make VACUUM into a no-op rather than throwing an ERROR -- demote
the ERROR into a WARNING. You could argue that we're just arbitrarily
deciding to not do a VACUUM just to be able to avoid throwing an error
if we do that. But isn't that already true with the patch that we
have? Is it really a "true VACUUM" if the operation can never advance
datfrozenxid? At least selectively demoting the ERROR to a WARNING is
"transparent" about it.

I'll answer that part in my reply to Tom, since there are good ideas in both.

--
Simon Riggs http://www.EnterpriseDB.com/

#15Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Peter Geoghegan (#13)
Re: Allow single table VACUUM in transaction block

On Sun, 6 Nov 2022 at 20:40, Peter Geoghegan <pg@bowt.ie> wrote:

On Sun, Nov 6, 2022 at 11:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

In general, I do not believe in encouraging users to run VACUUM
manually in the first place. We would be far better served by
spending our effort to improve autovacuum's shortcomings.

I couldn't agree more. A lot of problems seem related to the idea that
VACUUM is just a command that the DBA periodically runs to get a
predictable fixed result, a little like CREATE INDEX. That conceptual
model isn't exactly wrong; it just makes it much harder to apply any
kind of context about the needs of the table over time. There is a
natural cycle to how VACUUM (really autovacuum) is run, and the
details matter.

There is a significant amount of relevant context that we can't really
use right now. That wouldn't be true if VACUUM only ran within an
autovacuum worker (by definition). The VACUUM command itself would
still be available, and support the same user interface, more or less.
Under the hood the VACUUM command would work by enqueueing a VACUUM
job, to be performed asynchronously by an autovacuum worker. Perhaps
the initial enqueue operation could be transactional, fixing Simon's complaint.

Ah, I see you got to this idea first!

Yes, what we need is for the "VACUUM command" to not fail in a script.
Not sure anyone cares where the work takes place.

Enqueuing a request for autovacuum to do that work, then blocking
until it is complete would do the job.

"No more VACUUMs outside of autovacuum" would enable more advanced
autovacuum.c scheduling, allowing us to apply a lot more context about
the costs and benefits, without having to treat manual VACUUM as an
independent thing. We could coalesce together redundant VACUUM jobs,
suspend and resume VACUUM operations, and have more strategies to deal
with problems as they emerge.

+1, but clearly this would not make temp table VACUUMs work.

I'd like to see some sort of direct attack on its inability to deal
with temp tables, for instance. (Force the owning backend to
do it? Temporarily change the access rules so that the data
moves to shared buffers? Dunno, but we sure haven't tried hard.)

This was a $DIRECT attack on making temp tables work! ;-)

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum. So the answer is to
always run a VACUUM FULL on temp tables since this skips any issues
with indexes etc..

We would need to check a few things first.... maybe something like
this (mostly borrowed heavily from COPY)

InvalidateCatalogSnapshot();
if (!ThereAreNoPriorRegisteredSnapshots() || !ThereAreNoReadyPortals())
ereport(WARNING,
(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
errmsg("vacuum of temporary table ignored because
of prior transaction activity")));
CheckTableNotInUse(rel, "VACUUM");

This is a good example of the kind of thing I have in mind. Perhaps it
could work by killing the backend that owns the temp relation when
things truly get out of hand? I think that that would be a perfectly
reasonable trade-off.

+1

Another related idea: better behavior in the event of a manually
issued VACUUM (now just an enqueued autovacuum) that cannot do useful
work due to the presence of a long running snapshot. The VACUUM
doesn't have to dutifully report "success" when there is no practical
sense in which it was successful. There could be a back and forth
conversation between autovacuum.c and vacuumlazy.c that makes sure
that something useful happens sooner or later. The passage of time
really matters here.

Regrettably, neither vacuum nor autovacuum waits for xmin to change;
perhaps it should.

--
Simon Riggs http://www.EnterpriseDB.com/

In reply to: Simon Riggs (#15)
Re: Allow single table VACUUM in transaction block

On Mon, Nov 7, 2022 at 12:20 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

Another related idea: better behavior in the event of a manually
issued VACUUM (now just an enqueued autovacuum) that cannot do useful
work due to the presence of a long running snapshot. The VACUUM
doesn't have to dutifully report "success" when there is no practical
sense in which it was successful. There could be a back and forth
conversation between autovacuum.c and vacuumlazy.c that makes sure
that something useful happens sooner or later. The passage of time
really matters here.

Regrettably, neither vacuum nor autovacuum waits for xmin to change;
perhaps it should.

Yes, it's very primitive right now. In fact I recently discovered that
just using the reloption version (not the GUC version) of
autovacuum_freeze_max_age in a totally straightforward way is all it
takes to utterly confuse autovacuum.c:

/messages/by-id/CAH2-Wz=DJAokY_GhKJchgpa8k9t_H_OVOvfPEn97jGNr9W=deg@mail.gmail.com

It's easy to convince autovacuum.c to launch antiwraparound
autovacuums that reliably have no chance of advancing relfrozenxid to
a degree that satisfies autovacuum.c. It will launch antiwraparound
autovacuums again and again, never realizing that VACUUM doesn't
really care about what it expects (at least not with the reloption in
use). Clearly that's just broken. It also suggests a more general
design problem, at least in my mind.

--
Peter Geoghegan

#17Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Simon Riggs (#15)
Re: Allow single table VACUUM in transaction block

On Mon, 7 Nov 2022 at 08:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum. So the answer is to
always run a VACUUM FULL on temp tables since this skips any issues
with indexes etc..

So I see 3 options for what to do next

1. Force the FULL option for all tables, when executed in a
transaction block. This gets round the reasonable objections to
running a concurrent vacuum in a shared xact block. As Justin points
out, CLUSTER is already supported, which uses the same code.

2. Force the FULL option for temp tables, when executed in a
transaction block. In a later patch, queue up an autovacuum run for
regular tables.

3. Return with feedback this patch. (But then what happens with temp tables?)

Thoughts?

--
Simon Riggs http://www.EnterpriseDB.com/

#18Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Simon Riggs (#17)
1 attachment(s)
Re: Allow single table VACUUM in transaction block

On Tue, 8 Nov 2022 at 03:10, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Mon, 7 Nov 2022 at 08:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum.

Thoughts?

New patch, which does this, when in a xact block

1. For temp tables, only VACUUM FULL is allowed
2. For persistent tables, an AV task is created to perform the vacuum,
which eventually performs a vacuum

The patch works, but there are various aspects of the design that need
input. Thoughts?

--
Simon Riggs http://www.EnterpriseDB.com/

Attachments:

single_table_vacuum_in_xact.v3.patchapplication/octet-stream; name=single_table_vacuum_in_xact.v3.patchDownload
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index c582021d29..3ca300b859 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -366,7 +366,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
    </para>
 
    <para>
-    <command>VACUUM</command> cannot be executed inside a transaction block.
+    <command>VACUUM</command> cannot be executed inside a transaction block
+    for temporary tables, unless <literal>FULL</literal> is specified and
+    the command specifies only a single table.
+    For persistent tables, <command>VACUUM</command> of single tables is allowed
+    inside a transaction block, but is interpreted as a request for an autovacuum
+    worker to perform the vacuum task instead. The caller does not wait for the task
+    to complete; should the task execution fail, there is no response to the caller.
    </para>
 
    <para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 7e386250ae..dcd309a5d0 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -208,7 +208,8 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
 
 				recorded = AutoVacuumRequestWork(AVW_BRINSummarizeRange,
 												 RelationGetRelid(idxRel),
-												 lastPageRange);
+												 lastPageRange,
+												 0);
 				if (!recorded)
 					ereport(LOG,
 							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 3c8ea21475..e17da07ba4 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -269,6 +269,55 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	/* user-invoked vacuum uses VACOPT_VERBOSE instead of log_min_duration */
 	params.log_min_duration = -1;
 
+	/*
+	 * If we're in a transaction block or subtransaction then we will fail
+	 * later during vacuum(), so instead, interpret VACUUM as a request to
+	 * initiate an avw vacuum task, if this is a single persistent table.
+	 */
+	if ((params.options & VACOPT_VACUUM) != 0 &&
+		(IsTransactionBlock() ||
+		 IsSubTransaction()) &&
+		vacstmt->rels != NIL &&
+		list_length(vacstmt->rels) == 1)
+	{
+		ListCell   *lc;
+
+		foreach(lc, vacstmt->rels)
+		{
+			VacuumRelation *vrel = lfirst_node(VacuumRelation, lc);
+			Relation rel = relation_openrv(vrel->relation, AccessShareLock);
+
+			if (!RelationUsesLocalBuffers(rel))
+			{
+				bool requested;
+
+				/*
+				 * Request an autovacuum worker does the work for us,
+				 * and skip the actual execution in the current backend.
+				 * Note that we do not wait for the AV worker to complete
+				 * the task, nor do we check whether it succeeded or not.
+				 */
+				requested = AutoVacuumRequestWork(AVW_VacuumImmediate,
+													rel->rd_id,
+													InvalidBlockNumber,
+													params.options);
+				if (requested)
+					ereport(NOTICE,
+							(errmsg("autovacuum of \"%s\" was requested, using the options specified",
+									RelationGetRelationName(rel))));
+				else
+					ereport(LOG,
+							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+							 errmsg("request for immediate autovacuum of \"%s\" was not recorded",
+									RelationGetRelationName(rel))));
+				relation_close(rel, NoLock);
+				return;
+			}
+			else
+				relation_close(rel, NoLock);
+		}
+	}
+
 	/* Now go through the common routine */
 	vacuum(vacstmt->rels, &params, NULL, isTopLevel);
 }
@@ -297,6 +346,7 @@ vacuum(List *relations, VacuumParams *params,
 	   BufferAccessStrategy bstrategy, bool isTopLevel)
 {
 	static bool in_vacuum = false;
+	bool single_table_ok = false;
 
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -306,15 +356,22 @@ vacuum(List *relations, VacuumParams *params,
 
 	stmttype = (params->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
+	if ((params->options & VACOPT_FULL) != 0 &&
+		relations != NIL &&
+		list_length(relations) == 1)
+		single_table_ok = true;
+
 	/*
-	 * We cannot run VACUUM inside a user transaction block; if we were inside
-	 * a transaction, then our commit- and start-transaction-command calls
+	 * Single-table VACUUM can run inside a user transaction block, but
+	 * we cannot run multiple VACUUMs 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.
+	 * dependencies on this, too, so when running in a transaction block, vacuum
+	 * will skip some of its normal actions, see later for details.
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (params->options & VACOPT_VACUUM)
+	if (params->options & VACOPT_VACUUM && !single_table_ok)
 	{
 		PreventInTransactionBlock(isTopLevel, stmttype);
 		in_outer_xact = false;
@@ -401,9 +458,8 @@ vacuum(List *relations, VacuumParams *params,
 	 * 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.)
+	 * release locks as soon as possible, except for a single table VACUUM
+	 * when it is executed inside a transaction block.
 	 *
 	 * 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
@@ -412,7 +468,7 @@ vacuum(List *relations, VacuumParams *params,
 	 * transactions so we can release locks sooner.
 	 */
 	if (params->options & VACOPT_VACUUM)
-		use_own_xacts = true;
+		use_own_xacts = !in_outer_xact;
 	else
 	{
 		Assert(params->options & VACOPT_ANALYZE);
@@ -427,6 +483,7 @@ vacuum(List *relations, VacuumParams *params,
 	}
 
 	/*
+	 * Tell vacuum_rel whether it will need to manage its own transaction. If so,
 	 * 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
 	 * command, and so we are executing inside a transaction already. We
@@ -437,6 +494,9 @@ vacuum(List *relations, VacuumParams *params,
 	if (use_own_xacts)
 	{
 		Assert(!in_outer_xact);
+		Assert(!IsInTransactionBlock(isTopLevel));
+
+		params->use_own_xact = true;
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -445,6 +505,8 @@ vacuum(List *relations, VacuumParams *params,
 		/* matches the StartTransaction in PostgresMain() */
 		CommitTransactionCommand();
 	}
+	else
+		params->use_own_xact = false;
 
 	/* Turn vacuum cost accounting on or off, and set/clear in_vacuum */
 	PG_TRY();
@@ -1837,9 +1899,10 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	Assert(params != NULL);
 
 	/* Begin a transaction for vacuuming this relation */
-	StartTransactionCommand();
+	if (params->use_own_xact)
+		StartTransactionCommand();
 
-	if (!(params->options & VACOPT_FULL))
+	if (!(params->options & VACOPT_FULL) && (params->use_own_xact))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1852,6 +1915,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 		 * contents of other tables is arguably broken, but we won't break it
 		 * here by violating transaction semantics.)
 		 *
+		 * Don't set PROC_IN_VACUUM if running in a transaction block, since it
+		 * would be very bad for other users to ignore our xact in that case.
+		 * Note that setting the flag is an optional performance tweak, not
+		 * required for correct operation of VACUUM.
+		 *
 		 * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by
 		 * autovacuum; it's used to avoid canceling a vacuum that was invoked
 		 * in an emergency.
@@ -1876,7 +1944,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * cutoff xids in local memory wrapping around, and to have updated xmin
 	 * horizons.
 	 */
-	PushActiveSnapshot(GetTransactionSnapshot());
+	if (params->use_own_xact)
+		PushActiveSnapshot(GetTransactionSnapshot());
 
 	/*
 	 * Check for user-requested abort.  Note we want this to be inside a
@@ -1899,8 +1968,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/* leave if relation could not be opened or locked */
 	if (!rel)
 	{
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1917,8 +1989,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 								  params->options & VACOPT_VACUUM))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1934,8 +2009,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 				(errmsg("skipping \"%s\" --- cannot vacuum non-tables or special system tables",
 						RelationGetRelationName(rel))));
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1949,8 +2027,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (RELATION_IS_OTHER_TEMP(rel))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1962,8 +2043,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		/* It's OK to proceed with ANALYZE on this table */
 		return true;
 	}
@@ -1977,9 +2061,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * NOTE: this cannot block, even if someone else is waiting for access,
 	 * because the lock manager knows that both lock requests are from the
 	 * same process.
+	 *
+	 * If we are in a transaction block, vacuum both main table and toast
+	 * table within the existing transaction, so no session lock required.
 	 */
 	lockrelid = rel->rd_lockInfo.lockRelId;
-	LockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		LockRelationIdForSession(&lockrelid, lmode);
 
 	/*
 	 * Set index_cleanup option based on index_cleanup reloption if it wasn't
@@ -2075,8 +2163,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Complete the transaction and free all temporary memory used.
 	 */
-	PopActiveSnapshot();
-	CommitTransactionCommand();
+	if (params->use_own_xact)
+	{
+		PopActiveSnapshot();
+		CommitTransactionCommand();
+	}
 
 	/*
 	 * If the relation has a secondary toast rel, vacuum that too while we
@@ -2091,7 +2182,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Now release the session-level lock on the main table.
 	 */
-	UnlockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		UnlockRelationIdForSession(&lockrelid, lmode);
 
 	/* Report that we really did it. */
 	return true;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..c6a15097b5 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -258,6 +258,7 @@ typedef struct AutoVacuumWorkItem
 	Oid			avw_database;
 	Oid			avw_relation;
 	BlockNumber avw_blockNumber;
+	bits32      avw_vac_options;
 } AutoVacuumWorkItem;
 
 #define NUM_WORKITEMS	256
@@ -2663,6 +2664,41 @@ perform_work_item(AutoVacuumWorkItem *workitem)
 									ObjectIdGetDatum(workitem->avw_relation),
 									Int64GetDatum((int64) workitem->avw_blockNumber));
 				break;
+			case AVW_VacuumImmediate:
+				{
+					BufferAccessStrategy	bstrategy;
+					autovac_table			tab;
+
+					tab.at_relid = workitem->avw_relation;
+
+					/* Set options */
+					tab.at_params.options = workitem->avw_vac_options;
+					tab.at_params.index_cleanup = VACOPTVALUE_ENABLED;
+					tab.at_params.truncate = VACOPTVALUE_DISABLED;
+
+					/* Deliberately avoid using autovacuum parameters, since this is immediate */
+					tab.at_vacuum_cost_delay = VacuumCostDelay;
+					tab.at_vacuum_cost_limit = VacuumCostLimit;
+					tab.at_dobalance = false;
+					tab.at_sharedrel = false;
+
+					/* Set names in case of error */
+					tab.at_relname = pstrdup(get_rel_name(tab.at_relid));
+					tab.at_nspname = pstrdup(get_namespace_name(get_rel_namespace(tab.at_relid)));
+					tab.at_datname = pstrdup(get_database_name(MyDatabaseId));
+
+					/* XXX what other options should we set? */
+
+					/*
+					 * Create a buffer access strategy object for VACUUM to use.  We want to
+					 * use the same one across all the vacuum operations we perform, since the
+					 * point is for VACUUM not to blow out the shared cache.
+					 */
+					bstrategy = GetAccessStrategy(BAS_VACUUM);
+
+					autovacuum_do_vac_analyze(&tab, bstrategy);
+				}
+				break;
 			default:
 				elog(WARNING, "unrecognized work item found: type %d",
 					 workitem->avw_type);
@@ -3210,6 +3246,11 @@ autovac_report_workitem(AutoVacuumWorkItem *workitem,
 			snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
 					 "autovacuum: BRIN summarize");
 			break;
+
+		case AVW_VacuumImmediate:
+			snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
+					 "autovacuum: user vacuum");
+			break;
 	}
 
 	/*
@@ -3250,7 +3291,7 @@ AutoVacuumingActive(void)
  */
 bool
 AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
-					  BlockNumber blkno)
+					  BlockNumber blkno, bits32 options)
 {
 	int			i;
 	bool		result = false;
@@ -3273,6 +3314,7 @@ AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
 		workitem->avw_database = MyDatabaseId;
 		workitem->avw_relation = relationId;
 		workitem->avw_blockNumber = blkno;
+		workitem->avw_vac_options = options;
 		result = true;
 
 		/* done */
@@ -3281,6 +3323,8 @@ AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
 
 	LWLockRelease(AutovacuumLock);
 
+	/* XXX Should this wake up the AVL? */
+
 	return result;
 }
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5d816ba7f4..35310c588b 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,6 +227,8 @@ typedef struct VacuumParams
 	VacOptValue index_cleanup;	/* Do index vacuum and cleanup */
 	VacOptValue truncate;		/* Truncate empty pages at the end */
 
+	bool		use_own_xact;	/* Use own xact in vacuum_rel? */
+
 	/*
 	 * The number of parallel vacuum workers.  0 by default which means choose
 	 * based on the number of indexes.  -1 indicates parallel vacuum is
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index 9d40fd6d54..f8b91f111b 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -22,7 +22,8 @@
  */
 typedef enum
 {
-	AVW_BRINSummarizeRange
+	AVW_BRINSummarizeRange,
+	AVW_VacuumImmediate
 } AutoVacuumWorkItemType;
 
 
@@ -74,7 +75,7 @@ extern void AutovacuumLauncherIAm(void);
 #endif
 
 extern bool AutoVacuumRequestWork(AutoVacuumWorkItemType type,
-								  Oid relationId, BlockNumber blkno);
+								  Oid relationId, BlockNumber blkno, bits32 options);
 
 /* shared memory stuff */
 extern Size AutoVacuumShmemSize(void);
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c63a157e5f..a3209477a1 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -282,6 +282,19 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 ERROR:  PROCESS_TOAST required with VACUUM FULL
+-- Single table inside transaction block
+CREATE TEMPORARY TABLE vactst_temp (LIKE vactst);
+BEGIN;
+VACUUM FULL vactst_temp;
+COMMIT;
+BEGIN;
+VACUUM vactst_temp;
+ERROR:  VACUUM cannot run inside a transaction block
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+NOTICE:  autovacuum of "vactst" was requested, using the options specified
+COMMIT;
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 9faa8a34a6..6e489a2e40 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -237,6 +237,18 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 
+-- Single table inside transaction block
+CREATE TEMPORARY TABLE vactst_temp (LIKE vactst);
+BEGIN;
+VACUUM FULL vactst_temp;
+COMMIT;
+BEGIN;
+VACUUM vactst_temp;
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+COMMIT;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
#19Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Simon Riggs (#18)
1 attachment(s)
Re: Allow single table VACUUM in transaction block

On Mon, 14 Nov 2022 at 19:52, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Tue, 8 Nov 2022 at 03:10, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Mon, 7 Nov 2022 at 08:20, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

Temp tables are actually easier, since we don't need any of the
concurrency features we get with lazy vacuum.

Thoughts?

New patch, which does this, when in a xact block

1. For temp tables, only VACUUM FULL is allowed
2. For persistent tables, an AV task is created to perform the vacuum,
which eventually performs a vacuum

The patch works, but there are various aspects of the design that need
input. Thoughts?

New version.

--
Simon Riggs http://www.EnterpriseDB.com/

Attachments:

single_table_vacuum_in_xact.v4.patchapplication/octet-stream; name=single_table_vacuum_in_xact.v4.patchDownload
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index c582021d29..3ca300b859 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -366,7 +366,13 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
    </para>
 
    <para>
-    <command>VACUUM</command> cannot be executed inside a transaction block.
+    <command>VACUUM</command> cannot be executed inside a transaction block
+    for temporary tables, unless <literal>FULL</literal> is specified and
+    the command specifies only a single table.
+    For persistent tables, <command>VACUUM</command> of single tables is allowed
+    inside a transaction block, but is interpreted as a request for an autovacuum
+    worker to perform the vacuum task instead. The caller does not wait for the task
+    to complete; should the task execution fail, there is no response to the caller.
    </para>
 
    <para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 7e386250ae..dcd309a5d0 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -208,7 +208,8 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
 
 				recorded = AutoVacuumRequestWork(AVW_BRINSummarizeRange,
 												 RelationGetRelid(idxRel),
-												 lastPageRange);
+												 lastPageRange,
+												 0);
 				if (!recorded)
 					ereport(LOG,
 							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 3c8ea21475..380291f241 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -269,6 +269,82 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	/* user-invoked vacuum uses VACOPT_VERBOSE instead of log_min_duration */
 	params.log_min_duration = -1;
 
+	/*
+	 * Special case for handling normal VACUUMs in a transaction block.
+	 *
+	 * If we're in a transaction block or subtransaction then we will fail
+	 * later during vacuum(), so instead, interpret VACUUM as a request to
+	 * initiate an avw vacuum task, if this is a single persistent table.
+	 */
+	if ((params.options & VACOPT_VACUUM) != 0 &&
+		(params.options & VACOPT_SKIP_LOCKED) == 0 &&
+		(params.options & VACOPT_FULL) == 0 &&
+		vacstmt->rels != NIL &&
+		list_length(vacstmt->rels) == 1 &&
+		(IsTransactionBlock() ||
+		 IsSubTransaction()))
+	{
+		foreach(lc, vacstmt->rels)
+		{
+			VacuumRelation *vrel = lfirst_node(VacuumRelation, lc);
+
+			/*
+			 * We need a lock to allow us to check permissions and to see
+			 * if the relation is persistent. We can't easily handle SKIP LOCKED here.
+			 * We release the lock almost immediately to avoid lock upgrade hazard
+			 * and to ensure we don't interfere with AV.
+			 */
+			Relation rel = relation_openrv(vrel->relation, AccessShareLock);
+
+			if (!RelationUsesLocalBuffers(rel))
+			{
+				bool requested;
+
+				/*
+				 * Check permissions, using identical check to vacuum_rel().
+				 *
+				 * Check if relation needs to be skipped based on ownership.  This check
+				 * happens also when building the relation list to vacuum for a manual
+				 * operation, and needs to be done additionally here as VACUUM could
+				 * happen across multiple transactions where relation ownership could have
+				 * changed in-between.  Make sure to only generate logs for VACUUM in this
+				 * case.
+				 */
+				if (!vacuum_is_relation_owner(RelationGetRelid(rel),
+											  rel->rd_rel,
+											  params.options & VACOPT_VACUUM))
+				{
+					relation_close(rel, AccessShareLock);
+					return;
+				}
+
+				/*
+				 * Request an autovacuum worker does the work for us,
+				 * and skip the actual execution in the current backend.
+				 * Note that we do not wait for the AV worker to complete
+				 * the task, nor do we check whether it succeeded or not.
+				 */
+				requested = AutoVacuumRequestWork(AVW_VacuumImmediate,
+													RelationGetRelid(rel),
+													InvalidBlockNumber,
+													params.options);
+				if (requested)
+					ereport(NOTICE,
+							(errmsg("autovacuum of \"%s\" was requested, using the options specified",
+									RelationGetRelationName(rel))));
+				else
+					ereport(LOG,
+							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+							 errmsg("request for immediate autovacuum of \"%s\" was not recorded",
+									RelationGetRelationName(rel))));
+				relation_close(rel, AccessShareLock);
+				return;
+			}
+			else
+				relation_close(rel, AccessShareLock);
+		}
+	}
+
 	/* Now go through the common routine */
 	vacuum(vacstmt->rels, &params, NULL, isTopLevel);
 }
@@ -297,6 +373,7 @@ vacuum(List *relations, VacuumParams *params,
 	   BufferAccessStrategy bstrategy, bool isTopLevel)
 {
 	static bool in_vacuum = false;
+	bool single_table_ok = false;
 
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -306,15 +383,22 @@ vacuum(List *relations, VacuumParams *params,
 
 	stmttype = (params->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
+	if ((params->options & VACOPT_FULL) != 0 &&
+		relations != NIL &&
+		list_length(relations) == 1)
+		single_table_ok = true;
+
 	/*
-	 * We cannot run VACUUM inside a user transaction block; if we were inside
-	 * a transaction, then our commit- and start-transaction-command calls
+	 * Single-table VACUUM can run inside a user transaction block, but
+	 * we cannot run multiple VACUUMs 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.
+	 * dependencies on this, too, so when running in a transaction block, vacuum
+	 * will skip some of its normal actions, see later for details.
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (params->options & VACOPT_VACUUM)
+	if (params->options & VACOPT_VACUUM && !single_table_ok)
 	{
 		PreventInTransactionBlock(isTopLevel, stmttype);
 		in_outer_xact = false;
@@ -401,9 +485,8 @@ vacuum(List *relations, VacuumParams *params,
 	 * 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.)
+	 * release locks as soon as possible, except for a single table VACUUM
+	 * when it is executed inside a transaction block.
 	 *
 	 * 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
@@ -412,7 +495,7 @@ vacuum(List *relations, VacuumParams *params,
 	 * transactions so we can release locks sooner.
 	 */
 	if (params->options & VACOPT_VACUUM)
-		use_own_xacts = true;
+		use_own_xacts = !in_outer_xact;
 	else
 	{
 		Assert(params->options & VACOPT_ANALYZE);
@@ -427,6 +510,7 @@ vacuum(List *relations, VacuumParams *params,
 	}
 
 	/*
+	 * Tell vacuum_rel whether it will need to manage its own transaction. If so,
 	 * 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
 	 * command, and so we are executing inside a transaction already. We
@@ -437,6 +521,9 @@ vacuum(List *relations, VacuumParams *params,
 	if (use_own_xacts)
 	{
 		Assert(!in_outer_xact);
+		Assert(!IsInTransactionBlock(isTopLevel));
+
+		params->use_own_xact = true;
 
 		/* ActiveSnapshot is not set by autovacuum */
 		if (ActiveSnapshotSet())
@@ -445,6 +532,8 @@ vacuum(List *relations, VacuumParams *params,
 		/* matches the StartTransaction in PostgresMain() */
 		CommitTransactionCommand();
 	}
+	else
+		params->use_own_xact = false;
 
 	/* Turn vacuum cost accounting on or off, and set/clear in_vacuum */
 	PG_TRY();
@@ -1837,9 +1926,10 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	Assert(params != NULL);
 
 	/* Begin a transaction for vacuuming this relation */
-	StartTransactionCommand();
+	if (params->use_own_xact)
+		StartTransactionCommand();
 
-	if (!(params->options & VACOPT_FULL))
+	if (!(params->options & VACOPT_FULL) && (params->use_own_xact))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1852,6 +1942,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 		 * contents of other tables is arguably broken, but we won't break it
 		 * here by violating transaction semantics.)
 		 *
+		 * Don't set PROC_IN_VACUUM if running in a transaction block, since it
+		 * would be very bad for other users to ignore our xact in that case.
+		 * Note that setting the flag is an optional performance tweak, not
+		 * required for correct operation of VACUUM.
+		 *
 		 * We also set the VACUUM_FOR_WRAPAROUND flag, which is passed down by
 		 * autovacuum; it's used to avoid canceling a vacuum that was invoked
 		 * in an emergency.
@@ -1876,7 +1971,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * cutoff xids in local memory wrapping around, and to have updated xmin
 	 * horizons.
 	 */
-	PushActiveSnapshot(GetTransactionSnapshot());
+	if (params->use_own_xact)
+		PushActiveSnapshot(GetTransactionSnapshot());
 
 	/*
 	 * Check for user-requested abort.  Note we want this to be inside a
@@ -1899,8 +1995,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/* leave if relation could not be opened or locked */
 	if (!rel)
 	{
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1917,8 +2016,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 								  params->options & VACOPT_VACUUM))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1934,8 +2036,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 				(errmsg("skipping \"%s\" --- cannot vacuum non-tables or special system tables",
 						RelationGetRelationName(rel))));
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1949,8 +2054,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (RELATION_IS_OTHER_TEMP(rel))
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		return false;
 	}
 
@@ -1962,8 +2070,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		relation_close(rel, lmode);
-		PopActiveSnapshot();
-		CommitTransactionCommand();
+		if (params->use_own_xact)
+		{
+			PopActiveSnapshot();
+			CommitTransactionCommand();
+		}
 		/* It's OK to proceed with ANALYZE on this table */
 		return true;
 	}
@@ -1977,9 +2088,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	 * NOTE: this cannot block, even if someone else is waiting for access,
 	 * because the lock manager knows that both lock requests are from the
 	 * same process.
+	 *
+	 * If we are in a transaction block, vacuum both main table and toast
+	 * table within the existing transaction, so no session lock required.
 	 */
 	lockrelid = rel->rd_lockInfo.lockRelId;
-	LockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		LockRelationIdForSession(&lockrelid, lmode);
 
 	/*
 	 * Set index_cleanup option based on index_cleanup reloption if it wasn't
@@ -2075,8 +2190,11 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Complete the transaction and free all temporary memory used.
 	 */
-	PopActiveSnapshot();
-	CommitTransactionCommand();
+	if (params->use_own_xact)
+	{
+		PopActiveSnapshot();
+		CommitTransactionCommand();
+	}
 
 	/*
 	 * If the relation has a secondary toast rel, vacuum that too while we
@@ -2091,7 +2209,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	/*
 	 * Now release the session-level lock on the main table.
 	 */
-	UnlockRelationIdForSession(&lockrelid, lmode);
+	if (params->use_own_xact)
+		UnlockRelationIdForSession(&lockrelid, lmode);
 
 	/* Report that we really did it. */
 	return true;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..c6a15097b5 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -258,6 +258,7 @@ typedef struct AutoVacuumWorkItem
 	Oid			avw_database;
 	Oid			avw_relation;
 	BlockNumber avw_blockNumber;
+	bits32      avw_vac_options;
 } AutoVacuumWorkItem;
 
 #define NUM_WORKITEMS	256
@@ -2663,6 +2664,41 @@ perform_work_item(AutoVacuumWorkItem *workitem)
 									ObjectIdGetDatum(workitem->avw_relation),
 									Int64GetDatum((int64) workitem->avw_blockNumber));
 				break;
+			case AVW_VacuumImmediate:
+				{
+					BufferAccessStrategy	bstrategy;
+					autovac_table			tab;
+
+					tab.at_relid = workitem->avw_relation;
+
+					/* Set options */
+					tab.at_params.options = workitem->avw_vac_options;
+					tab.at_params.index_cleanup = VACOPTVALUE_ENABLED;
+					tab.at_params.truncate = VACOPTVALUE_DISABLED;
+
+					/* Deliberately avoid using autovacuum parameters, since this is immediate */
+					tab.at_vacuum_cost_delay = VacuumCostDelay;
+					tab.at_vacuum_cost_limit = VacuumCostLimit;
+					tab.at_dobalance = false;
+					tab.at_sharedrel = false;
+
+					/* Set names in case of error */
+					tab.at_relname = pstrdup(get_rel_name(tab.at_relid));
+					tab.at_nspname = pstrdup(get_namespace_name(get_rel_namespace(tab.at_relid)));
+					tab.at_datname = pstrdup(get_database_name(MyDatabaseId));
+
+					/* XXX what other options should we set? */
+
+					/*
+					 * Create a buffer access strategy object for VACUUM to use.  We want to
+					 * use the same one across all the vacuum operations we perform, since the
+					 * point is for VACUUM not to blow out the shared cache.
+					 */
+					bstrategy = GetAccessStrategy(BAS_VACUUM);
+
+					autovacuum_do_vac_analyze(&tab, bstrategy);
+				}
+				break;
 			default:
 				elog(WARNING, "unrecognized work item found: type %d",
 					 workitem->avw_type);
@@ -3210,6 +3246,11 @@ autovac_report_workitem(AutoVacuumWorkItem *workitem,
 			snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
 					 "autovacuum: BRIN summarize");
 			break;
+
+		case AVW_VacuumImmediate:
+			snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
+					 "autovacuum: user vacuum");
+			break;
 	}
 
 	/*
@@ -3250,7 +3291,7 @@ AutoVacuumingActive(void)
  */
 bool
 AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
-					  BlockNumber blkno)
+					  BlockNumber blkno, bits32 options)
 {
 	int			i;
 	bool		result = false;
@@ -3273,6 +3314,7 @@ AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
 		workitem->avw_database = MyDatabaseId;
 		workitem->avw_relation = relationId;
 		workitem->avw_blockNumber = blkno;
+		workitem->avw_vac_options = options;
 		result = true;
 
 		/* done */
@@ -3281,6 +3323,8 @@ AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
 
 	LWLockRelease(AutovacuumLock);
 
+	/* XXX Should this wake up the AVL? */
+
 	return result;
 }
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 5d816ba7f4..35310c588b 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,6 +227,8 @@ typedef struct VacuumParams
 	VacOptValue index_cleanup;	/* Do index vacuum and cleanup */
 	VacOptValue truncate;		/* Truncate empty pages at the end */
 
+	bool		use_own_xact;	/* Use own xact in vacuum_rel? */
+
 	/*
 	 * The number of parallel vacuum workers.  0 by default which means choose
 	 * based on the number of indexes.  -1 indicates parallel vacuum is
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index 9d40fd6d54..f8b91f111b 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -22,7 +22,8 @@
  */
 typedef enum
 {
-	AVW_BRINSummarizeRange
+	AVW_BRINSummarizeRange,
+	AVW_VacuumImmediate
 } AutoVacuumWorkItemType;
 
 
@@ -74,7 +75,7 @@ extern void AutovacuumLauncherIAm(void);
 #endif
 
 extern bool AutoVacuumRequestWork(AutoVacuumWorkItemType type,
-								  Oid relationId, BlockNumber blkno);
+								  Oid relationId, BlockNumber blkno, bits32 options);
 
 /* shared memory stuff */
 extern Size AutoVacuumShmemSize(void);
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c63a157e5f..a3209477a1 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -282,6 +282,19 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 ERROR:  PROCESS_TOAST required with VACUUM FULL
+-- Single table inside transaction block
+CREATE TEMPORARY TABLE vactst_temp (LIKE vactst);
+BEGIN;
+VACUUM FULL vactst_temp;
+COMMIT;
+BEGIN;
+VACUUM vactst_temp;
+ERROR:  VACUUM cannot run inside a transaction block
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+NOTICE:  autovacuum of "vactst" was requested, using the options specified
+COMMIT;
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 9faa8a34a6..6e489a2e40 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -237,6 +237,18 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 
+-- Single table inside transaction block
+CREATE TEMPORARY TABLE vactst_temp (LIKE vactst);
+BEGIN;
+VACUUM FULL vactst_temp;
+COMMIT;
+BEGIN;
+VACUUM vactst_temp;
+COMMIT;
+BEGIN;
+VACUUM (ANALYZE) vactst;
+COMMIT;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
#20Greg Stark
stark@mit.edu
In reply to: Simon Riggs (#19)
Re: Allow single table VACUUM in transaction block

I think the idea of being able to request an autovacuum worker for a
specific table is actually very good. I think it's what most users
actually want when they are running vacuum. In fact in previous jobs
people have built infrastructure that basically duplicates autovacuum
just so they could do this.

However I'm not a fan of commands that sometimes do one thing and
sometimes magically do something very different. I don't like the idea
that the same vacuum command would sometimes run in-process and
sometimes do this out of process request. And the rules for when it
does which are fairly complex to explain -- it runs in process unless
you're in a transaction when it runs out of process unless it's a
temporary table ...

I think this requesting autovacuum worker should be a distinct
command. Or at least an explicit option to vacuum.

Also, I was confused reading the thread above about mention of VACUUM
FULL. I don't understand why it's relevant at all. We certainly can't
force VACUUM FULL when it wasn't requested on potentially large
tables.

#21Tom Lane
tgl@sss.pgh.pa.us
In reply to: Greg Stark (#20)
Re: Allow single table VACUUM in transaction block

Greg Stark <stark@mit.edu> writes:

I think this requesting autovacuum worker should be a distinct
command. Or at least an explicit option to vacuum.

+1. That'd reduce confusion, and perhaps we could remove some
of the restrictions.

regards, tom lane

#22Justin Pryzby
pryzby@telsasoft.com
In reply to: Greg Stark (#20)
Re: Allow single table VACUUM in transaction block

On Wed, Nov 16, 2022 at 05:14:07PM -0500, Greg Stark wrote:

I think this requesting autovacuum worker should be a distinct
command. Or at least an explicit option to vacuum.

+1. I was going to suggest VACUUM (NOWAIT) ..

--
Justin

#23Robert Haas
robertmhaas@gmail.com
In reply to: Greg Stark (#20)
Re: Allow single table VACUUM in transaction block

On Wed, Nov 16, 2022 at 5:14 PM Greg Stark <stark@mit.edu> wrote:

However I'm not a fan of commands that sometimes do one thing and
sometimes magically do something very different. I don't like the idea
that the same vacuum command would sometimes run in-process and
sometimes do this out of process request. And the rules for when it
does which are fairly complex to explain -- it runs in process unless
you're in a transaction when it runs out of process unless it's a
temporary table ...

100% agree.

--
Robert Haas
EDB: http://www.enterprisedb.com

#24Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Justin Pryzby (#22)
Re: Allow single table VACUUM in transaction block

On Thu, 17 Nov 2022 at 20:00, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Nov 16, 2022 at 05:14:07PM -0500, Greg Stark wrote:

I think this requesting autovacuum worker should be a distinct
command. Or at least an explicit option to vacuum.

+1. I was going to suggest VACUUM (NOWAIT) ..

Yes, I have no problem with an explicit command.

At the moment the patch runs VACUUM in the background in an autovacuum
process, but the call is asynchronous, since we do not wait for the
command to finish (or even start).

So the command names I was thinking of would be one of these:

VACUUM (BACKGROUND) or VACUUM (AUTOVACUUM) - which might be clearer
or
VACUUM (ASYNC) - which is more descriptive of the behavior

or we could go for both
VACUUM (BACKGROUND, ASYNC) - since this allows us to have a
BACKGROUND, SYNC version in the future

Thoughts?

--
Simon Riggs http://www.EnterpriseDB.com/

#25Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Robert Haas (#23)
Re: Allow single table VACUUM in transaction block

On Thu, 17 Nov 2022 at 20:06, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Nov 16, 2022 at 5:14 PM Greg Stark <stark@mit.edu> wrote:

However I'm not a fan of commands that sometimes do one thing and
sometimes magically do something very different. I don't like the idea
that the same vacuum command would sometimes run in-process and
sometimes do this out of process request. And the rules for when it
does which are fairly complex to explain -- it runs in process unless
you're in a transaction when it runs out of process unless it's a
temporary table ...

100% agree.

I agree as well.

At the moment, the problem (OT) is that VACUUM behaves inconsistently

Outside a transaction - works perfectly
In a transaction - throws ERROR, which prevents a whole script from
executing correctly

What we are trying to do is avoid the ERROR. I don't want them to
behave like this, but that's the only option possible to avoid ERROR.

So if consistency is also a strong requirement, then maybe we should
make that new command the default, i.e. make VACUUM always just a
request to vacuum in background. That way it will be consistent.

Can we at least have a vacuum_runs_in_background = on | off, to allow
users to take advantage of this WITHOUT needing to rewrite all of
their scripts?

--
Simon Riggs http://www.EnterpriseDB.com/

#26Robert Haas
robertmhaas@gmail.com
In reply to: Simon Riggs (#25)
Re: Allow single table VACUUM in transaction block

On Fri, Nov 18, 2022 at 7:04 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

Outside a transaction - works perfectly
In a transaction - throws ERROR, which prevents a whole script from
executing correctly

Right, but your proposal would move that inconsistency to a different
place. It wouldn't eliminate it. I don't think we can pretend that
nobody will notice their operation being moved to the background. For
instance, there might not be an available background worker for a long
time, which could mean that some vacuums work right away and others
just sit there for reasons that aren't obvious to the user.

So if consistency is also a strong requirement, then maybe we should
make that new command the default, i.e. make VACUUM always just a
request to vacuum in background. That way it will be consistent.

Since one fairly common reason for running vacuum in the foreground is
needing to vacuum a table when all autovacuum workers are busy, or
when they are vacuuming it with a cost limit and it needs to get done
sooner, I think this would surprise a lot of users in a negative way.

Can we at least have a vacuum_runs_in_background = on | off, to allow
users to take advantage of this WITHOUT needing to rewrite all of
their scripts?

I'm not entirely convinced that's a good idea, but happy to hear what
others think.

--
Robert Haas
EDB: http://www.enterprisedb.com

#27Tom Lane
tgl@sss.pgh.pa.us
In reply to: Robert Haas (#26)
Re: Allow single table VACUUM in transaction block

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Nov 18, 2022 at 7:04 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

So if consistency is also a strong requirement, then maybe we should
make that new command the default, i.e. make VACUUM always just a
request to vacuum in background. That way it will be consistent.

Since one fairly common reason for running vacuum in the foreground is
needing to vacuum a table when all autovacuum workers are busy, or
when they are vacuuming it with a cost limit and it needs to get done
sooner, I think this would surprise a lot of users in a negative way.

It would also break a bunch of our regression tests, which expect a
VACUUM to complete immediately.

Can we at least have a vacuum_runs_in_background = on | off, to allow
users to take advantage of this WITHOUT needing to rewrite all of
their scripts?

I'm not entirely convinced that's a good idea, but happy to hear what
others think.

I think the answer to that one is flat no. We learned long ago that GUCs
with significant semantic impact on queries are a bad idea. For example,
if a user issues VACUUM expecting behavior A and she gets behavior B
because somebody changed the postgresql.conf entry, she won't be happy.

Basically, I am not buying Simon's requirement that this be transparent.
I think the downsides would completely outweigh whatever upside there
may be (and given the shortage of prior complaints, I don't think the
upside is very large).

regards, tom lane

#28Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Tom Lane (#27)
Re: Allow single table VACUUM in transaction block

On Fri, 18 Nov 2022 at 18:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Nov 18, 2022 at 7:04 AM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

So if consistency is also a strong requirement, then maybe we should
make that new command the default, i.e. make VACUUM always just a
request to vacuum in background. That way it will be consistent.

Since one fairly common reason for running vacuum in the foreground is
needing to vacuum a table when all autovacuum workers are busy, or
when they are vacuuming it with a cost limit and it needs to get done
sooner, I think this would surprise a lot of users in a negative way.

It would also break a bunch of our regression tests, which expect a
VACUUM to complete immediately.

Can we at least have a vacuum_runs_in_background = on | off, to allow
users to take advantage of this WITHOUT needing to rewrite all of
their scripts?

I'm not entirely convinced that's a good idea, but happy to hear what
others think.

I think the answer to that one is flat no. We learned long ago that GUCs
with significant semantic impact on queries are a bad idea. For example,
if a user issues VACUUM expecting behavior A and she gets behavior B
because somebody changed the postgresql.conf entry, she won't be happy.

Basically, I am not buying Simon's requirement that this be transparent.
I think the downsides would completely outweigh whatever upside there
may be (and given the shortage of prior complaints, I don't think the
upside is very large).

Just to say I'm happy with that decision and will switch to the
request for a background vacuum.

--
Simon Riggs http://www.EnterpriseDB.com/

#29Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Simon Riggs (#24)
1 attachment(s)
Re: Allow single table VACUUM in transaction block

On Fri, 18 Nov 2022 at 11:54, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Thu, 17 Nov 2022 at 20:00, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Wed, Nov 16, 2022 at 05:14:07PM -0500, Greg Stark wrote:

I think this requesting autovacuum worker should be a distinct
command. Or at least an explicit option to vacuum.

+1. I was going to suggest VACUUM (NOWAIT) ..

Yes, I have no problem with an explicit command.

At the moment the patch runs VACUUM in the background in an autovacuum
process, but the call is asynchronous, since we do not wait for the
command to finish (or even start).

So the command names I was thinking of would be one of these:

VACUUM (BACKGROUND) or VACUUM (AUTOVACUUM) - which might be clearer
or
VACUUM (ASYNC) - which is more descriptive of the behavior

or we could go for both
VACUUM (BACKGROUND, ASYNC) - since this allows us to have a
BACKGROUND, SYNC version in the future

Attached patch implements VACUUM (BACKGROUND).

There are quite a few small details to consider; please read the docs
and comments.

There is a noticeable delay before the background vacuum starts.

--
Simon Riggs http://www.EnterpriseDB.com/

Attachments:

background_vacuum.v3.patchapplication/octet-stream; name=background_vacuum.v3.patchDownload
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index c582021d29..c30d467644 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -250,6 +250,23 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term><literal>BACKGROUND</literal></term>
+    <listitem>
+     <para>
+      Specifies that <command>VACUUM</command> will make a request to
+      Autovacuum to process the task in the background. When this option is
+      specified the command returns immediately, without waiting for the
+      task to complete and without notification of success or failure.
+      Execution of the task by autovacuum will take place at a future time
+      and is not prioritized over its other planned actions. There is no
+      current limit on the number of background tasks that can be queued,
+      but the command will throw an ERROR if the task queue is full.
+      This option may only be specified with a single persistent table.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>TRUNCATE</literal></term>
     <listitem>
@@ -366,7 +383,8 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
    </para>
 
    <para>
-    <command>VACUUM</command> cannot be executed inside a transaction block.
+    <command>VACUUM</command> cannot be executed inside a transaction block,
+    except when the <option>BACKGROUND</option> option is specified.
    </para>
 
    <para>
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 7e386250ae..dcd309a5d0 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -208,7 +208,8 @@ brininsert(Relation idxRel, Datum *values, bool *nulls,
 
 				recorded = AutoVacuumRequestWork(AVW_BRINSummarizeRange,
 												 RelationGetRelid(idxRel),
-												 lastPageRange);
+												 lastPageRange,
+												 0);
 				if (!recorded)
 					ereport(LOG,
 							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 3c8ea21475..8fde1fcc3a 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -114,6 +114,7 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	bool		full = false;
 	bool		disable_page_skipping = false;
 	bool		process_toast = true;
+	bool		background = false;
 	ListCell   *lc;
 
 	/* index_cleanup and truncate values unspecified for now */
@@ -148,6 +149,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 			full = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "disable_page_skipping") == 0)
 			disable_page_skipping = defGetBoolean(opt);
+		else if (strcmp(opt->defname, "background") == 0)
+			background = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "index_cleanup") == 0)
 		{
 			/* Interpret no string as the default, which is 'auto' */
@@ -244,6 +247,23 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 		}
 	}
 
+	if (background && (params.options & VACOPT_VERBOSE))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("VACUUM (BACKGROUND) cannot be verbose")));
+
+	if (background && (params.options & VACOPT_SKIP_LOCKED))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("VACUUM (BACKGROUND) cannot SKIP_LOCKED")));
+
+	if (background &&
+		(vacstmt->rels == NIL ||
+		list_length(vacstmt->rels) != 1))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("VACUUM (BACKGROUND) only allowed on a single table")));
+
 	/*
 	 * All freeze ages are zero if the FREEZE option is given; otherwise pass
 	 * them as -1 which means to use the default values.
@@ -269,6 +289,86 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	/* user-invoked vacuum uses VACOPT_VERBOSE instead of log_min_duration */
 	params.log_min_duration = -1;
 
+	/*
+	 * If we are requested to run vacuum in the background, request work
+	 * from Autovacuum. Make sure this is a table that AV can see
+	 * and check permissions now also.
+	 *
+	 * Note also that "background" is not a VACOPT item, since we don't need
+	 * vacuum() to know about that, and we wouldn't want autovacuum to itself
+	 * try to queue something into the background for execution.
+	 *
+	 * XXX note this does not yet handle partitioned tables correctly;
+	 * these just fail silently in vacuum_rel() at present.
+	 */
+	if (background)
+	{
+		foreach(lc, vacstmt->rels)
+		{
+			VacuumRelation *vrel = lfirst_node(VacuumRelation, lc);
+
+			/*
+			 * We need a lock to allow us to check permissions and to see
+			 * if the relation is persistent. We can't easily handle SKIP LOCKED here.
+			 * We release the lock almost immediately to avoid lock upgrade hazard
+			 * and to ensure we don't interfere with AV.
+			 */
+			Relation rel = relation_openrv(vrel->relation, AccessShareLock);
+
+			if (RelationUsesLocalBuffers(rel))
+			{
+				relation_close(rel, AccessShareLock);
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("BACKGROUND option not supported on temporary tables")));
+			}
+			else
+			{
+				bool requested;
+
+				/*
+				 * Check permissions, using identical check to vacuum_rel().
+				 *
+				 * Check if relation needs to be skipped based on ownership.  This check
+				 * happens also when building the relation list to vacuum for a manual
+				 * operation, and needs to be done additionally here as VACUUM could
+				 * happen across multiple transactions where relation ownership could have
+				 * changed in-between.  Make sure to only generate logs for VACUUM in this
+				 * case.
+				 */
+				if (!vacuum_is_relation_owner(RelationGetRelid(rel),
+											  rel->rd_rel,
+											  params.options & VACOPT_VACUUM))
+				{
+					relation_close(rel, AccessShareLock);
+					return;
+				}
+
+				/*
+				 * Request an autovacuum worker does the work for us,
+				 * and skip the actual execution in the current backend.
+				 * Note that we do not wait for the AV worker to complete
+				 * the task, nor do we check whether it succeeded or not.
+				 */
+				requested = AutoVacuumRequestWork(AVW_BackgroundVacuum,
+													RelationGetRelid(rel),
+													InvalidBlockNumber,
+													params.options);
+				if (requested)
+					ereport(NOTICE,
+							(errmsg("autovacuum of \"%s\" has been requested, using the options specified",
+									RelationGetRelationName(rel))));
+				else
+					ereport(ERROR,
+							(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+							 errmsg("task queue full; background autovacuum of \"%s\" was not recorded",
+									RelationGetRelationName(rel))));
+				relation_close(rel, AccessShareLock);
+				return;
+			}
+		}
+	}
+
 	/* Now go through the common routine */
 	vacuum(vacstmt->rels, &params, NULL, isTopLevel);
 }
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 601834d4b4..446c87827c 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -258,6 +258,7 @@ typedef struct AutoVacuumWorkItem
 	Oid			avw_database;
 	Oid			avw_relation;
 	BlockNumber avw_blockNumber;
+	bits32      avw_vac_options;
 } AutoVacuumWorkItem;
 
 #define NUM_WORKITEMS	256
@@ -619,6 +620,12 @@ AutoVacLauncherMain(int argc, char *argv[])
 
 	AutoVacuumShmem->av_launcherpid = MyProcPid;
 
+	/*
+	 * Advertise our latch that backends can use to wake us up while we're
+	 * sleeping.
+	 */
+	ProcGlobal->autovacuumLatch = &MyProc->procLatch;
+
 	/*
 	 * Create the initial database list.  The invariant we want this list to
 	 * keep is that it's ordered by decreasing next_time.  As soon as an entry
@@ -2663,6 +2670,66 @@ perform_work_item(AutoVacuumWorkItem *workitem)
 									ObjectIdGetDatum(workitem->avw_relation),
 									Int64GetDatum((int64) workitem->avw_blockNumber));
 				break;
+			case AVW_BackgroundVacuum:
+				{
+					BufferAccessStrategy	bstrategy;
+					autovac_table			tab;
+
+					tab.at_relid = workitem->avw_relation;
+					tab.at_sharedrel = false;
+
+					tab.at_params.index_cleanup = VACOPTVALUE_ENABLED;
+					tab.at_params.truncate = VACOPTVALUE_DISABLED;
+
+					/* Set options */
+					tab.at_params.options = workitem->avw_vac_options;
+
+					/*
+					 * All freeze ages are zero if the FREEZE option is given; otherwise pass
+					 * them as -1 which means to use the default values.
+					*/
+					if (tab.at_params.options & VACOPT_FREEZE)
+					{
+						tab.at_params.freeze_min_age = 0;
+						tab.at_params.freeze_table_age = 0;
+						tab.at_params.multixact_freeze_min_age = 0;
+						tab.at_params.multixact_freeze_table_age = 0;
+					}
+					else
+					{
+						tab.at_params.freeze_min_age = -1;
+						tab.at_params.freeze_table_age = -1;
+						tab.at_params.multixact_freeze_min_age = -1;
+						tab.at_params.multixact_freeze_table_age = -1;
+					}
+
+					/* user-invoked vacuum is never "for wraparound" */
+					tab.at_params.is_wraparound = false;
+
+					/* use default values */
+					tab.at_params.log_min_duration = 0;
+					tab.at_params.nworkers = 0;
+
+					/* Deliberately avoid using autovacuum parameters, since this is for user */
+					tab.at_vacuum_cost_delay = VacuumCostDelay;
+					tab.at_vacuum_cost_limit = VacuumCostLimit;
+					tab.at_dobalance = false;
+
+					/* Set names in case of error */
+					tab.at_relname = pstrdup(get_rel_name(tab.at_relid));
+					tab.at_nspname = pstrdup(get_namespace_name(get_rel_namespace(tab.at_relid)));
+					tab.at_datname = pstrdup(get_database_name(MyDatabaseId));
+
+					/*
+					 * Create a buffer access strategy object for VACUUM to use.  We want to
+					 * use the same one across all the vacuum operations we perform, since the
+					 * point is for VACUUM not to blow out the shared cache.
+					 */
+					bstrategy = GetAccessStrategy(BAS_VACUUM);
+
+					autovacuum_do_vac_analyze(&tab, bstrategy);
+				}
+				break;
 			default:
 				elog(WARNING, "unrecognized work item found: type %d",
 					 workitem->avw_type);
@@ -3210,6 +3277,11 @@ autovac_report_workitem(AutoVacuumWorkItem *workitem,
 			snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
 					 "autovacuum: BRIN summarize");
 			break;
+
+		case AVW_BackgroundVacuum:
+			snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
+					 "autovacuum: user vacuum");
+			break;
 	}
 
 	/*
@@ -3250,7 +3322,7 @@ AutoVacuumingActive(void)
  */
 bool
 AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
-					  BlockNumber blkno)
+					  BlockNumber blkno, bits32 options)
 {
 	int			i;
 	bool		result = false;
@@ -3273,6 +3345,7 @@ AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
 		workitem->avw_database = MyDatabaseId;
 		workitem->avw_relation = relationId;
 		workitem->avw_blockNumber = blkno;
+		workitem->avw_vac_options = options;
 		result = true;
 
 		/* done */
@@ -3281,6 +3354,14 @@ AutoVacuumRequestWork(AutoVacuumWorkItemType type, Oid relationId,
 
 	LWLockRelease(AutovacuumLock);
 
+	/*
+	 * You might think we would issue a SetLatch here to wake up the
+	 * Autovacuum, but the current mechanism uses specifically timed
+	 * requests, so that would require significant rethinking.
+	 * We just need to be patient that AV will get to our task
+	 * eventually.
+	 */
+
 	return result;
 }
 
diff --git a/src/include/postmaster/autovacuum.h b/src/include/postmaster/autovacuum.h
index 9d40fd6d54..44cd979e9b 100644
--- a/src/include/postmaster/autovacuum.h
+++ b/src/include/postmaster/autovacuum.h
@@ -22,7 +22,8 @@
  */
 typedef enum
 {
-	AVW_BRINSummarizeRange
+	AVW_BRINSummarizeRange,
+	AVW_BackgroundVacuum
 } AutoVacuumWorkItemType;
 
 
@@ -74,7 +75,7 @@ extern void AutovacuumLauncherIAm(void);
 #endif
 
 extern bool AutoVacuumRequestWork(AutoVacuumWorkItemType type,
-								  Oid relationId, BlockNumber blkno);
+								  Oid relationId, BlockNumber blkno, bits32 options);
 
 /* shared memory stuff */
 extern Size AutoVacuumShmemSize(void);
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 8d096fdeeb..92fa275178 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -400,6 +400,8 @@ typedef struct PROC_HDR
 	Latch	   *walwriterLatch;
 	/* Checkpointer process's latch */
 	Latch	   *checkpointerLatch;
+	/* Autovacuum process's latch */
+	Latch	   *autovacuumLatch;
 	/* Current shared estimate of appropriate spins_per_delay value */
 	int			spins_per_delay;
 	/* Buffer id of the buffer that Startup process waits for pin on, or -1 */
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c63a157e5f..1e3b182414 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -282,6 +282,13 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 ERROR:  PROCESS_TOAST required with VACUUM FULL
+-- Single table inside transaction block
+VACUUM (BACKGROUND) vactst;
+NOTICE:  autovacuum of "vactst" has been requested, using the options specified
+BEGIN;
+VACUUM (BACKGROUND) vactst;
+NOTICE:  autovacuum of "vactst" has been requested, using the options specified
+COMMIT;
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
diff --git a/src/test/regress/sql/vacuum.sql b/src/test/regress/sql/vacuum.sql
index 9faa8a34a6..3782e13a2b 100644
--- a/src/test/regress/sql/vacuum.sql
+++ b/src/test/regress/sql/vacuum.sql
@@ -237,6 +237,12 @@ ALTER TABLE vactst ALTER COLUMN t SET STORAGE EXTERNAL;
 VACUUM (PROCESS_TOAST FALSE) vactst;
 VACUUM (PROCESS_TOAST FALSE, FULL) vactst;
 
+-- Single table inside transaction block
+VACUUM (BACKGROUND) vactst;
+BEGIN;
+VACUUM (BACKGROUND) vactst;
+COMMIT;
+
 DROP TABLE vaccluster;
 DROP TABLE vactst;
 DROP TABLE vacparted;
#30Justin Pryzby
pryzby@telsasoft.com
In reply to: Simon Riggs (#29)
Re: Allow single table VACUUM in transaction block

On Mon, Nov 21, 2022 at 03:07:25PM +0000, Simon Riggs wrote:

Attached patch implements VACUUM (BACKGROUND).

There are quite a few small details to consider; please read the docs
and comments.

There is a noticeable delay before the background vacuum starts.

You disallowed some combinations of unsupported options, but not others,
like FULL, PARALLEL, etc. They should either be supported or
prohibited.

+                                       /* use default values */
+                                       tab.at_params.log_min_duration = 0;

0 isn't the default ?

Maybe VERBOSE should mean to set min_duration=0, otherwise it should use
the default ?

You only handle one rel, but ExecVacuum() has a loop around rels.

+NOTICE: autovacuum of "vactst" has been requested, using the options specified

=> I don't think it's useful to say "using the options specified".

Should autovacuum de-duplicate requests ?
BRIN doesn't do that, but it's intended for append-only tables, so the
issue doesn't really come up.

Could add psql tab-completion.

Is it going to be confusing that the session's GUC variables won't be
transmitted to autovacuum ? For example, the freeze and costing
parameters.

--
Justin

#31Simon Riggs
simon.riggs@enterprisedb.com
In reply to: Justin Pryzby (#30)
Re: Allow single table VACUUM in transaction block

On Tue, 22 Nov 2022 at 16:43, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Mon, Nov 21, 2022 at 03:07:25PM +0000, Simon Riggs wrote:

Attached patch implements VACUUM (BACKGROUND).

There are quite a few small details to consider; please read the docs
and comments.

There is a noticeable delay before the background vacuum starts.

You disallowed some combinations of unsupported options, but not others,
like FULL, PARALLEL, etc. They should either be supported or
prohibited.

+                                       /* use default values */
+                                       tab.at_params.log_min_duration = 0;

0 isn't the default ?

Maybe VERBOSE should mean to set min_duration=0, otherwise it should use
the default ?

+1

You only handle one rel, but ExecVacuum() has a loop around rels.

+NOTICE: autovacuum of "vactst" has been requested, using the options specified

=> I don't think it's useful to say "using the options specified".

Should autovacuum de-duplicate requests ?
BRIN doesn't do that, but it's intended for append-only tables, so the
issue doesn't really come up.

Easy to do

Could add psql tab-completion.

Is it going to be confusing that the session's GUC variables won't be
transmitted to autovacuum ? For example, the freeze and costing
parameters.

I think we should start with the "how do I want it to behave" parts of
the above and leave spelling and tab completion as final items.

Other questions are whether there should be a limit on number of
background vacuums submitted at any time.
Whether there should be a GUC that specifies the max number of queued tasks.
Do we need a query that shows what items are queued?
etc

Justin, if you wanted to take up the patch from here, I would be more
than happy. You have the knowledge and insight to make this work
right.

We should probably start a new CF patch entry so we can return the
original patch as rejected, then continue with this new idea
separately.

--
Simon Riggs http://www.EnterpriseDB.com/

#32Justin Pryzby
pryzby@telsasoft.com
In reply to: Simon Riggs (#31)
Re: Allow single table VACUUM in transaction block

On Tue, Nov 22, 2022 at 05:16:59PM +0000, Simon Riggs wrote:

Justin, if you wanted to take up the patch from here, I would be more
than happy. You have the knowledge and insight to make this work
right.

I have no particular use for this, so I wouldn't be a good person to
finish or shepherd the patch.

--
Justin