Re: [PG19] A canceled notify read leaves the NOTIFY queue stuck

shihao zhong <zhong950419(at)gmail(dot)com> writes:
> If a listener fails while taking its snapshot in
> asyncQueueReadAllNotifications(), its isAdvancing flag stays set. After
> that no NOTIFY wakes or advances it, so the queue stops shrinking and
> fills up. This is new in 19, from 282b1cde9de.

Ooops.

> The attached patch clears the flag on abort.

Playing around with this example, it avoids the queue-filling problem,
but there's still an undesirable aspect: the N process has sent the L
process a NOTIFY, but the L process does not see that, at least not
till another NOTIFY is sent (and then it reports both at once).
That's because the L process already cleared notifyInterruptPending,
so it won't re-enter ProcessIncomingNotify. This is measurably worse
than pre-v19: before, L would get the notify message when it next did
something, even without another NOTIFY. (I'm not immediately sure
why that changed.) The old behavior is still not great though;
ideally it'd report the notify right after finishing the aborted
COMMIT.

I tried to fix this by re-setting notifyInterruptPending in the new
block of code, but that's totally disastrous in the given example.
Having gotten out of the transaction, we immediately try to do
ProcessIncomingNotify again, and again we block in
GetTransactionSnapshot(). The user-visible effect is that you cannot
cancel the L process' COMMIT, even with multiple control-Cs, until the
W process commits or aborts. There might be other reasons why doing
this would turn into an endless failure loop, so on the whole that's
probably a bad idea. But I'd like to understand why v18 didn't
require receipt of a new NOTIFY before reporting the pending one.

Also, this business about GetTransactionSnapshot possibly blocking is
something that nobody foresaw when the async.c code was written.
I wonder if we should modify async.c to avoid it. Do we really need a
serializable snapshot for reading the notify queue? If we do, can we
acquire it before making any important state changes?

Anyway, the proposed patch is clearly an improvement so I'll go
ahead and push it. But we seem to have more to do here.

regards, tom lane