On Wed, 7 Dec 2022 at 22:02, Greg Stark <st...@mit.edu> wrote: > > Seems like this should just be in > > heapam_relation_nontransactional_truncate()?
So here I've done it that way. It is a bit of an unfortunate layering since it means the heapam_handler is doing the catalog change but it does seem inconvenient to pass relfrozenxid etc back up and have the caller make the changes when there are no other changes to make. Also, I'm not sure what changed but maybe there was some other commits in vacuum.c in the meantime. I remember being frustrated previously trying to reuse that code but now it works fine. So I was able to reduce the copy-pasted code significantly. (The tests are probably not worth committing, they're just here for my own testing to be sure it's doing anything) -- greg
From f6f800819c497817c3f2957683fc521abe82c2b7 Mon Sep 17 00:00:00 2001 From: Greg Stark <st...@mit.edu> Date: Thu, 31 Mar 2022 15:49:19 -0400 Subject: [PATCH v8 2/3] Update relfrozenxmin when truncating temp tables Make ON COMMIT DELETE ROWS reset relfrozenxmin and other table stats to the same values used in initial table creation. Otherwise even typical short-lived transactions in long-lived sessions using temporary tables can easily cause them to reach transaction wraparound and autovacuum cannot come to the rescue for temporary tables. Also optimize the relminmxid used for for temporary tables to be our own oldest MultiXactId instead of the globally oldest one. This avoids the expensive calculation of the latter on every transaction commit. This code path is also used by truncation of tables created within the same transaction. --- src/backend/access/heap/heapam_handler.c | 51 +++++++++++++++++++++++- src/backend/access/transam/multixact.c | 15 +++++++ src/backend/catalog/heap.c | 14 ++++++- src/include/access/multixact.h | 1 + 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index ab1bcf3522..b01a25884f 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -33,6 +33,7 @@ #include "catalog/storage.h" #include "catalog/storage_xlog.h" #include "commands/progress.h" +#include "commands/vacuum.h" #include "executor/executor.h" #include "miscadmin.h" #include "pgstat.h" @@ -587,9 +588,18 @@ heapam_relation_set_new_filelocator(Relation rel, * could reuse values from their local cache, so we are careful to * consider all currently running multis. * - * XXX this could be refined further, but is it worth the hassle? + * In the case of temporary tables we can refine this slightly and use a + * our own oldest visible MultiXactId. This is also cheaper to calculate + * which is nice since temporary tables might be getting created often. */ - *minmulti = GetOldestMultiXactId(); + if (persistence == RELPERSISTENCE_TEMP) + { + *minmulti = GetOurOldestMultiXactId(); + } + else + { + *minmulti = GetOldestMultiXactId(); + } srel = RelationCreateStorage(*newrlocator, persistence, true); @@ -618,6 +628,43 @@ heapam_relation_set_new_filelocator(Relation rel, static void heapam_relation_nontransactional_truncate(Relation rel) { + /* This function from vacuum.c does a non-transactional update of the + * catalog entry for this relation. That's ok because these values are + * always safe regardless of whether we commit and in any case this is + * either a temporary table or a filenode created in this transaction so + * this tuple will be irrelevant if we do not commit. It's also important + * to avoid lots of catalog bloat due to temporary tables being truncated + * on every transaction commit. + * + * We set in_outer_transaction=false because that controls whether + * vac_update_relstats updates other columns like relhasindex, + * relhasrules, relhastriggers which is not changing here. This is a bit + * of a hack, perhaps this parameter should change name. + * + * These parameters should stay in sync with + * heapam_relation_set_new_filelocator() above and AddNewRelationTuple() + * in heap.c. In theory this should probably return the relfrozenxid and + * relminmxid and heap_truncate_one_rel() in heap.c should handle + * num_tuples and num_pages but that would be slightly inconvenient and + * require an api change. + */ + + /* Ensure RecentXmin is valid -- it almost certainly is but regression + * tests turned up an unlikely corner case where it might not be */ + if (!FirstSnapshotSet) + (void)GetLatestSnapshot(); + Assert(FirstSnapshotSet); + vac_update_relstats(rel, + 0, /* num_pages */ + -1, /* num_tuples */ + 0, /* num_all_visible_pages */ + true, /* hasindex -- ignored due to in_outer_xact false */ + RecentXmin, /* frozenxid */ + RelationIsPermanent(rel) ? GetOldestMultiXactId() : GetOurOldestMultiXactId(), + NULL, NULL, /* frozenxid_updated, minmxid_updated */ + false /* in_outer_xac (see above) */ + ); + RelationTruncate(rel, 0); } diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index e1191a7564..b06e9ae18d 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -2489,6 +2489,21 @@ ExtendMultiXactMember(MultiXactOffset offset, int nmembers) } } +/* + * GetOurOldestMultiXactId + * + * Expose the oldest MultiXactId possibly seen as live by *this* + * transaction. This is mainly useful for initializing relminmxid on temp + * tables since they can't been modified by other transactions. + */ + +MultiXactId +GetOurOldestMultiXactId(void) +{ + MultiXactIdSetOldestVisible(); + return OldestVisibleMXactId[MyBackendId]; +} + /* * GetOldestMultiXactId * diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index bdd413f01b..8d89530e66 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/multixact.h" #include "access/relation.h" #include "access/table.h" @@ -72,6 +73,7 @@ #include "utils/fmgroids.h" #include "utils/inval.h" #include "utils/lsyscache.h" +#include "utils/snapmgr.h" #include "utils/syscache.h" @@ -972,7 +974,9 @@ AddNewRelationTuple(Relation pg_class_desc, */ new_rel_reltup = new_rel_desc->rd_rel; - /* The relation is empty */ + /* The relation is empty + (These are also in heapam_relation_nontransactional_truncate) + */ new_rel_reltup->relpages = 0; new_rel_reltup->reltuples = -1; new_rel_reltup->relallvisible = 0; @@ -2996,6 +3000,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) diff --git a/src/include/access/multixact.h b/src/include/access/multixact.h index 4cbe17de7b..ca7ebbe3b2 100644 --- a/src/include/access/multixact.h +++ b/src/include/access/multixact.h @@ -139,6 +139,7 @@ extern void MultiXactGetCheckptMulti(bool is_shutdown, MultiXactId *oldestMulti, Oid *oldestMultiDB); extern void CheckPointMultiXact(void); +extern MultiXactId GetOurOldestMultiXactId(void); extern MultiXactId GetOldestMultiXactId(void); extern void TruncateMultiXact(MultiXactId newOldestMulti, Oid newOldestMultiDB); -- 2.38.1
From 3bd4b85f53b7f25e623bfa5a5ac5d5659776094b Mon Sep 17 00:00:00 2001 From: Greg Stark <st...@mit.edu> Date: Thu, 31 Mar 2022 15:48:38 -0400 Subject: [PATCH v8 1/3] 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. Rename the function to avoid an incompatible ABI break. --- src/backend/access/transam/varsup.c | 12 ++++--- src/backend/catalog/namespace.c | 9 +++-- src/backend/postmaster/autovacuum.c | 52 ++++++++++++++++++++++------- src/include/catalog/namespace.h | 4 +-- 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c index 849a7ce9d6..778e94e962 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/namespace.c b/src/backend/catalog/namespace.c index bac0deb6da..27f633328f 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -3269,15 +3269,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) +checkTempNamespaceStatusAndPid(Oid namespaceId, pid_t *pid) { PGPROC *proc; int backendId; @@ -3304,6 +3307,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 0746d80224..07b39976dc 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2067,6 +2067,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) @@ -2074,6 +2076,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 = pgstat_fetch_stat_tabentry_ext(classForm->relisshared, + relid); + + /* 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. @@ -2085,7 +2097,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 = checkTempNamespaceStatusAndPid(classForm->relnamespace, &temp_pid); + if (temp_status == TEMP_NAMESPACE_IDLE) { /* * The table seems to be orphaned -- although it might be that @@ -2096,19 +2109,34 @@ do_autovacuum(void) */ 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(%lu).", + (unsigned long)temp_pid) + )); + } continue; } - /* Fetch reloptions and the pgstat entry for this table */ - relopts = extract_autovac_opts(tuple, pg_class_desc); - tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared, - relid); - - /* 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); @@ -2255,7 +2283,7 @@ do_autovacuum(void) continue; } - if (checkTempNamespaceStatus(classForm->relnamespace) != TEMP_NAMESPACE_IDLE) + if (checkTempNamespaceStatusAndPid(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 2a2a2e6489..77f648d085 100644 --- a/src/include/catalog/namespace.h +++ b/src/include/catalog/namespace.h @@ -39,7 +39,7 @@ typedef struct _FuncCandidateList } *FuncCandidateList; /* - * Result of checkTempNamespaceStatus + * Result of checkTempNamespaceStatusAndPid */ typedef enum TempNamespaceStatus { @@ -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 checkTempNamespaceStatusAndPid(Oid namespaceId, pid_t *pid); extern int GetTempNamespaceBackendId(Oid namespaceId); extern Oid GetTempToastNamespace(void); extern void GetTempNamespaceState(Oid *tempNamespaceId, -- 2.38.1
From 8308746f62a44e9b8fcbf49078ccefee95035a9b Mon Sep 17 00:00:00 2001 From: Greg Stark <st...@mit.edu> Date: Thu, 31 Mar 2022 15:50:02 -0400 Subject: [PATCH v8 3/3] Add test for truncating temp tables advancing relfrozenxid This test depends on other transactions not running at the same time so that relfrozenxid can advance so it has to be moved to its own schedule. --- src/test/regress/expected/temp.out | 37 ++++++++++++++++++++++++++++++ src/test/regress/parallel_schedule | 10 +++++--- src/test/regress/sql/temp.sql | 30 ++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out index a5b3ed34a3..244b868ef7 100644 --- a/src/test/regress/expected/temp.out +++ b/src/test/regress/expected/temp.out @@ -82,6 +82,43 @@ 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 text) ON COMMIT DELETE ROWS; +SELECT reltoastrelid, reltoastrelid::regclass AS relname FROM pg_class where oid = 'temptest'::regclass \gset toast_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset old_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = :toast_reltoastrelid \gset toast_old_ +BEGIN; +INSERT INTO temptest (select repeat('foobar',generate_series(1,1000))); +ANALYZE temptest; -- update relpages, reltuples +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset temp_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = :toast_reltoastrelid \gset toast_temp_ +COMMIT; +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset new_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = :toast_reltoastrelid \gset toast_new_ +-- make sure relpages and reltuple match a newly created table and +-- relfrozenxid is advanced +SELECT :old_relpages <> :temp_relpages AS pages_analyzed, + :old_relpages = :new_relpages AS pages_reset, + :old_reltuples <> :temp_reltuples AS tuples_analyzed, + :old_reltuples = :new_reltuples AS tuples_reset, + :old_relfrozenxid <> :new_relfrozenxid AS frozenxid_advanced; + pages_analyzed | pages_reset | tuples_analyzed | tuples_reset | frozenxid_advanced +----------------+-------------+-----------------+--------------+-------------------- + t | t | t | t | t +(1 row) + +-- The toast table can't be analyzed so relpages and reltuples can't +-- be tested easily make sure frozenxid is advanced +SELECT :toast_old_relfrozenxid <> :toast_new_relfrozenxid AS frozenxid_advanced; + frozenxid_advanced +-------------------- + t +(1 row) + DROP TABLE temptest; -- Test ON COMMIT DROP BEGIN; diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule index 9a139f1e24..b83e3847f2 100644 --- a/src/test/regress/parallel_schedule +++ b/src/test/regress/parallel_schedule @@ -116,10 +116,14 @@ test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath # ---------- # Another group of parallel tests # with depends on create_misc -# 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 this alone because it transiently uses 2 connections and also +# tests relfrozenxid advances when truncating temp tables +# ---------- +test: temp # ---------- # Another group of parallel tests diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql index 424d12b283..5f8647a8aa 100644 --- a/src/test/regress/sql/temp.sql +++ b/src/test/regress/sql/temp.sql @@ -79,6 +79,36 @@ 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 text) ON COMMIT DELETE ROWS; + +SELECT reltoastrelid, reltoastrelid::regclass AS relname FROM pg_class where oid = 'temptest'::regclass \gset toast_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset old_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = :toast_reltoastrelid \gset toast_old_ +BEGIN; +INSERT INTO temptest (select repeat('foobar',generate_series(1,1000))); +ANALYZE temptest; -- update relpages, reltuples +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset temp_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = :toast_reltoastrelid \gset toast_temp_ +COMMIT; +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = 'temptest'::regclass \gset new_ +SELECT relpages, reltuples, relfrozenxid FROM pg_class where oid = :toast_reltoastrelid \gset toast_new_ +-- make sure relpages and reltuple match a newly created table and +-- relfrozenxid is advanced +SELECT :old_relpages <> :temp_relpages AS pages_analyzed, + :old_relpages = :new_relpages AS pages_reset, + :old_reltuples <> :temp_reltuples AS tuples_analyzed, + :old_reltuples = :new_reltuples AS tuples_reset, + :old_relfrozenxid <> :new_relfrozenxid AS frozenxid_advanced; +-- The toast table can't be analyzed so relpages and reltuples can't +-- be tested easily make sure frozenxid is advanced +SELECT :toast_old_relfrozenxid <> :toast_new_relfrozenxid AS frozenxid_advanced; + +DROP TABLE temptest; + -- Test ON COMMIT DROP BEGIN; -- 2.38.1