Missing calls to UnlockBuffers() - unify error handling?

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Missing calls to UnlockBuffers() - unify error handling?
Date: 2025-11-06 15:34:58
Message-ID: o7yumhiblmsgqgrhx5auwinw44236cbeaktdv2unhzy5flr3mg@63iyw65bsawl
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

While hacking on a patch to "inline" the lock implementation of content locks
into storage/buffer/, I looked at the callers of UnlockBuffers() and compared
them to the callers of LWLockReleaseAll().

Somewhat disconcertingly, there are more callers of LWLockReleaseAll(),
without - afaict - analysis whether that's right.

E.g., why is it correct that ShutdownAuxiliaryProcess() does not call
UnlockBuffers() anywhere? It's *possible* that that turns out to be safe,
because there's no aux process acquiring cleanup locks, but even if that were
true today, if that ever changed we'd very likely miss that
ShutdownAuxiliaryProcess() would need to be updated.

I guess it's ok that pgarch.c doesn't call UnlockBuffers(), it probably is ok
that walsummarizer.c and method_worker.c don't. For walsender.c I'm a lot less
clear.

Widening the perspective a bit, our current approach for such error-handling /
shutdown functions seems ... not maintainable. In particular we have a
substantial number of top-level sigsetjmp() handlers that have slightly
different recovery codepaths. When adding a new process type, how is one
supposed to even know what function one needs to call?

PostgresMain() has this comment:
/*
* NOTE: if you are tempted to add more code in this if-block,
* consider the high probability that it should be in
* AbortTransaction() instead. The only stuff done directly here
* should be stuff that is guaranteed to apply *only* for outer-level
* error recovery, such as adjusting the FE/BE protocol status.
*/

But a) that clearly hasn't worked, given how long the following block is, and
b) can't really work that well, because we have plenty processes that don't
use transaction, therefore putting cleanup in AbortTransaction() doesn't go
that far.

I don't quite know how it should look like, but it seems pretty obvious that
it should be more unified than it is. My gut feeling is that we should have a
single error recovery function that has a flags argument guiding which
subsystems should be reset and which shouldn't.

Greetings,

Andres Freund

Browse pgsql-hackers by date

  From Date Subject
Next Message Álvaro Herrera 2025-11-06 15:48:31 Re: [PATCH] Add pg_get_subscription_ddl() function
Previous Message Matheus Alcantara 2025-11-06 15:29:14 Re: Include extension path on pg_available_extensions