Re: SV: Problem with pg_notify / listen

On Fri, Nov 27, 2020 at 05:13:28PM -0500, Tom Lane wrote:
> I wrote:
> > * 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.)

Commit d4031d7 added this comment about necessity:

+ * ... Mutual exclusion must end after any limit
+ * update that would permit other backends to write fresh data into the
+ * segment immediately preceding the one containing cutoffPage. Otherwise,
+ * when the SLRU is quite full, SimpleLruTruncate() might delete that segment
+ * after it has accrued freshly-written data.

... but ...

> After thinking more about that, I'm pretty sure there is a bug there:
> a newly-arriving backend could attempt to scan the queue starting at
> QUEUE_TAIL, concurrently with SimpleLruTruncate removing the page(s)
> it wants to scan. In typical cases no failure will occur because
> Exec_ListenPreCommit could advance its queue pointer to a safe place
> by observing the pointers of other backends. However, if the new
> listener were the only one in its database, we'd have trouble.

... agreed. In general, recycling SLRU space entails three steps that shall
not overlap:

1. Stop reading data in the space, regulated by some "logical tail".
2. Unlink files wholly within the bounds of the space.
3. Start writing data into the space, regulated by some "physical tail" (most
often called a "stop limit").

Commit d4031d7 fixed overlap of (2) and (3). For pg_notify, though, it
introduced overlap of (1) and (2). I've now checked the other SLRUs for
similar problems, but I found nothing urgent:

- pg_xact and pg_subtrans have oldestClogXid as their logical tail and
xidStopLimit as their physical tail. All good.

- pg_multixact/offsets and pg_multixact/members store no logical tails. They
have multiStopLimit and offsetStopLimit as physical tails. Under the right
race condition, pg_get_multixact_members() could get a low-level failure.
That's an undocumented debug function, so letting it fail that way is fine.

- pg_serial has tailXid as its logical tail. It stores no physical tail.
This causes some nearly-impossible bugs, discussed in the comments that
https://postgr.es/m/20201109045319.GA459206@rfd.leadboat.com adds to
predicate.c.

> @@ -286,6 +288,7 @@ static AsyncQueueControl *asyncQueueControl;
>
> #define QUEUE_HEAD (asyncQueueControl->head)
> #define QUEUE_TAIL (asyncQueueControl->tail)
> +#define QUEUE_TAIL_PAGE (asyncQueueControl->tailPage)

I think we don't yet have the right name here, seeing QUEUE_TAIL_PAGE !=
QUEUE_POS_PAGE(QUEUE_TAIL) sounds paradoxical, yet happens regularly. How
about naming it QUEUE_STOP_PAGE?

Otherwise, this looks good. Thanks for diagnosing and fixing this defect.