Re: [PATCH] Improve performance of NOTIFY over many databases (v2)

Martijn van Oosterhout <kleptog(at)gmail(dot)com> writes:
> Here is the rebased second patch.

This throws multiple compiler warnings for me:

async.c: In function 'asyncQueueUnregister':
async.c:1293: warning: unused variable 'advanceTail'
async.c: In function 'asyncQueueAdvanceTail':
async.c:2153: warning: 'slowbackendpid' may be used uninitialized in this function

Also, I don't exactly believe this bit:

+ /* If we are advancing to a new page, remember this so after the
+ * transaction commits we can attempt to advance the tail
+ * pointer, see ProcessCompletedNotifies() */
+ if (QUEUE_POS_OFFSET(QUEUE_HEAD) == 0)
+ backendTryAdvanceTail = true;

It seems unlikely that insertion would stop exactly at a page boundary,
but that seems to be what this is looking for.

But, really ... do we need the backendTryAdvanceTail flag at all?
I'm dubious, because it seems like asyncQueueReadAllNotifications
would have already covered the case if we're listening. If we're
not listening, but we signalled some other listeners, it falls
to them to kick us if we're the slowest backend. If we're not the
slowest backend then doing asyncQueueAdvanceTail isn't useful.

I agree with getting rid of the asyncQueueAdvanceTail call in
asyncQueueUnregister; on reflection doing that there seems pretty unsafe,
because we're not necessarily in a transaction and hence anything that
could possibly error is a bad idea. However, it'd be good to add a
comment explaining that we're not doing that and why it's ok not to.

I'm fairly unimpressed with the "kick a random slow backend" logic.
There can be no point in kicking any but the slowest backend, ie
one whose pointer is exactly the oldest. Since we're already computing
the min pointer in that loop, it would actually take *less* logic inside
the loop to remember the/a backend that had that pointer value, and then
decide afterwards whether it's slow enough to merit a kick.

regards, tom lane