Re: Missing calls to UnlockBuffers() - unify error handling?

Hi Andres,

> 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.
>
>
+1 for the idea. I am interested in writing a patch for the same if you
would like.

As you mentioned, these code blocks under the if
(sigsetjmp(local_sigjmp_buf, 1) != 0)
statement have a very similar set of function calls, depending on the
process type—whether
it's an auxiliary process, background process, or client backend.
There are also similarities across these types; for instance, all of them
register a ProcKill
callback as an on_shmem_exit callback..

Currently, we perform LWLockReleaseAll() at the before_shmem_exit stage,
such as in the ShutdownAuxiliaryProcess() callback for auxiliary processes
and
ShutdownPostgres() for client backends.
However, there are some inconsistencies:
1. The client backend does not call LWLockReleaseAll() until ProcKill(),
if it is not
in a transaction.
2. The background processes and AutovacuumLauncher do not register a
before_shmem_callback
for calling LWLockReleaseAll() but may invoke it directly within the
sigsetjmp() blocks.
3. Some auxiliary processes also call LWLockReleaseAll() early in the
sigsetjmp() block.

> Seems we should just reorder the sequence in ProcKill() to first call
> LWLockReleaseAll()

It would be too late to call it in ProcKill, since dsm_backend_shutdown()
would
detach segments containing the LWLocks before this. Using a
before_shmem_exit callback or
handling it in the initial steps of sigsetjmp might be more suitable.

Thank you,
Rahila Syed