Avoid assertion failure with LISTEN in a serializable transaction.

If LISTEN is the only action in a serializable-mode transaction,
and the session was not previously listening, and the notify queue
is not empty, predicate.c reported an assertion failure.  That
happened because we'd acquire the transaction's initial snapshot
during PreCommit_Notify, which was called *after* predicate.c
expects any such snapshot to have been established.

To fix, just swap the order of the PreCommit_Notify and
PreCommit_CheckForSerializationFailure calls during CommitTransaction.
This will imply holding the notify-insertion lock slightly longer,
but the difference could only be meaningful in serializable mode,
which is an expensive option anyway.

It appears that this is just an assertion failure, with no
consequences in non-assert builds.  A snapshot used only to scan
the notify queue could not have been involved in any serialization
conflicts, so there would be nothing for
PreCommit_CheckForSerializationFailure to do except assign it a
prepareSeqNo and set the SXACT_FLAG_PREPARED flag.  And given no
conflicts, neither of those omissions affect the behavior of
ReleasePredicateLocks.  This admittedly once-over-lightly analysis
is backed up by the lack of field reports of trouble.

Per report from Mark Dilger.  The bug is old, so back-patch to all
supported branches; but the new test case only goes back to 9.6,
for lack of adequate isolationtester infrastructure before that.

Discussion: https://postgr.es/m/3ac7f397-4d5f-be8e-f354-440020675694@gmail.com
Discussion: https://postgr.es/m/13881.1574557302@sss.pgh.pa.us

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8fe38c33c6df225977e84185cb611715fe82197e..5c0d0f2af068eb7ce536870f8075553eba18aacb 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2112,6 +2112,14 @@ CommitTransaction(void)
    /* close large objects before lower-level cleanup */
    AtEOXact_LargeObject(true);
 
+   /*
+    * Insert notifications sent by NOTIFY commands into the queue.  This
+    * should be late in the pre-commit sequence to minimize time spent
+    * holding the notify-insertion lock.  However, this could result in
+    * creating a snapshot, so we must do it before serializable cleanup.
+    */
+   PreCommit_Notify();
+
    /*
     * Mark serializable transaction as complete for predicate locking
     * purposes.  This should be done as late as we can put it and still allow
@@ -2122,13 +2130,6 @@ CommitTransaction(void)
    if (!is_parallel_worker)
        PreCommit_CheckForSerializationFailure();
 
-   /*
-    * Insert notifications sent by NOTIFY commands into the queue.  This
-    * should be late in the pre-commit sequence to minimize time spent
-    * holding the notify-insertion lock.
-    */
-   PreCommit_Notify();
-
    /* Prevent cancel/die interrupt while cleaning up */
    HOLD_INTERRUPTS();
 
@@ -2344,6 +2345,8 @@ PrepareTransaction(void)
    /* close large objects before lower-level cleanup */
    AtEOXact_LargeObject(true);
 
+   /* NOTIFY requires no work at this point */
+
    /*
     * Mark serializable transaction as complete for predicate locking
     * purposes.  This should be done as late as we can put it and still allow
@@ -2351,8 +2354,6 @@ PrepareTransaction(void)
     */
    PreCommit_CheckForSerializationFailure();
 
-   /* NOTIFY will be handled below */
-
    /*
     * Don't allow PREPARE TRANSACTION if we've accessed a temporary table in
     * this transaction.  Having the prepared xact hold locks on another

diff --git a/src/test/isolation/expected/async-notify.out b/src/test/isolation/expected/async-notify.out
index 7ad26b7d8f20e726fdc79904ead04dfeba5cb7d1..79427789b1aeb3af7afef9a4f09e02803c2d8101 100644 (file)
--- a/src/test/isolation/expected/async-notify.out
+++ b/src/test/isolation/expected/async-notify.out
@@ -1,4 +1,4 @@
-Parsed test spec with 2 sessions
+Parsed test spec with 3 sessions
 
 starting permutation: listenc notify1 notify2 notify3 notifyf
 step listenc: LISTEN c1; LISTEN c2;
@@ -83,6 +83,17 @@ listener: NOTIFY "c1" with payload "" from notifier
 listener: NOTIFY "c2" with payload "payload" from notifier
 listener: NOTIFY "c2" with payload "" from notifier
 
+starting permutation: l2listen l2begin notify1 lbegins llisten lcommit l2commit l2stop
+step l2listen: LISTEN c1;
+step l2begin: BEGIN;
+step notify1: NOTIFY c1;
+step lbegins: BEGIN ISOLATION LEVEL SERIALIZABLE;
+step llisten: LISTEN c1; LISTEN c2;
+step lcommit: COMMIT;
+step l2commit: COMMIT;
+listener2: NOTIFY "c1" with payload "" from notifier
+step l2stop: UNLISTEN *;
+
 starting permutation: llisten lbegin usage bignotify usage
 step llisten: LISTEN c1; LISTEN c2;
 step lbegin: BEGIN;

diff --git a/src/test/isolation/specs/async-notify.spec b/src/test/isolation/specs/async-notify.spec
index daf7bef903c0df32a29aea05304e9fec61359a54..a7b2600d25c651ffafa1e1d0e46f538ab83f1a25 100644 (file)
--- a/src/test/isolation/specs/async-notify.spec
+++ b/src/test/isolation/specs/async-notify.spec
@@ -41,8 +41,18 @@ session "listener"
 step "llisten" { LISTEN c1; LISTEN c2; }
 step "lcheck"  { SELECT 1 AS x; }
 step "lbegin"  { BEGIN; }
+step "lbegins" { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step "lcommit" { COMMIT; }
 teardown       { UNLISTEN *; }
 
+# In some tests we need a second listener, just to block the queue.
+
+session "listener2"
+step "l2listen"    { LISTEN c1; }
+step "l2begin" { BEGIN; }
+step "l2commit"    { COMMIT; }
+step "l2stop"  { UNLISTEN *; }
+
 
 # Trivial cases.
 permutation "listenc" "notify1" "notify2" "notify3" "notifyf"
@@ -59,6 +69,10 @@ permutation "llisten" "notify1" "notify2" "notify3" "notifyf" "lcheck"
 # Again, with local delivery too.
 permutation "listenc" "llisten" "notify1" "notify2" "notify3" "notifyf" "lcheck"
 
+# Check for bug when initial listen is only action in a serializable xact,
+# and notify queue is not empty
+permutation "l2listen" "l2begin" "notify1" "lbegins" "llisten" "lcommit" "l2commit" "l2stop"
+
 # Verify that pg_notification_queue_usage correctly reports a non-zero result,
 # after submitting notifications while another connection is listening for
 # those notifications and waiting inside an active transaction.  We have to