Re: Speed up transaction completion faster after many relations are accessed in a transaction

On 20/01/2026 19:46, Andres Freund wrote:
> On 2026-01-20 10:43:09 +0200, Heikki Linnakangas wrote:
>> I was able to reproduce and debug that. The patch made the assumption that
>> when a process is about to exit, in ShutdownPostgres(), it can only have
>> session-level locks left, after we have done AbortOutOfAnyTransaction().
>> That assumption did not hold, because if proc_exit() is called while we're
>> waiting on a lock, the lock is not yet registered with the resource owner
>> (or if it's a session lock, it's not yet added to the 'session_locks' list).
>> Therefore, when ShutdownPostgres() called AbortOutOfAnyTransaction() and
>> LockReleaseSession(USER_LOCKMETHOD), it would not clean up the lock objects
>> for the lock we were waiting on. That's pretty straightforward to fix by
>> also calling LockErrorCleanup() from ShutdownPostgres(). Alternatively,
>> perhaps we should register the lock with the resource owner earlier.
>
> Hm. There is a LockErrorCleanup() in AbortTransaction(), why does that not
> suffice? If we're in the middle of a lock acquisition, we should be inside a
> transaction, and thus the AbortOutOfAnyTransaction() should call
> AbortTransaction(), which in turn should call LockErrorCleanup().

Session locks can be acquired outside a transaction. In the failing
test, it's a parallel apply worker blocked on a pg_lock_transaction() call.

> The coding around this does seem pretty grotty today :(. Why do we have
> LockErrorCleanup() strewn across ProcessInterrupts(), rather than solve the
> need to do so more centrally?

Yeah, it's not pretty...

> It's not even complete within ProcessInterrupts(), from what I can
> tell - don't we e.g. need to do LockErrorCleanup() for the FATAL due
> to TransactionTimeout or ClientConnectionLost?
I'm not convinced that any of those calls in ProcessInterrupts() are
needed. If the process is exiting with FATAL or proc_exit, the
ShutdownPostgres exit callback should release the lock. (On 'master',
thanks to LockReleaseAll(), and with this patch, thanks to the
LockErrorCleanup() call). And on ERROR, transaction abort should
likewise clean it up. Is it somehow important to clean up the awaited
lock earlier, before the error processing starts?

>> v10-0001-Assert-that-all-fastpath-locks-are-released-befo.patch adds a few
>> assertions, which are not probably good to have even without the rest of the
>> patches.
>
> Seems reasonable to me. Don't see a reason to wait applying this one.

Thanks, will commit.
- Heikki