Re: Schema variables - new implementation for Postgres 15

Hi,

On Mon, Jan 24, 2022 at 12:33:11PM +0100, Pavel Stehule wrote:
> 
> here is updated patch with locking support

Thanks for updating the patch!

While the locking is globally working as intended, I found a few problems with
it.

First, I don't think that acquiring the lock in
get_session_variable_type_typmod_collid() and prepare_variable_for_reading() is
the correct approach.  In transformColumnRef() and transformLetStmt() you first
call IdentifyVariable() to check if the given name is a variable without
locking it and later try to lock the variable if you get a valid Oid.  This is
bug prone as any other backend could drop the variable between the two calls
and you would end up with a cache lookup failure.  I think the lock should be
acquired during IdentifyVariable.  It should probably be optional as one
codepath only needs the information to raise a warning when a variable is
shadowed, so a concurrent drop isn't a problem there.

For prepare_variable_for_reading(), the callers are CopySessionVariable() and
GetSessionVariable().  IIUC those should take care of executor-time locks, but
shouldn't there be some changes for planning, like in AcquirePlannerLocks()?

Some other comments on this part of the patch:

@@ -717,6 +730,9 @@ RemoveSessionVariable(Oid varid)
    Relation    rel;
    HeapTuple   tup;

+   /* Wait, when dropped variable is not used */
+   LockDatabaseObject(VariableRelationId, varid, 0, AccessExclusiveLock);

Why do you explicitly try to acquire an AEL on the variable here?
RemoveObjects / get_object_address should guarantee that this was already done.
You could add an assert LockHeldByMe() here, but no other code path do it so it
would probably waste cycles in assert builds for nothing as it's a fundamental
guarantee.


@@ -747,6 +763,9 @@ RemoveSessionVariable(Oid varid)
     * only when current transaction will be commited.
     */
    register_session_variable_xact_action(varid, ON_COMMIT_RESET);
+
+   /* Release lock */
+   UnlockDatabaseObject(VariableRelationId, varid, 0, AccessExclusiveLock);
 }

Why releasing the lock here?  It will be done at the end of the transaction,
and you certainly don't want other backends to start using this variable in
between.  Also, since you acquired the lock a second time it only decreases the
lock count in the locallock so the lock isn't released anyway.

+ * Returns type, typmod and collid of session variable.
+ *
+ * As a side effect this function acquires AccessShareLock on the
+ * related session variable.
  */
 void
-get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod, Oid *collid)
+get_session_variable_type_typmod_collid(Oid varid, Oid *typid, int32 *typmod, Oid *collid,
+                                       bool lock_held)


lock_held is a bit misleading.  If you keep some similar parameter for this or
another function, maybe name it lock_it or something like that instead?

Also, the comment isn't accurate and should say that an ASL is acquired iff the
variable is true.

+   /*
+    * Acquire a lock on session variable, which we won't release until commit.
+    * This ensure that one backend cannot to drop session variable used by
+    * second backend.
+    */

(and similar comments)
I don't think it's necessary to explain why we acquire locks, we should just
say that the lock will be kept for the whole transaction (and not until a
commit)

And while looking at nearby code, it's probably worthwhile to add an Assert in
create_sessionvars_hashtable() to validate that sessionvars htab is NULL.