Re: Assertion failing in master, predicate.c

Mark Dilger <hornschnorter(at)gmail(dot)com> writes:
> On 11/22/19 3:25 PM, Tom Lane wrote:
>> An alternative idea is to use some other way of getting a snapshot
>> in asyncQueueReadAllNotifications, one that always gets a current
>> snapshot and doesn't enter predicate.c. But that might have semantic
>> consequences on the timing of notifications. I'm not really sure
>> that anybody's ever thought hard about how async.c ought to act
>> in serializable mode, so this might or might not be a good change.

> The semantics of receiving a notification in serializable mode are
> not clear, unless you just insist on not receiving any. The whole
> point of serializable mode, as I understand it, it to be given the
> impression that all your work happens either before or after other
> transactions' work. It hardly makes sense to receive a notification
> mid transaction informing you of some other transaction having just
> changed something.

Well, you don't: notifications are only sent to the client between
transactions. After sleeping on it I have these thoughts:

* The other two callers of asyncQueueReadAllNotifications have just
started fresh transactions, so they have no issue. Regardless of
the session isolation level, they'll be reading the queue with a
freshly-taken snapsnot.

* The point of calling asyncQueueReadAllNotifications in
Exec_ListenPreCommit is to advance over already-committed queue entries
before we start sending any events to the client. Without this, a
newly-listening client could be sent some very stale events. (Note
that 51004c717 changed this from "somewhat stale" to "very stale".
I had thought briefly about whether we could fix the problem by just
removing this call of asyncQueueReadAllNotifications, but I do not
think people would find that side-effect acceptable.)

* Given that the idea is to ignore already-committed entries, it makes
sense that if LISTEN is called inside a serializable transaction block
then the cutoff ought to be the transaction's snapshot. Otherwise we'd
be dropping notifications from transactions that the calling session
can't have seen the effects of. That defeats the whole point.

* This says that not only is it okay to use a serializable snapshot
as the reference, but we *should* do so; that is, it's actually wrong
to use GetLatestSnapshot here, we should use GetTransactionSnapshot.
There's not going to be any real difference in read-committed mode,
but in repeatable-read or serializable mode we are risking dropping
events that it'd be better to send to the client.

I'm disinclined to make such a change in the back branches, but it'd
be reasonable to do so in HEAD.

Meanwhile, as far as fixing the assertion failure goes, I don't see
any alternative except to rearrange the order of operations during
commit.

regards, tom lane