On Thu, Aug 09, 2018 at 08:32:32AM +0530, Andres Freund wrote: > My point is that the documentation isn't sufficient. Not that there's an > active problem.
OK. Attached is the latest version if the patch I have that I was preparing for commit. On top of isTempNamespaceInUse I have this note: + * 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. Would you be fine if I add an extra note like what's in BackendIdGetProc? Say "The result may be out of date quickly, so the caller must be careful how to handle this information." -- Michael
From 1e9ba9fa9b172bda1ea54b1f3be1b930973ff45f Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Wed, 8 Aug 2018 19:45:31 +0200 Subject: [PATCH] Make autovacuum more aggressive to remove orphaned temp tables Commit dafa084, added in 10, made the removal of temporary orphaned tables more aggressive. This commit makes an extra step into the aggressiveness by adding a flag in each backend's MyProc which tracks down any temporary namespace currently in use. The flag is set when the namespace gets created and can be reset if the temporary namespace has been created in a transaction or sub-transaction which is aborted. The flag value assignment is assumed to be atomic, so this can be done in a lock-less fashion like other flags already present in PGPROC like databaseId or backendId. This new flag gets used by autovacuum to discard more aggressively orphaned tables by additionally checking for the database a backend is connected to as well as its temporary namespace in-use, removing orphaned temporary relations even if a backend reuses the same slot as one which created temporary relations in the past. Note that autovacuum workers would scan pg_class for temporary orphaned relations without seeing the new temporary namespace or any temporary relations until the transaction creating those gets committed, so that would be fine also if the assignment is non-atomic. The base idea of this patch comes from Robert Haas, has been written in its first version by Tsunakawa Takayuki, and heavily reviewed by me. Author: Tsunakawa Takayuki Reviewed-by: Michael Paquier, Kyotaro Horiguchi Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F8A4DC6@G01JPEXMBYT05 Backpatch: 11-, as PGPROC gains a new flag and we don't want silent ABI breakages on already released versions. --- src/backend/access/transam/twophase.c | 1 + src/backend/catalog/namespace.c | 69 ++++++++++++++++++++++++++- src/backend/postmaster/autovacuum.c | 20 +++----- src/backend/storage/lmgr/proc.c | 2 + src/include/catalog/namespace.h | 1 + src/include/storage/proc.h | 3 ++ 6 files changed, 82 insertions(+), 14 deletions(-) diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c index 0ae0722794..4aff6cf7f2 100644 --- a/src/backend/access/transam/twophase.c +++ b/src/backend/access/transam/twophase.c @@ -471,6 +471,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid, const char *gid, proc->backendId = InvalidBackendId; proc->databaseId = databaseid; proc->roleId = owner; + proc->tempNamespaceId = InvalidOid; proc->isBackgroundWorker = false; proc->lwWaiting = false; proc->lwWaitMode = 0; diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 0f67a122ed..19400e7d76 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -47,7 +47,7 @@ #include "parser/parse_func.h" #include "storage/ipc.h" #include "storage/lmgr.h" -#include "storage/sinval.h" +#include "storage/sinvaladt.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/catcache.h" @@ -3204,6 +3204,45 @@ isOtherTempNamespace(Oid namespaceId) return isAnyTempNamespace(namespaceId); } +/* + * isTempNamespaceInUse - is the given namespace owned and actively used + * by a backend? + * + * 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. + */ +bool +isTempNamespaceInUse(Oid namespaceId) +{ + PGPROC *proc; + int backendId; + + Assert(OidIsValid(MyDatabaseId)); + + backendId = GetTempNamespaceBackendId(namespaceId); + + if (backendId == InvalidBackendId || + backendId == MyBackendId) + return false; + + /* Is the backend alive? */ + proc = BackendIdGetProc(backendId); + if (proc == NULL) + return false; + + /* Is the backend connected to the same database we are looking at? */ + if (proc->databaseId != MyDatabaseId) + return false; + + /* Does the backend own the temporary namespace? */ + if (proc->tempNamespaceId != namespaceId) + return false; + + /* all good to go */ + return true; +} + /* * GetTempNamespaceBackendId - if the given namespace is a temporary-table * namespace (either my own, or another backend's), return the BackendId @@ -3893,6 +3932,16 @@ InitTempTableNamespace(void) myTempNamespace = namespaceId; myTempToastNamespace = toastspaceId; + /* + * Mark MyProc as owning this namespace which other processes can use to + * decide if a temporary namespace is in use or not. We assume that + * assignment of namespaceId is an atomic operation. Even if it is not, + * there is no visible temporary relations associated to it and the + * temporary namespace creation is not committed yet, so none of its + * contents should be seen yet if scanning pg_class or pg_namespace. + */ + MyProc->tempNamespaceId = namespaceId; + /* It should not be done already. */ AssertState(myTempNamespaceSubID == InvalidSubTransactionId); myTempNamespaceSubID = GetCurrentSubTransactionId(); @@ -3923,6 +3972,15 @@ AtEOXact_Namespace(bool isCommit, bool parallel) myTempNamespace = InvalidOid; myTempToastNamespace = InvalidOid; baseSearchPathValid = false; /* need to rebuild list */ + + /* + * Reset the temporary namespace flag in MyProc. The creation of + * the temporary namespace has failed for some reason and should + * not be seen by other processes as it has not been committed + * yet, hence this would be fine even if not atomic, still we + * assume that it is an atomic assignment. + */ + MyProc->tempNamespaceId = InvalidOid; } myTempNamespaceSubID = InvalidSubTransactionId; } @@ -3975,6 +4033,15 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid, myTempNamespace = InvalidOid; myTempToastNamespace = InvalidOid; baseSearchPathValid = false; /* need to rebuild list */ + + /* + * Reset the temporary namespace flag in MyProc. The creation of + * the temporary namespace has failed for some reason and should + * not be seen by other processes as it has not been committed + * yet, hence this would be fine even if not atomic, still we + * assume that it is an atomic assignment. + */ + MyProc->tempNamespaceId = InvalidOid; } } diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 1d9cfc63d2..978089575b 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -2080,14 +2080,11 @@ do_autovacuum(void) */ if (classForm->relpersistence == RELPERSISTENCE_TEMP) { - int backendID; - - backendID = GetTempNamespaceBackendId(classForm->relnamespace); - - /* We just ignore it if the owning backend is still active */ - if (backendID != InvalidBackendId && - (backendID == MyBackendId || - BackendIdGetProc(backendID) == NULL)) + /* + * We just ignore it if the owning backend is still active and + * using the temporary schema. + */ + if (!isTempNamespaceInUse(classForm->relnamespace)) { /* * The table seems to be orphaned -- although it might be that @@ -2215,7 +2212,6 @@ do_autovacuum(void) { Oid relid = lfirst_oid(cell); Form_pg_class classForm; - int backendID; ObjectAddress object; /* @@ -2257,10 +2253,8 @@ do_autovacuum(void) UnlockRelationOid(relid, AccessExclusiveLock); continue; } - backendID = GetTempNamespaceBackendId(classForm->relnamespace); - if (!(backendID != InvalidBackendId && - (backendID == MyBackendId || - BackendIdGetProc(backendID) == NULL))) + + if (isTempNamespaceInUse(classForm->relnamespace)) { UnlockRelationOid(relid, AccessExclusiveLock); continue; diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index 6f30e082b2..6f9aaa52fa 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -371,6 +371,7 @@ InitProcess(void) MyProc->backendId = InvalidBackendId; MyProc->databaseId = InvalidOid; MyProc->roleId = InvalidOid; + MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; MyPgXact->delayChkpt = false; MyPgXact->vacuumFlags = 0; @@ -552,6 +553,7 @@ InitAuxiliaryProcess(void) MyProc->backendId = InvalidBackendId; MyProc->databaseId = InvalidOid; MyProc->roleId = InvalidOid; + MyProc->tempNamespaceId = InvalidOid; MyProc->isBackgroundWorker = IsBackgroundWorker; MyPgXact->delayChkpt = false; MyPgXact->vacuumFlags = 0; diff --git a/src/include/catalog/namespace.h b/src/include/catalog/namespace.h index 7991de5e21..0e202372d5 100644 --- a/src/include/catalog/namespace.h +++ b/src/include/catalog/namespace.h @@ -137,6 +137,7 @@ extern bool isTempToastNamespace(Oid namespaceId); extern bool isTempOrTempToastNamespace(Oid namespaceId); extern bool isAnyTempNamespace(Oid namespaceId); extern bool isOtherTempNamespace(Oid namespaceId); +extern bool isTempNamespaceInUse(Oid namespaceId); extern int GetTempNamespaceBackendId(Oid namespaceId); extern Oid GetTempToastNamespace(void); extern void GetTempNamespaceState(Oid *tempNamespaceId, diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 5c19a61dcf..cb613c8076 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -114,6 +114,9 @@ struct PGPROC Oid databaseId; /* OID of database this backend is using */ Oid roleId; /* OID of role using this backend */ + Oid tempNamespaceId; /* OID of temp schema this backend is + * using */ + bool isBackgroundWorker; /* true if background worker. */ /* -- 2.18.0
signature.asc
Description: PGP signature
