Martijn van Oosterhout <klep...@gmail.com> writes:
> On Fri, 13 Sep 2019 at 22:04, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> But, really ... do we need the backendTryAdvanceTail flag at all?
> There are multiple issues here. asyncQueueReadAllNotifications() is
> going to be called by each listener simultaneously, so each listener
> is going to come to the same conclusion. On the other side, there is
> no guarantee we wake up anyone as a result of the NOTIFY, e.g. if
> there are no listeners in the current database. To be sure you try to
> advance the tail, you have to trigger on the sending side. The global
> is there because at the point we are inserting entries we are still in
> a user transaction, potentially holding many table locks (the issue we
> were running into in the first place). By setting
> backendTryAdvanceTail we can move the work to
> ProcessCompletedNotifies() which is after the transaction has
> committed and the locks released.
None of this seems to respond to my point: it looks to me like it would
work fine if you simply dropped the patch's additions in PreCommit_Notify
and ProcessCompletedNotifies, because there is already enough logic to
decide when to call asyncQueueAdvanceTail. In particular, the result from
Signal[MyDB]Backends tells us whether anyone else was awakened, and
ProcessCompletedNotifies already does asyncQueueAdvanceTail if not.
As long as we did awaken someone, the ball's now in their court to
make sure asyncQueueAdvanceTail happens eventually.
There are corner cases where someone else might get signaled but never
do asyncQueueAdvanceTail -- for example, if they're in process of exiting
--- but I think the whole point of this patch is that we don't care too
much if that occasionally fails to happen. If there's a continuing
stream of NOTIFY activity, asyncQueueAdvanceTail will happen often
enough to ensure that the queue storage doesn't bloat unreasonably.
regards, tom lane
