On Thu, Jul 26, 2018 at 07:05:11PM +0900, Kyotaro HORIGUCHI wrote: > At Wed, 18 Jul 2018 07:34:10 +0000, "Tsunakawa, Takayuki" > <tsunakawa.ta...@jp.fujitsu.com> wrote in > <0A3221C70F24FB45833433255569204D1FA538FD@G01JPEXMBYT05> >> From: Michael Paquier [mailto:mich...@paquier.xyz] >>> + /* Does the backend own the temp schema? */ >>> + if (proc->tempNamespaceId != namespaceID) >>> + return false; >>> I have a very hard time believing that this is safe lock-less, and a spin >>> lock would be enough it seems. >> >> The lwlock in BackendIdGetProc() flushes the CPU cache so that >> proc->tempNamespaceId reflects the latest value. Or, do you mean >> another spinlock elsewhere? > > It seems to be allowed that the series of checks on *proc results > in false-positive, which is the safer side for the usage, even it > is not atomically updated. Actually ->databaseId is written > without taking a lock.
Well, from postinit.c there are a couple of assumptions in this case, so it is neither white nor black: /* * Now we can mark our PGPROC entry with the database ID. * * We assume this is an atomic store so no lock is needed; though actually * things would work fine even if it weren't atomic. Anyone searching the * ProcArray for this database's ID should hold the database lock, so they * would not be executing concurrently with this store. A process looking * for another database's ID could in theory see a chance match if it read * a partially-updated databaseId value; but as long as all such searches * wait and retry, as in CountOtherDBBackends(), they will certainly see * the correct value on their next try. */ MyProc->databaseId = MyDatabaseId; Anyway, I have spent some time on this patch, and the thing is doing a rather bad job about why it is fine to assume that the update can happen lock-less, and the central part of it seems to be that autovacuum cannot see the temporary schema created, and any objects on it when it scans pg_class until it gets committed, and the tracking field is updated in PGPROC before the commit. > backend_uses_temp_namespace is taking two parameters but the > first one is always derived from the second. backendID doesn't > seem to be needed outside so just one parameter namespaceID is > needed. Yes, that reduces the number of calls to GetTempNamespaceBackendId. autovacuum.c is a pretty bad place for stuff as namespace.c holds all the logic related to temporary tablespaces, so I renamed the routine to isTempNamespaceInUse and moved it there. The patch I have now is attached. I have not been able to test it much, particularly with orphaned temp tables and autovacuum, which is something I'll try to look at this week end or perhaps at the beginning of next week, heading toward a commit if I am fine enough with it. Please feel free to look at it for the time being. Thanks, -- Michael
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 0f67a122ed..0630032148 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,42 @@ isOtherTempNamespace(Oid namespaceId)
return isAnyTempNamespace(namespaceId);
}
+/*
+ * isTempNamespaceInUse - is the given namespace owned and actively used
+ * by a backend?
+ *
+ * Note: this is used by autovacuum to detect the presence of orphaned
+ * temporary tables, based on the active namespace tracked by PGPROC entries.
+ */
+bool
+isTempNamespaceInUse(Oid namespaceId)
+{
+ PGPROC *proc;
+ int backendId;
+
+ 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 database being vacuumed? */
+ 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 +3929,15 @@ InitTempTableNamespace(void)
myTempNamespace = namespaceId;
myTempToastNamespace = toastspaceId;
+ /*
+ * Mark the proc entry as owning this namespace which autovacuum uses to
+ * decide if a temporary file entry can be marked as orphaned 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 by autovacuum when
+ * scanning pg_class.
+ */
+ MyProc->tempNamespaceId = namespaceId;
+
/* It should not be done already. */
AssertState(myTempNamespaceSubID == InvalidSubTransactionId);
myTempNamespaceSubID = GetCurrentSubTransactionId();
@@ -3923,6 +3968,14 @@ AtEOXact_Namespace(bool isCommit, bool parallel)
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
baseSearchPathValid = false; /* need to rebuild list */
+
+ /*
+ * Reset the temporary namespace flag in the proc entry. The
+ * creation of the temporary namespace has failed for some reason
+ * and cannot be seen by autovacuum as it has not been committed,
+ * hence it is fine to do this in a lock-less fashion.
+ */
+ MyProc->tempNamespaceId = InvalidOid;
}
myTempNamespaceSubID = InvalidSubTransactionId;
}
@@ -3975,6 +4028,14 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
baseSearchPathValid = false; /* need to rebuild list */
+
+ /*
+ * Reset the temporary namespace flag in the proc entry. The
+ * creation of the temporary namespace has failed for some reason
+ * and cannot be seen by autovacuum 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. */
/*
signature.asc
Description: PGP signature
