Here's an updated patch. I added some warning messages to autovacuum.
One thing I learned trying to debug this situation in production is
that it's nigh impossible to find the pid of the session using a
temporary schema. The number in the schema refers to the backendId in
the sinval stuff for which there's no external way to look up the
corresponding pid. It would have been very helpful if autovacuum had
just told me which backend pid to kill.
I still have the regression test in the patch and as before I think
it's probably not worth committing. I'm leaning to reverting that
section of the patch before comitting.
Incidentally there's still a hole here where a new session can attach
to an existing temporary schema where a table was never truncated due
to a session dieing abnormally. That new session could be a long-lived
session but never use the temporary schema causing the old table to
just sit there. Autovacuum has no way to tell it's not actually in
use. I tend to think the optimization to defer cleaning the temporary
schema until it's used might not really be an optimization. It still
needs to be cleaned someday so it's just moving the work around. Just
removing that optimization might be the easiest way to close this
hole. The only alternative I see is adding a flag to PROC or somewhere
where autovacuum can see if the backend has actually initialized the
temporary schema yet or not.
From 1315daf80e668b03cf2aab04106fe53ad606c9d0 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@intelerad.com>
Date: Tue, 12 Oct 2021 17:17:51 -0400
Subject: [PATCH v2] Update relfrozenxmin when truncating temp tables
Make ON COMMIT DELETE ROWS reset relfrozenxmin and other table stats
like normal truncate. Otherwise even typical short-lived transactions
using temporary tables can easily cause them to reach relfrozenxid.
Also add warnings when old temporary tables are found to still be in
use during autovacuum. Long lived sessions using temporary tables are
required to vacuum them themselves.
For the warning to be useful modify checkTempNamespaceStatus to return
the backend pid using it so that we can inform super-user which pid to
terminate. Otherwise it's quite tricky to determine as a user.
---
src/backend/access/transam/varsup.c | 12 ++++++---
src/backend/catalog/heap.c | 53 +++++++++++++++++++++++++++++++++++++
src/backend/catalog/namespace.c | 9 +++++--
src/backend/postmaster/autovacuum.c | 48 ++++++++++++++++++++++++---------
src/include/catalog/namespace.h | 2 +-
src/test/regress/expected/temp.out | 21 +++++++++++++++
src/test/regress/parallel_schedule | 9 ++++---
src/test/regress/sql/temp.sql | 19 +++++++++++++
8 files changed, 151 insertions(+), 22 deletions(-)
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index a6e98e7..84ccd2f 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -129,14 +129,16 @@ GetNewTransactionId(bool isSubXact)
errmsg("database is not accepting commands to avoid wraparound data loss in database \"%s\"",
oldest_datname),
errhint("Stop the postmaster and vacuum that database in single-user mode.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
else
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("database is not accepting commands to avoid wraparound data loss in database with OID %u",
oldest_datoid),
errhint("Stop the postmaster and vacuum that database in single-user mode.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
}
else if (TransactionIdFollowsOrEquals(xid, xidWarnLimit))
{
@@ -149,14 +151,16 @@ GetNewTransactionId(bool isSubXact)
oldest_datname,
xidWrapLimit - xid),
errhint("To avoid a database shutdown, execute a database-wide VACUUM in that database.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
else
ereport(WARNING,
(errmsg("database with OID %u must be vacuumed within %u transactions",
oldest_datoid,
xidWrapLimit - xid),
errhint("To avoid a database shutdown, execute a database-wide VACUUM in that database.\n"
- "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
+ "You might also need to commit or roll back old prepared transactions,\n"
+ "drop temporary tables, or drop stale replication slots.")));
}
/* Re-acquire lock and start over */
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 5898203..80b682f 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -30,6 +30,7 @@
#include "postgres.h"
#include "access/genam.h"
+#include "access/heapam.h"
#include "access/htup_details.h"
#include "access/multixact.h"
#include "access/relation.h"
@@ -3316,6 +3317,48 @@ RelationTruncateIndexes(Relation heapRelation)
}
/*
+ * Reset the relfrozenxid and other stats to the same values used when
+ * creating tables. This is used after non-transactional truncation.
+ *
+ * This reduces the need for long-running programs to vacuum their own
+ * temporary tables (since they're not covered by autovacuum) at least in the
+ * case where they're ON COMMIT DELETE ROWS.
+ *
+ * see also src/backend/commands/vacuum.c vac_update_relstats()
+ * also see AddNewRelationTuple() above
+ */
+
+static void
+ResetVacStats(Relation rel)
+{
+ HeapTuple ctup;
+ Form_pg_class pgcform;
+ Relation classRel;
+
+ /* Fetch a copy of the tuple to scribble on */
+ classRel = table_open(RelationRelationId, RowExclusiveLock);
+ ctup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(RelationGetRelid(rel)));
+ if (!HeapTupleIsValid(ctup))
+ elog(ERROR, "pg_class entry for relid %u vanished during truncation",
+ RelationGetRelid(rel));
+ pgcform = (Form_pg_class) GETSTRUCT(ctup);
+
+ /*
+ * Update relfrozenxid
+ */
+
+ pgcform->relpages = 0;
+ pgcform->reltuples = -1;
+ pgcform->relallvisible = 0;
+ pgcform->relfrozenxid = RecentXmin;
+ pgcform->relminmxid = GetOldestMultiXactId();
+
+ heap_inplace_update(classRel, ctup);
+
+ table_close(classRel, RowExclusiveLock);
+}
+
+/*
* heap_truncate
*
* This routine deletes all data within all the specified relations.
@@ -3323,6 +3366,14 @@ RelationTruncateIndexes(Relation heapRelation)
* This is not transaction-safe! There is another, transaction-safe
* implementation in commands/tablecmds.c. We now use this only for
* ON COMMIT truncation of temporary tables, where it doesn't matter.
+ *
+ * Or whenever a table's relfilenode was created within the same transaction
+ * such as when the table was created or truncated (normally) within this
+ * transaction.
+ *
+ * The correctness of this code depends on the fact that the table creation or
+ * truncation would be rolled back *including* the insert/update to the
+ * pg_class row that we update in place here.
*/
void
heap_truncate(List *relids)
@@ -3379,6 +3430,7 @@ heap_truncate_one_rel(Relation rel)
/* Truncate the underlying relation */
table_relation_nontransactional_truncate(rel);
+ ResetVacStats(rel);
/* If the relation has indexes, truncate the indexes too */
RelationTruncateIndexes(rel);
@@ -3390,6 +3442,7 @@ heap_truncate_one_rel(Relation rel)
Relation toastrel = table_open(toastrelid, AccessExclusiveLock);
table_relation_nontransactional_truncate(toastrel);
+ ResetVacStats(rel);
RelationTruncateIndexes(toastrel);
/* keep the lock... */
table_close(toastrel, NoLock);
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 4de8400..bf59dbd 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3271,15 +3271,18 @@ isOtherTempNamespace(Oid namespaceId)
/*
* checkTempNamespaceStatus - is the given namespace owned and actively used
- * by a backend?
+ * by a backend? Optionally return the pid of the owning backend if there is
+ * one. Returned pid is only meaningful when TEMP_NAMESPACE_IN_USE but note
+ * below about race conditions.
*
* Note: this can be used while scanning relations in pg_class to detect
* orphaned temporary tables or namespaces with a backend connected to a
* given database. The result may be out of date quickly, so the caller
* must be careful how to handle this information.
+ *
*/
TempNamespaceStatus
-checkTempNamespaceStatus(Oid namespaceId)
+checkTempNamespaceStatus(Oid namespaceId, pid_t *pid)
{
PGPROC *proc;
int backendId;
@@ -3306,6 +3309,8 @@ checkTempNamespaceStatus(Oid namespaceId)
return TEMP_NAMESPACE_IDLE;
/* Yup, so namespace is busy */
+ if (pid)
+ *pid = proc->pid;
return TEMP_NAMESPACE_IN_USE;
}
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9633232..1ab0bf1 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2084,6 +2084,8 @@ do_autovacuum(void)
bool dovacuum;
bool doanalyze;
bool wraparound;
+ TempNamespaceStatus temp_status;
+ pid_t temp_pid;
if (classForm->relkind != RELKIND_RELATION &&
classForm->relkind != RELKIND_MATVIEW)
@@ -2091,6 +2093,16 @@ do_autovacuum(void)
relid = classForm->oid;
+ /* Fetch reloptions and the pgstat entry for this table */
+ relopts = extract_autovac_opts(tuple, pg_class_desc);
+ tabentry = get_pgstat_tabentry_relid(relid, classForm->relisshared,
+ shared, dbentry);
+
+ /* Check if it needs vacuum or analyze */
+ relation_needs_vacanalyze(relid, relopts, classForm, tabentry,
+ effective_multixact_freeze_max_age,
+ &dovacuum, &doanalyze, &wraparound);
+
/*
* Check if it is a temp table (presumably, of some other backend's).
* We cannot safely process other backends' temp tables.
@@ -2102,7 +2114,8 @@ do_autovacuum(void)
* using the temporary schema. Also, for safety, ignore it if the
* namespace doesn't exist or isn't a temp namespace after all.
*/
- if (checkTempNamespaceStatus(classForm->relnamespace) == TEMP_NAMESPACE_IDLE)
+ temp_status = checkTempNamespaceStatus(classForm->relnamespace, &temp_pid);
+ if (temp_status == TEMP_NAMESPACE_IDLE)
{
/*
* The table seems to be orphaned -- although it might be that
@@ -2112,20 +2125,31 @@ do_autovacuum(void)
* Remember it so we can try to delete it later.
*/
orphan_oids = lappend_oid(orphan_oids, relid);
+ } else if (temp_status == TEMP_NAMESPACE_NOT_TEMP) {
+ elog(LOG, "autovacuum: found temporary table \"%s.%s.%s\" in non-temporary namespace",
+ get_database_name(MyDatabaseId),
+ get_namespace_name(classForm->relnamespace),
+ NameStr(classForm->relname));
+ } else if (temp_status == TEMP_NAMESPACE_IN_USE && wraparound) {
+ /* The table is not orphaned -- however it seems to be in need
+ * of a wraparound vacuum which we cannot do. Sessions using
+ * long-lived temporary tables need to be responsible for
+ * vacuuming them and failing to do so is endangering the
+ * whole cluster.
+ */
+ ereport(LOG,
+ (errmsg("autovacuum: cannot vacuum temporary table \"%s.%s.%s\" in danger of causing transaction wraparound",
+ get_database_name(MyDatabaseId),
+ get_namespace_name(classForm->relnamespace),
+ NameStr(classForm->relname)),
+ errhint("Long-lived clients must vacuum temporary tables themselves periodically.\n"
+ "As super-user drop this table or terminate this session with pg_terminate_backend(%d).",
+ temp_pid)
+ ));
}
continue;
}
- /* Fetch reloptions and the pgstat entry for this table */
- relopts = extract_autovac_opts(tuple, pg_class_desc);
- tabentry = get_pgstat_tabentry_relid(relid, classForm->relisshared,
- shared, dbentry);
-
- /* Check if it needs vacuum or analyze */
- relation_needs_vacanalyze(relid, relopts, classForm, tabentry,
- effective_multixact_freeze_max_age,
- &dovacuum, &doanalyze, &wraparound);
-
/* Relations that need work are added to table_oids */
if (dovacuum || doanalyze)
table_oids = lappend_oid(table_oids, relid);
@@ -2272,7 +2296,7 @@ do_autovacuum(void)
continue;
}
- if (checkTempNamespaceStatus(classForm->relnamespace) != TEMP_NAMESPACE_IDLE)
+ if (checkTempNamespaceStatus(classForm->relnamespace, NULL) != TEMP_NAMESPACE_IDLE)
{
UnlockRelationOid(relid, AccessExclusiveLock);
continue;
diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h
index b98f284..43f89f8 100644
--- a/src/include/catalog/namespace.h
+++ b/src/include/catalog/namespace.h
@@ -155,7 +155,7 @@ extern bool isTempToastNamespace(Oid namespaceId);
extern bool isTempOrTempToastNamespace(Oid namespaceId);
extern bool isAnyTempNamespace(Oid namespaceId);
extern bool isOtherTempNamespace(Oid namespaceId);
-extern TempNamespaceStatus checkTempNamespaceStatus(Oid namespaceId);
+extern TempNamespaceStatus checkTempNamespaceStatus(Oid namespaceId, pid_t *pid);
extern int GetTempNamespaceBackendId(Oid namespaceId);
extern Oid GetTempToastNamespace(void);
extern void GetTempNamespaceState(Oid *tempNamespaceId,
diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out
index a5b3ed3..1fee552 100644
--- a/src/test/regress/expected/temp.out
+++ b/src/test/regress/expected/temp.out
@@ -83,6 +83,27 @@ SELECT * FROM temptest;
(0 rows)
DROP TABLE temptest;
+-- Test that ON COMMIT DELETE ROWS resets the relfrozenxid when the
+-- table is truncated. This requires this test not be run in parallel
+-- with other tests as concurrent transactions will hold back the
+-- globalxmin
+CREATE TEMP TABLE temptest(col int) ON COMMIT DELETE ROWS;
+SELECT relpages, reltuples, relallvisible, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset old_
+BEGIN;
+INSERT INTO temptest (select generate_series(1,1000));
+ANALYZE temptest; -- update relpages, reltuples
+COMMIT;
+SELECT relpages, reltuples, relallvisible, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset new_
+SELECT :old_relpages = :new_relpages AS pages_reset,
+ :old_reltuples = :new_reltuples AS tuples_reset,
+ :old_relallvisible = :new_relallvisible AS allvisible_reset,
+ :old_relfrozenxid <> :new_relfrozenxid AS frozenxid_advanced;
+ pages_reset | tuples_reset | allvisible_reset | frozenxid_advanced
+-------------+--------------+------------------+--------------------
+ t | t | t | t
+(1 row)
+
+DROP TABLE temptest;
-- Test ON COMMIT DROP
BEGIN;
CREATE TEMP TABLE temptest(col int) ON COMMIT DROP;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 7be8917..f60100f 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -112,10 +112,13 @@ test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath
# ----------
# Another group of parallel tests
-# NB: temp.sql does a reconnect which transiently uses 2 connections,
-# so keep this parallel group to at most 19 tests
# ----------
-test: plancache limit plpgsql copy2 temp domain rangefuncs prepare conversion truncate alter_table sequence polymorphism rowtypes returning largeobject with xml
+test: plancache limit plpgsql copy2 domain rangefuncs prepare conversion truncate alter_table sequence polymorphism rowtypes returning largeobject with xml
+
+# Run temp by itself so it can verify relfrozenxid advances when
+# truncating temp tables (and because it does a reconnect which may
+# transiently use two connections)
+test: temp
# ----------
# Another group of parallel tests
diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql
index 424d12b..5f0c39b 100644
--- a/src/test/regress/sql/temp.sql
+++ b/src/test/regress/sql/temp.sql
@@ -79,6 +79,25 @@ SELECT * FROM temptest;
DROP TABLE temptest;
+-- Test that ON COMMIT DELETE ROWS resets the relfrozenxid when the
+-- table is truncated. This requires this test not be run in parallel
+-- with other tests as concurrent transactions will hold back the
+-- globalxmin
+CREATE TEMP TABLE temptest(col int) ON COMMIT DELETE ROWS;
+
+SELECT relpages, reltuples, relallvisible, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset old_
+BEGIN;
+INSERT INTO temptest (select generate_series(1,1000));
+ANALYZE temptest; -- update relpages, reltuples
+COMMIT;
+SELECT relpages, reltuples, relallvisible, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset new_
+SELECT :old_relpages = :new_relpages AS pages_reset,
+ :old_reltuples = :new_reltuples AS tuples_reset,
+ :old_relallvisible = :new_relallvisible AS allvisible_reset,
+ :old_relfrozenxid <> :new_relfrozenxid AS frozenxid_advanced;
+
+DROP TABLE temptest;
+
-- Test ON COMMIT DROP
BEGIN;
--
1.8.3.1
