On Thu, Aug 09, 2018 at 02:29:54AM -0700, Andres Freund wrote:
> On 2018-08-09 09:00:29 +0200, Michael Paquier wrote:
>> 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."
>
> That's a caveat, not a documentation of the memory ordering /
> concurrency.
I think that I am going to add this note anyway in the new routine.
> You somewhere need a comment to the effect of "We are
> guaranteed to see a recent enough value of ->tempNamespace because
> anybody creating a temporary table acquires a lock - serving as a memory
> barrier - during the creation of said table, after assigning the
> tempNamespace variable. At the same time, before considering dropping a
> temporary table as orphaned, we acquire a lock and recheck tempNamespace
> afterwards". Note that I'm not explaining the concrete model you have
> here, but the way you could explain a theoretical one.
Okay, here is an idea... When assigning the value I have now that:
+ /*
+ * 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.
+ */
I actually have tried to mention what you are willing to see in the
comments with the last sentence. So that is awkward :)
I would propose to reword the last sentence of the patch as follows
then:
"Even if it is not atomic, 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."
When resetting the value on abort I have that:
+ /*
+ * 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.
+ */
Hence I would propose the following wording for this part:
"Reset the temporary namespace flag in MyProc. We assume that this
operation is atomic, however it would be fine even if not atomic as the
temporary table which created this namespace is still locked until this
transaction aborts so it would not be visible yet."
Better ideas are of course welcome.
--
Michael