Re: SV: Problem with pg_notify / listen

[ redirecting to pgsql-bugs ]

Gustavsson Mikael <mikael.gustavsson@smhi.se> writes:
> Clarification, we upgraded from PG 11.9 to PG 11.10.

Hmm ... so the only commit that touched async.c in that interval was
d4031d784.  That changed asyncQueueAdvanceTail() so that, rather
than updating QUEUE_TAIL atomically, it would first compute the new
tail pointer, then release AsyncQueueLock for awhile, then finally
update QUEUE_TAIL with the previously-computed value.

I think that's all right in HEAD and v13, because of their limited
usage of the QUEUE_TAIL pointer.  But I now realize that it's *not*
all right in older branches, because in those branches
asyncQueueAdvanceTail is only called in one of two ways:

1. If ProcessCompletedNotifies realizes that nobody is there to
read the notification it just sent, it'll call asyncQueueAdvanceTail.
In a system actively using notifications, that likely won't ever
happen.

2. Otherwise, asyncQueueAdvanceTail is called from asyncQueueUnregister
or asyncQueueReadAllNotifications, but only if the backend believes
itself to be the hindmost backend, ie it saw that QUEUE_TAIL was
equal to its own queue pointer.

Thus, we now have the possibility for the following race condition:

1. Backend A performs asyncQueueReadAllNotifications and thence
asyncQueueAdvanceTail (hence, A was the hindmost to start with).

2. A computes a new tail pointer, which no longer matches its own
queue pointer, but does match backend B's (ie, B is currently
hindmost).  A now releases AsyncQueueLock, allowing a small
interval in which ...

3. B performs asyncQueueReadAllNotifications.  It sees that its
queue pointer does not match QUEUE_TAIL, so it doesn't need to
call asyncQueueAdvanceTail.  But it does advance its own pointer.

4. A re-acquires AsyncQueueLock and updates QUEUE_TAIL with what
is now a stale value that matches no backend's queue pointer.

5. After this, no execution of asyncQueueUnregister or
asyncQueueReadAllNotifications will call asyncQueueAdvanceTail,
so unless we get to a case where a notify is sent with no
listeners to hear it, the queue will never be emptied.  Ooops.


So the question is what to do about this.  We could dodge the
problem by back-patching 51004c717, but that's a considerably
larger change than I want to stick into the older branches.
More practical possibilities seem to include:

* Change the code back to being atomic, ie go ahead and update
QUEUE_TAIL immediately, and truncate the SLRU only afterward.
Why is it necessary, or even safe, to perform the SLRU truncation
before changing QUEUE_TAIL?  (IOW, I wonder if there isn't a bug
there in HEAD too.)

* Revert d4031d784's effects in async.c in the pre-v13 branches,
on the grounds that the cure was worse than the disease.

* Change asyncQueueAdvanceTail so that it does a whole fresh
computation of the tail pointer after it re-acquires the lock.
I think this is OK; it might mean that we miss an opportunity
to truncate the SLRU, but there'll be another one.

* Find another pathway in which to call asyncQueueAdvanceTail
occasionally.  While the obvious place would be "if we're
about to complain about the queue being full", that's probably
not good enough, because it'd mean that the queue grows quite
large before we recover from the race condition --- and a very
stale tail pointer has bad implications for the cost of
Exec_ListenPreCommit.  I'm not sure about good places otherwise.

Thoughts?

            regards, tom lane