On Tue, Feb 16, 2010 at 11:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Joachim Wieland <joe@mcknight.de> writes:
>> [ listen/notify patch ]
>
> I found a number of implementation problems having to do with wraparound
> behavior and error recovery. I think they're all fixed, but any
> remaining bugs are probably my fault not yours.
First, thanks for the rework you have done and thanks for applying this.
While I can see a lot of improvements over my version, I think the
logic in asyncQueueProcessPageEntries() needs to be reordered:
+ static bool
+ asyncQueueProcessPageEntries(QueuePosition *current,
+ QueuePosition stop,
+ char *page_buffer)
[...]
+ do
+ {
[...]
+ /*
+ * Advance *current over this message, possibly to the next page.
+ * As noted in the comments for asyncQueueReadAllNotifications, we
+ * must do this before possibly failing while processing the message.
+ */
+ reachedEndOfPage = asyncQueueAdvance(current, qe->length);
[...]
+ if (TransactionIdDidCommit(qe->xid))
[...]
+ else if (TransactionIdDidAbort(qe->xid))
[...]
+ else
+ {
+ /*
+ * The transaction has neither committed nor aborted so far,
+ * so we can't process its message yet. Break out of the loop.
+ */
+ reachedStop = true;
+ break;
In the beginning you are advancing *current but later on you could
find out that the transaction is still running. As the position in the
queue has already advanced you would miss one notification here
because you'd restart directly behind this notification in the
queue...
Joachim
