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

Hi,

On 2026-01-20 10:43:09 +0200, Heikki Linnakangas wrote:
> Returning to an old thread that I just remembered:

Yea, this one would be a nice one to get in at some point...

> On 09/01/2024 08:24, vignesh C wrote:
> > On Thu, 9 Nov 2023 at 21:48, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> > >
> > > On 18/09/2023 07:08, David Rowley wrote:
> > > > On Fri, 15 Sept 2023 at 22:37, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> > > A few quick comments:
> > >
> > > - It would be nice to add a test for the issue that you fixed in patch
> > > v7, i.e. if you prepare a transaction while holding session-level locks.
> > >
> > > - GrantLockLocal() now calls MemoryContextAlloc(), which can fail if you
> > > are out of memory. Is that handled gracefully or is the lock leaked?
>
> The above are still valid comments..

Yea. We probably should move the resource acquisition to earlier in the lock
acquisition process to avoid the complexity to even have to deal with this.

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

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

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

Greetings,

Andres Freund