Re: Proposed patch for LISTEN/NOTIFY race condition

This patch responds to the problem reported by Laurent Birtz here:
http://archives.postgresql.org/pgsql-bugs/2008-03/msg00094.php
The difficulty is that LISTEN modifies pg_listener and then releases
lock early, so that there is an interval where a concurrent NOTIFY
doesn't see the pg_listener entry. In simple cases this causes no
problem, but given just the right interleaving of execution it is
possible for a listener to miss a notification that it "should have"
gotten. The proposed fix moves all updating of pg_listener to end
of transaction, making it reasonable to hold the exclusive lock on
pg_listener until commit. (The simpler fix of just not releasing
lock during LISTEN seems to me too prone to result in deadlocks,
since applications might try to touch random other tables in that
same transaction.)

The patch is fairly bulky but it is actually an extremely
straightforward refactoring of the existing code, plus a nearly exact
copy-and-paste of the existing logic that handles the pending-NOTIFY
list to construct code that handles a pending-LISTEN-and-UNLISTEN list.
I had thought there would be some complexity in managing the
pending-actions list, but as long as we don't try to be smart about
folding out redundant or conflicting commands, it really is trivial.
We just apply the same sequence of table updates that we would have
anyway, only a bit later in the transaction.

I took the opportunity to clean up one or two bits of ancient crufty
code, notably changing some volatile interrupt-service flag variables
from "int" to "sig_atomic_t" and using proper GetDatum macros instead
of casts.

The patch disallows LISTEN/NOTIFY in a prepared transaction, and there
is also a side effect on whether a transaction can see entries in
pg_listener for its own uncommitted LISTEN commands. I think fixing
the race condition is sufficient justification for these potential small
incompatibilities ... anyone think differently?

This patch will apply against HEAD and 8.3, but not cleanly against
prior versions. Since this code hasn't changed materially (except for
additions of subtransaction and 2PC support) for a long time,
back-patching should be straightforward, but I haven't actually done
that yet.

Comments?

regards, tom lane

I wrote:
> The patch disallows LISTEN/NOTIFY in a prepared transaction,

I meant LISTEN/UNLISTEN, of course. See pghackers discussion here:
http://archives.postgresql.org/pgsql-hackers/2008-03/msg00290.php

regards, tom lane

Tom Lane wrote:

> The patch disallows LISTEN/NOTIFY in a prepared transaction, and there
> is also a side effect on whether a transaction can see entries in
> pg_listener for its own uncommitted LISTEN commands.

I wonder if it would've been easier to make NOTIFY use SnapshotDirty to
examine pg_listener, thus causing it to see the uncommitted rows. Would
that work at all?

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> The patch disallows LISTEN/NOTIFY in a prepared transaction, and there
>> is also a side effect on whether a transaction can see entries in
>> pg_listener for its own uncommitted LISTEN commands.

> I wonder if it would've been easier to make NOTIFY use SnapshotDirty to
> examine pg_listener, thus causing it to see the uncommitted rows. Would
> that work at all?

I don't think so. NOTIFY has to update the rows, not only see them,
and you can't update someone else's uncommitted row. In any case we'd
be venturing into uncharted territory, which isn't something I'd want
to backpatch. The existing patch is just altering the timing of the
same actions we take already, so it seems a lot safer. IMHO anyway.

regards, tom lane