On Mon, Aug 13, 2018 at 01:53:18PM -0300, Alvaro Herrera wrote: > I do share Andres' concerns on the wording the comment. I would say > something like > > /* > * Reset the temporary namespace flag in MyProc. We assume this to be > * an atomic assignment. > * > * Because this subtransaction is rolling back, the pg_namespace > * row is not visible to anyone else anyway, but that doesn't matter: > * it's not a problem if objects contained in this namespace are removed > * concurrently. > */
> The fact of assignment being atomic and the fact of the pg_namespace row > being visible are separately important. You care about it being atomic > because it means you must not have someone read "16" (0x10) when you > were partway removing the value "65552" (0x10010), thus causing that > someone removing namespace 16. And you care about the visibility of the > pg_namespace row because of whether you're worried about a third party > removing the tables from that namespace or not: since the subxact is > aborting, you are not. I was thinking about adding "Even if it is not atomic" or such at the beginning of the paragraph, but at the end your phrasing sounds better to me. So I have hacked up the attached, which also reworks the comment in InitTempTableNamespace in the same spirit. Thoughts? -- Michael
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 3971346e73..bd8ae0ae4b 100644 --- a/src/backend/catalog/namespace.c +++ b/src/backend/catalog/namespace.c @@ -3938,8 +3938,10 @@ InitTempTableNamespace(void) * 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, * the temporary relation which resulted in the creation of this temporary - * namespace is still locked until the current transaction commits, so it - * would not be accessible yet, acting as a barrier. + * namespace is still locked until the current transaction commits, and + * its pg_namespace row is not visible yet. However it does not matter: + * this flag makes the namespace as being in use, so no objects created + * on it would be removed concurrently. */ MyProc->tempNamespaceId = namespaceId; @@ -3976,10 +3978,12 @@ AtEOXact_Namespace(bool isCommit, bool parallel) /* * Reset the temporary namespace flag in MyProc. We assume that - * this operation is atomic. Even if it is not, the temporary - * table which created this namespace is still locked until this - * transaction aborts so it would not be visible yet, acting as a - * barrier. + * this operation is atomic. + * + * Because this transaction is aborting, the pg_namespace row is + * not visible to anyone else anyway, but that doesn't matter: + * it's not a problem if objects contained in this namespace are + * removed concurrently. */ MyProc->tempNamespaceId = InvalidOid; } @@ -4037,10 +4041,12 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid, /* * Reset the temporary namespace flag in MyProc. We assume that - * this operation is atomic. Even if it is not, the temporary - * table which created this namespace is still locked until this - * transaction aborts so it would not be visible yet, acting as a - * barrier. + * this operation is atomic. + * + * Because this subtransaction is aborting, the pg_namespace row + * is not visible to anyone else anyway, but that doesn't matter: + * it's not a problem if objects contained in this namespace are + * removed concurrently. */ MyProc->tempNamespaceId = InvalidOid; }
signature.asc
Description: PGP signature
