Don't ignore tuple locks propagated by our updates

If a tuple was locked by transaction A, and transaction B updated it,
the new version of the tuple created by B would be locked by A, yet
visible only to B; due to an oversight in HeapTupleSatisfiesUpdate, the
lock held by A wouldn't get checked if transaction B later deleted (or
key-updated) the new version of the tuple.  This might cause referential
integrity checks to give false positives (that is, allow deletes that
should have been rejected).

This is an easy oversight to have made, because prior to improved tuple
locks in commit 0ac5ad5134f it wasn't possible to have tuples created by
our own transaction that were also locked by remote transactions, and so
locks weren't even considered in that code path.

It is recommended that foreign keys be rechecked manually in bulk after
installing this update, in case some referenced rows are missing with
some referencing row remaining.

Per bug reported by Daniel Wood in
CAPweHKe5QQ1747X2c0tA=5zf4YnS2xcvGf13Opd-1Mq24rF1cQ@mail.gmail.com

diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 55a8ca7ac49d747f18c02d259d8a72d27c878f36..03581bea6637375aaccd54b17db88310bda7d9b5 100644 (file)
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1274,6 +1274,39 @@ retry:
        return truelength;
 }
 
+/*
+ * MultiXactHasRunningRemoteMembers
+ *             Does the given multixact have still-live members from
+ *             transactions other than our own?
+ */
+bool
+MultiXactHasRunningRemoteMembers(MultiXactId multi)
+{
+       MultiXactMember *members;
+       int                     nmembers;
+       int                     i;
+
+       nmembers = GetMultiXactIdMembers(multi, &members, true);
+       if (nmembers <= 0)
+               return false;
+
+       for (i = 0; i < nmembers; i++)
+       {
+               /* not interested in our own members */
+               if (TransactionIdIsCurrentTransactionId(members[i].xid))
+                       continue;
+
+               if (TransactionIdIsInProgress(members[i].xid))
+               {
+                       pfree(members);
+                       return true;
+               }
+       }
+
+       pfree(members);
+       return false;
+}
+
 /*
  * mxactMemberComparator
  *             qsort comparison function for MultiXactMember

diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index e5d0b0a666e5af315d4f4ced4c39c3bd59a9773e..1ff1da2f07607a35920877aec2281c8a5cfa8ed0 100644 (file)
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -493,8 +493,36 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
                        if (tuple->t_infomask & HEAP_XMAX_INVALID)      /* xid invalid */
                                return HeapTupleMayBeUpdated;
 
-                       if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))        /* not deleter */
-                               return HeapTupleMayBeUpdated;
+                       if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
+                       {
+                               TransactionId   xmax;
+
+                               xmax = HeapTupleHeaderGetRawXmax(tuple);
+
+                               /*
+                                * Careful here: even though this tuple was created by our own
+                                * transaction, it might be locked by other transactions, if
+                                * the original version was key-share locked when we updated
+                                * it.
+                                */
+
+                               if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
+                               {
+                                       if (MultiXactHasRunningRemoteMembers(xmax))
+                                               return HeapTupleBeingUpdated;
+                                       else
+                                               return HeapTupleMayBeUpdated;
+                               }
+
+                               /* if locker is gone, all's well */
+                               if (!TransactionIdIsInProgress(xmax))
+                                       return HeapTupleMayBeUpdated;
+
+                               if (!TransactionIdIsCurrentTransactionId(xmax))
+                                       return HeapTupleBeingUpdated;
+                               else
+                                       return HeapTupleMayBeUpdated;
+                       }
 
                        if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
                        {
@@ -507,7 +535,11 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
 
                                /* updating subtransaction must have aborted */
                                if (!TransactionIdIsCurrentTransactionId(xmax))
+                               {
+                                       if (MultiXactHasRunningRemoteMembers(HeapTupleHeaderGetRawXmax(tuple)))
+                                               return HeapTupleBeingUpdated;
                                        return HeapTupleMayBeUpdated;
+                               }
                                else
                                {
                                        if (HeapTupleHeaderGetCmax(tuple) >= curcid)

diff --git a/src/include/access/multixact.h b/src/include/access/multixact.h
index 0e3b273b9e2463f251641b156f0d838212c237ba..5f82907daccd1a061dcbd916ae2783ef221f8964 100644 (file)
--- a/src/include/access/multixact.h
+++ b/src/include/access/multixact.h
@@ -89,6 +89,7 @@ extern bool MultiXactIdIsRunning(MultiXactId multi);
 extern void MultiXactIdSetOldestMember(void);
 extern int GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **xids,
                                          bool allow_old);
+extern bool MultiXactHasRunningRemoteMembers(MultiXactId multi);
 extern bool MultiXactIdPrecedes(MultiXactId multi1, MultiXactId multi2);
 extern bool MultiXactIdPrecedesOrEquals(MultiXactId multi1,
                                                        MultiXactId multi2);

diff --git a/src/test/isolation/expected/propagate-lock-delete.out b/src/test/isolation/expected/propagate-lock-delete.out
new file mode 100644 (file)
index 0000000..b668b89
--- /dev/null
+++ b/src/test/isolation/expected/propagate-lock-delete.out
@@ -0,0 +1,105 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1b s1l s2b s2l s3b s3u s3d s1c s2c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s2b: BEGIN;
+step s2l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u: UPDATE parent SET c=lower(c);
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s2c: COMMIT;
+step s3d: <... completed>
+error in steps s2c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s2b s2l s3b s3u s3svu s3d s1c s2c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s2b: BEGIN;
+step s2l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u: UPDATE parent SET c=lower(c);
+step s3svu: SAVEPOINT f; UPDATE parent SET c = 'bbb'; ROLLBACK TO f;
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s2c: COMMIT;
+step s3d: <... completed>
+error in steps s2c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s2b s2l s3b s3u2 s3d s1c s2c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s2b: BEGIN;
+step s2l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u2: UPDATE parent SET i = i;
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s2c: COMMIT;
+step s3d: <... completed>
+error in steps s2c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s2b s2l s3b s3u2 s3svu s3d s1c s2c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s2b: BEGIN;
+step s2l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u2: UPDATE parent SET i = i;
+step s3svu: SAVEPOINT f; UPDATE parent SET c = 'bbb'; ROLLBACK TO f;
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s2c: COMMIT;
+step s3d: <... completed>
+error in steps s2c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s3b s3u s3d s1c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u: UPDATE parent SET c=lower(c);
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s3d: <... completed>
+error in steps s1c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s3b s3u s3svu s3d s1c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u: UPDATE parent SET c=lower(c);
+step s3svu: SAVEPOINT f; UPDATE parent SET c = 'bbb'; ROLLBACK TO f;
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s3d: <... completed>
+error in steps s1c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s3b s3u2 s3d s1c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u2: UPDATE parent SET i = i;
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s3d: <... completed>
+error in steps s1c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;
+
+starting permutation: s1b s1l s3b s3u2 s3svu s3d s1c s3c
+step s1b: BEGIN;
+step s1l: INSERT INTO child VALUES (1);
+step s3b: BEGIN;
+step s3u2: UPDATE parent SET i = i;
+step s3svu: SAVEPOINT f; UPDATE parent SET c = 'bbb'; ROLLBACK TO f;
+step s3d: DELETE FROM parent; <waiting ...>
+step s1c: COMMIT;
+step s3d: <... completed>
+error in steps s1c s3d: ERROR:  update or delete on table "parent" violates foreign key constraint "child_i_fkey" on table "child"
+step s3c: COMMIT;

diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index dd4b404183674173bca44e76f7086d90787fc164..1e73b4aebd734803329db3b76e74ce74d5b53a18 100644 (file)
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -21,5 +21,6 @@ test: delete-abort-savept-2
 test: aborted-keyrevoke
 test: multixact-no-deadlock
 test: multixact-no-forget
+test: propagate-lock-delete
 test: drop-index-concurrently-1
 test: timeouts

diff --git a/src/test/isolation/specs/propagate-lock-delete.spec b/src/test/isolation/specs/propagate-lock-delete.spec
new file mode 100644 (file)
index 0000000..857c36b
--- /dev/null
+++ b/src/test/isolation/specs/propagate-lock-delete.spec
@@ -0,0 +1,42 @@
+# When an update propagates a preexisting lock on the updated tuple, make sure
+# we don't ignore the lock in subsequent operations of the new version.  (The
+# version with the aborted savepoint uses a slightly different code path).
+setup
+{
+       create table parent (i int, c char(3));
+       create unique index parent_idx on parent (i);
+       insert into parent values (1, 'AAA');
+       create table child (i int references parent(i));
+}
+
+teardown
+{
+       drop table child, parent;
+}
+
+session "s1"
+step "s1b"  { BEGIN; }
+step "s1l"     { INSERT INTO child VALUES (1); }
+step "s1c"     { COMMIT; }
+
+session "s2"
+step "s2b"  { BEGIN; }
+step "s2l"     { INSERT INTO child VALUES (1); }
+step "s2c"     { COMMIT; }
+
+session "s3"
+step "s3b"     { BEGIN; }
+step "s3u"     { UPDATE parent SET c=lower(c); }       # no key update
+step "s3u2"    { UPDATE parent SET i = i; }            # key update
+step "s3svu" { SAVEPOINT f; UPDATE parent SET c = 'bbb'; ROLLBACK TO f; }
+step "s3d"     { DELETE FROM parent; }
+step "s3c"     { COMMIT; }
+
+permutation "s1b" "s1l" "s2b" "s2l" "s3b" "s3u"          "s3d" "s1c" "s2c" "s3c"
+permutation "s1b" "s1l" "s2b" "s2l" "s3b" "s3u"  "s3svu" "s3d" "s1c" "s2c" "s3c"
+permutation "s1b" "s1l" "s2b" "s2l" "s3b" "s3u2"         "s3d" "s1c" "s2c" "s3c"
+permutation "s1b" "s1l" "s2b" "s2l" "s3b" "s3u2" "s3svu" "s3d" "s1c" "s2c" "s3c"
+permutation "s1b" "s1l"             "s3b" "s3u"          "s3d" "s1c"       "s3c"
+permutation "s1b" "s1l"             "s3b" "s3u"  "s3svu" "s3d" "s1c"       "s3c"
+permutation "s1b" "s1l"             "s3b" "s3u2"         "s3d" "s1c"       "s3c"
+permutation "s1b" "s1l"             "s3b" "s3u2" "s3svu" "s3d" "s1c"       "s3c"