Re: SV: Problem with pg_notify / listen - Mailing list pgsql-bugs
From | Tom Lane |
---|---|
Subject | Re: SV: Problem with pg_notify / listen |
Date | |
Msg-id | 716909.1606498665@sss.pgh.pa.us Whole thread Raw |
Responses |
Re: SV: Problem with pg_notify / listen
|
List | pgsql-bugs |
[ 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
pgsql-bugs by date: