Thank you for having a look at this. Apologies for not getting back to
you sooner.
On Wed, 5 Jul 2023 at 21:44, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 10/02/2023 04:51, David Rowley wrote:
> > I've attached another set of patches. I do need to spend longer
> > looking at this. I'm mainly attaching these as CI seems to be
> > highlighting a problem that I'm unable to recreate locally and I
> > wanted to see if the attached fixes it.
>
> I like this patch's approach.
>
> > index 296dc82d2ee..edb8b6026e5 100644
> > --- a/src/backend/commands/discard.c
> > +++ b/src/backend/commands/discard.c
> > @@ -71,7 +71,7 @@ DiscardAll(bool isTopLevel)
> > Async_UnlistenAll();
> > - LockReleaseAll(USER_LOCKMETHOD, true);
> > + LockReleaseSession(USER_LOCKMETHOD);
> > ResetPlanCache();
>
> This assumes that there are no transaction-level advisory locks. I think
> that's OK. It took me a while to convince myself of that, though. I
> think we need a high level comment somewhere that explains what
> assumptions we make on which locks can be held in session mode and which
> in transaction mode.
Isn't it ok because DISCARD ALL cannot run inside a transaction block,
so there should be no locks taken apart from possibly session-level
locks?
I've added a call to LockAssertNoneHeld(false) in there.
> > @@ -3224,14 +3206,6 @@ PostPrepare_Locks(TransactionId xid)
> > Assert(lock->nGranted <= lock->nRequested);
> > Assert((proclock->holdMask & ~lock->grantMask) == 0);
> >
> > - /* Ignore it if nothing to release (must be a session lock) */
> > - if (proclock->releaseMask == 0)
> > - continue;
> > -
> > - /* Else we should be releasing all locks */
> > - if (proclock->releaseMask != proclock->holdMask)
> > - elog(PANIC, "we seem to have dropped a bit somewhere");
> > -
> > /*
> > * We cannot simply modify proclock->tag.myProc to reassign
> > * ownership of the lock, because that's part of the hash key and
>
> This looks wrong. If you prepare a transaction that is holding any
> session locks, we will now transfer them to the prepared transaction.
> And its locallock entry will be out of sync. To fix, I think we could
> keep around the hash table that CheckForSessionAndXactLocks() builds,
> and use that here.
Good catch. I've modified the patch to keep the hash table built in
CheckForSessionAndXactLocks around for longer so that we can check for
session locks.
I've attached an updated patch mainly to get CI checking this. I
suspect something is wrong as subscription/015_stream is timing out.
I've not gotten to the bottom of that yet.
David