Re: Schema variables - new implementation for Postgres 15

Hi

út 25. 1. 2022 v 6:18 odesílatel Julien Rouhaud <rjuju123(at)gmail(dot)com> napsal:

> 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.
>

I moved lock to IdentifyVariable routine

>
> 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()?
>

done

>
> 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.
>
>
removed

>
> @@ -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.
>
>
removed

+ * 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.
>

removed

>
> + /*
> + * 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)
>

removed

>
> 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.
>

done

attached updated patch

Regards

Pavel