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.
> @@ -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.
--
Heikki Linnakangas
Neon (https://neon.tech)
