On Fri, Jul 27, 2018 at 08:27:26AM +0000, Tsunakawa, Takayuki wrote: > I don't have a strong opinion, but I wonder which of namespace.c or > autovacuum.c is suitable, because isTempNamespaceInUse is specific to > autovacuum.
I think that there is also a point in allowing other backends to use it as well, so I left it in namespace.c. I have been doing more testing with this patch today. In order to catch code paths where this is triggered I have for example added an infinite loop in do_autovacuum when finding a temp table which exits once a given on-disk file is found. This lets plenty of time to attach autovacuum to a debugger, and play with other sessions in parallel, so I have checked the transactional assumption this patch relied on, and tested down to v10 as that's where removal of orphaned temp relations has been made more aggressive. This patch can also be applied cleanly there. The check on MyDatabaseId does not actually matter much as the same temporary namespace OID would get reused only after an OID wraparound with a backend using a different database but I left it anyway as that's more consistent to me to check for database and namespace, and added a sanity check to make sure that the routine gets called only by a process connected to a database. I am going to be out of town for a couple of days, and the next minor release planned is close by, so I am not pushing that today, but I'd like to do so after the next minor release if there are no objections. Attached is a patch with a proposal of commit message. -- Michael
From 503868a13ff3c9cf41a3f3295977bd7c41f4ab6f Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Mon, 30 Jul 2018 16:54:27 +0900 Subject: [PATCH] Make autovacuum more aggressive to remove orphaned temp tables Commitdafa084, added in 10, made the removal of temporary orphaned tables more aggressive. This commit makes an extra step in the aggressiveness by adding a flag in each backend's MyProc which tracks down any 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 got aborted. This flag can be updated in a lock-less fashion, as autovacuum workers would scan pg_class without seeing the temporary namespace or any temporary relations on the transaction creating it gets committed. The base idea of this patch comes from Robert Haas. Author: Tsunakawa Takayuki Reviewed-by: Michael Paquier, Kyotaro Horiguchi Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F8A4DC6@G01JPEXMBYT05 Backpatch: 10- --- src/backend/catalog/namespace.c | 65 ++++++++++++++++++++++++++++- src/backend/postmaster/autovacuum.c | 19 ++++----- src/backend/storage/lmgr/proc.c | 2 + src/include/catalog/namespace.h | 1 + src/include/storage/proc.h | 3 ++ 5 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 0f67a122ed..b0fbb0b010 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,14 @@ 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. Even if this is an + * atomic operation, this can be safely done lock-less as no temporary + * relations associated to it can be seen yet if scanning pg_class. + */ + MyProc->tempNamespaceId = namespaceId; + /* It should not be done already. */ AssertState(myTempNamespaceSubID == InvalidSubTransactionId); myTempNamespaceSubID = GetCurrentSubTransactionId(); @@ -3923,6 +3970,14 @@ 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 cannot + * be seen by other processes as it has not been committed yet, + * hence it is fine to do this in a lock-less fashion. + */ + MyProc->tempNamespaceId = InvalidOid; } myTempNamespaceSubID = InvalidSubTransactionId; } @@ -3975,6 +4030,14 @@ 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 cannot + * be seen by other processes as it has not been committed, hence + * it is fine to do this in a lock-less fashion. + */ + MyProc->tempNamespaceId = InvalidOid; } } diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 1d9cfc63d2..9f45ffcea3 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 @@ -2257,10 +2254,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
