Hi Andres,
On Thu, Jan 12, 2023 at 11:31 AM Andres Freund <andres@anarazel.de> wrote:
> I know that WaitForLockers() is an existing function :). I'm not sure it's
> entirely suitable for your use case. So I mainly wanted to point out that if
> you end up writing a separate version of it, you still need to integrate with
> the deadlock detection.
I see. What about it seems potentially unsuitable?
> On 2023-01-11 23:03:30 -0800, Will Mortensen wrote:
> > To my very limited understanding, from looking at the existing callers and
> > the implementation of LOCK, that would look something like this (assuming
> > we're in a SQL command like LOCK and calling unmodified WaitForLockers()
> > with a single table):
> >
> > 1. Call something like RangeVarGetRelidExtended() with AccessShareLock
> > to ensure the table is not dropped and obtain the table oid
> >
> > 2. Use SET_LOCKTAG_RELATION() to construct the lock tag from the oid
> >
> > 3. Call WaitForLockers(), which internally calls GetLockConflicts() and
> > VirtualXactLock(). These certainly take plenty of locks of various types,
> > and will likely sleep in LockAcquire() waiting for transactions to finish,
> > but there don't seem to be any unusual pre/postconditions, nor do we
> > hold any unusual locks already.
>
> I suspect that keeping the AccessShareLock while doing the WaitForLockers() is
> likely to increase the deadlock risk noticeably. I think for the use case you
> might get away with resolving the relation names, building the locktags, and
> then release the lock, before calling WaitForLockers. If somebody drops the
> table or such, you'd presumably still get desired behaviour that way, without
> the increased deaadlock risk.
That makes sense. I agree it seems fine to just return if e.g. the table is
dropped.
FWIW re: deadlocks in general, I probably didn't highlight it well in my
original email, but the existing solution for this use case (as Marco
described in his blog post) is to actually lock the table momentarily.
Marco's blog post uses ShareRowExclusiveLock, but I think ShareLock is
sufficient for us; in any case, that's stronger than the AccessShareLock that
we need to merely wait.
And actually locking the table with e.g. ShareLock seems perhaps *more*
likely to cause deadlocks (and hurts performance), since it not only waits for
existing conflicting lockers (e.g. RowExclusiveLock) as desired, but also
undesirably blocks other transactions from newly acquiring conflicting locks
in the meantime. Hence the motivation for this feature. :-)
I'm sure I may be missing something though. Thanks for all your feedback. :-)
