Re: [PATCH] Fix ProcKill lock-group vs procLatch recycle race

On Thu, May 07, 2026 at 04:57:28PM +0500, Andrey Borodin wrote:
> Done so. I still keep Vlad's injection_point_condition.h though.
> It seems useful. But no more Meson\Makefile changes, and new sql stuff lives
> now near removable_cutoff() SQL.

Because you want to have your callbacks in a new regress_prockill.c
while being able to reuse the structures. I don't see why not on
clarity ground. The creation of injection_point_condition.h could be
done in its own commit, independent of the rest, but that's just me
being pedantic about such matters. I think that this should be
backpatched anyway, that could make the introduction of new tests
easier.

> Now we need an expert in ProcKill(). I also will review the patchset
> in more detail, but, perhaps, after pgconf.dev.

I have spent a few hours with my head down looking at this patch,
because the issue is quite bad.

So.. The root idea of the patch is that we need to make sure that
no PGPROC appears on the freelist until its procLatch has been
disowned, preventing any attempts from other backends to grab an entry
that could be thought with a latch already owned, which is what the
PANIC is about. The key to all that is just to make sure that
SwitchBackToLocalLatch() and DisownLatch() happen earlier so as
recycled entries cannot be seen by other backends. And that's true
since 9.6. Oops.

+ * Must be called from a controller session that is not one of the victims.
+ * Using injection_points_attach() from the victim itself would register a
+ * before_shmem_exit(injection_points_cleanup) hook that fires before ProcKill
+ * (which is on_shmem_exit) and would detach the point before the victim ever
+ * reaches it. Calling InjectionPointAttach() directly from a separate
+ * controller session leaves no such hook on the victims.

The cleanup hook is good enough in some cases, but yeah it would not
work here. I am OK to live with the tweak that
prockill_attach_injection_wait relies on (aka my line is that folks
are free to push all the stuff they want in the injection_points
module, just don't pollute the backend APIs and keep them simple).
So, sorry, I guess? :D

That's just to say that I like the amount of hackery you have put here
in terms of lock grouping manipulations. That't nice. I want more of
this.

+# Two backends form a lock group, then are terminated concurrently. The test
+# uses injection points placed inside ProcKill() to pause both victims there
+# and verify that a freshly forked backend can claim the recycled PGPROC slot
+# without hitting "latch already owned by PID ..." PANIC.

This claim is slightly incorrect here. If I undo the fix, place the
two INJECTION_POINT macros and re-run the test, the test does not
produce a PANIC. In order to reproduce a PANIC, it seems that we
would need something more complicated with an extra point after a
DisownLatch(). That is not required to me, still this is misleading
and could be improved, I guess..

FWIW, I was first confused by the addition of push_leader and
push_self as presented in the patch, which are here to control how the
free lists are manipulated so as we do not rely on the "proc" lookup,
which is a reference we kept around because MyProc is reset. This
felt as an unnecessary refactoring piece that aims at simplifying the
way to think through the process, but I happen to like the way it
does, so no objections here in terms on including this kind of
refactoring piece into the fix itself. A huge advantage of the new
code is that it documents clearly which state is expected where,
particularly around the lockGroupLeader manipulations. Or is there a
subtle thing I am missing from the patch that justifies this code
change? I think that we should refactor the patch into two pieces,
for clarity:
- Patch 1 refactors the freelist manipulations and introduces the two
new boolean flags, documenting more precily how the freelists are
manipulated and why things are done this way (leader, self, etc.).
- Patch 2, which is about the earlier DisownLatch(), then becomes a
fix of its own. That's the primary fix we care about, to avoid the
entries to be recycled due to a too-early PGPROC entry marked as
available.

If there is a stricter relationship between patches 1 and 2, please
state so, but from what I can see it is not a good idea to mix two
separate concepts within a single patch. In short, the actual bug fix
is about calling DisownLatch() earlier. And there may be a second
fact hidden in the refactoring proposed, which may appear after doing
the first thing. The commit message seems to states that the second
fact is required after fixing the DisownLatch() issue. If this
statement is true, for me it is an argument in favor of doing the
refactoring around the manipulation of the free lists first, then fix
the actual bug. I don't object to doing the refactoring in the
back-branches if required and if absolutely necessary. I have read
the code, and it is an improvement, but please let's do things cleanly
and in an ordered fashion. If we don't need the refactoring pieces in
the back-branches, let's keep the fix as simple as possible.

Also, I suspect that the test is racy? For one, I strongly suspect
that you are going to need tweaks similar 011_lock_stats.pl in
test_misc where we rely on some query_until() with psql banners, to
make sure that the waits actually happen.. I got the point about
pg_stat_activity not being something we can rely on here. That's
something that would be easier to see on an overloaded machine.
--
Michael