On Sat, Oct 1, 2016 at 5:15 AM, Peter Geoghegan <p...@heroku.com> wrote:
> On Fri, Sep 30, 2016 at 5:33 PM, Konstantin Knizhnik
> <k.knizh...@postgrespro.ru> wrote:
>> So the question is whether it is correct that ExecOnConflictUpdate tries to
>> access and update tuple without holding lock on the buffer?
>
> You're right -- this is a bug in Postgres.
(Thomas Munro and Kevin Grittner added to CC list.)
Attached is a revision of Thomas' patch to fix a related issue; it now
also fixes this no-buffer-lock-held issue. He posted his version of
this to a -general mailing list thread a week ago [1]. This was a
thread about spurious serialization failure with ON CONFLICT DO
NOTHING. I understand that Kevin is still not happy with the behavior
under SSI even with our fix, since serialization failures will still
occur more often than necessary (see other thread for details of what
I'm talking about).
I believe this patch to be a strict improvement on master, and I
suggest it be applied in advance of the deadline to get fixes in for
the next set of point releases. Note that this revision includes
Thomas' isolation test, which is completely unchanged. I still intend
to work with Kevin to do better than this under SSI, though that will
probably be for a future point release. The behavior proposed by my
fix here is the right thing for REPEATABLE READ isolation level, which
has nothing to do with Kevin's proposed more careful handling under
SSI. Besides, the buffer pin bug reported by Konstantin on this thread
really should be addressed ASAP.
[1]
https://www.postgresql.org/message-id/CAEepm=3ra9ngdhocdbtb4iib7mwdavqybns3f47svkh1mk-...@mail.gmail.com
--
Peter Geoghegan
From 648de70f226831af04e1d31329118c325161da0b Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <p...@bowt.ie>
Date: Wed, 19 Oct 2016 13:59:20 -0700
Subject: [PATCH] Fix ON CONFLICT bugs at higher isolation levels.
When used at higher isolation levels, ON CONFLICT DO NOTHING could raise
spurious serialization failure errors. This happened when duplicate
values were proposed for insertion within a single ON CONFLICT DO
NOTHING command. (Similar ON CONFLICT DO UPDATE cases typically raised
valid cardinality violation errors instead.)
Separately, though in the same code path, insufficient care was taken
when a visibility check was performed on some existing, conflicting
tuple. At least a shared buffer lock is required on any buffer
containing a tuple whose visibility is checked through tqual.c routines,
which may set hint bits (a buffer pin is therefore insufficient).
To fix the first issue, check if tuple was created by current
transaction as a further condition of raising a serialization failure
in relevant test. To fix the second issue, outsource visibility test to
preexisting heap_fetch() call.
Patch by Thomas Munroe and Peter Geoghegan. First bug reported by Jason
Dusek. Second bug reported by Konstantin Knizhnik.
Report: <CAO3NbwOycQjt2Oqy2VW-eLTq2M5uGMyHnGm=rnga4mjqcyd...@mail.gmail.com>
Report: <57ee93c8.8080...@postgrespro.ru>
Backpatch-through: 9.5
---
src/backend/executor/nodeModifyTable.c | 41 ++++++++++------------
.../expected/insert-conflict-do-nothing-2.out | 29 +++++++++++++++
src/test/isolation/isolation_schedule | 1 +
.../specs/insert-conflict-do-nothing-2.spec | 34 ++++++++++++++++++
4 files changed, 82 insertions(+), 23 deletions(-)
create mode 100644 src/test/isolation/expected/insert-conflict-do-nothing-2.out
create mode 100644 src/test/isolation/specs/insert-conflict-do-nothing-2.spec
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index af7b26c..3c4f458 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -179,7 +179,7 @@ ExecProcessReturning(ResultRelInfo *resultRelInfo,
}
/*
- * ExecCheckHeapTupleVisible -- verify heap tuple is visible
+ * ExecCheckTIDVisible -- fetch tuple, and verify its visibility
*
* It would not be consistent with guarantees of the higher isolation levels to
* proceed with avoiding insertion (taking speculative insertion's alternative
@@ -187,23 +187,6 @@ ExecProcessReturning(ResultRelInfo *resultRelInfo,
* Check for the need to raise a serialization failure, and do so as necessary.
*/
static void
-ExecCheckHeapTupleVisible(EState *estate,
- HeapTuple tuple,
- Buffer buffer)
-{
- if (!IsolationUsesXactSnapshot())
- return;
-
- if (!HeapTupleSatisfiesVisibility(tuple, estate->es_snapshot, buffer))
- ereport(ERROR,
- (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
- errmsg("could not serialize access due to concurrent update")));
-}
-
-/*
- * ExecCheckTIDVisible -- convenience variant of ExecCheckHeapTupleVisible()
- */
-static void
ExecCheckTIDVisible(EState *estate,
ResultRelInfo *relinfo,
ItemPointer tid)
@@ -212,14 +195,26 @@ ExecCheckTIDVisible(EState *estate,
Buffer buffer;
HeapTupleData tuple;
- /* Redundantly check isolation level */
if (!IsolationUsesXactSnapshot())
return;
tuple.t_self = *tid;
- if (!heap_fetch(rel, SnapshotAny, &tuple, &buffer, false, NULL))
- elog(ERROR, "failed to fetch conflicting tuple for ON CONFLICT");
- ExecCheckHeapTupleVisible(estate, &tuple, buffer);
+ if (!heap_fetch(rel, estate->es_snapshot, &tuple, &buffer, true, NULL))
+ {
+ /*
+ * Must not raise serialization failure when multiple values are
+ * proposed for insertion by the same command with duplication among
+ * values (this is at least possible with ON CONFLICT DO NOTHING, where
+ * cardinality violation errors are never raised). Avoid this with
+ * final check that determines if tuple was inserted by another
+ * transaction.
+ */
+ if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple.t_data)))
+ ereport(ERROR,
+ (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("could not serialize access due to concurrent update")));
+ }
+
ReleaseBuffer(buffer);
}
@@ -1170,7 +1165,7 @@ ExecOnConflictUpdate(ModifyTableState *mtstate,
* snapshot. This is in line with the way UPDATE deals with newer tuple
* versions.
*/
- ExecCheckHeapTupleVisible(estate, &tuple, buffer);
+ ExecCheckTIDVisible(estate, resultRelInfo, &tuple.t_self);
/* Store target's existing tuple in the state's dedicated slot */
ExecStoreTuple(&tuple, mtstate->mt_existing, buffer, false);
diff --git a/src/test/isolation/expected/insert-conflict-do-nothing-2.out b/src/test/isolation/expected/insert-conflict-do-nothing-2.out
new file mode 100644
index 0000000..b379ab1
--- /dev/null
+++ b/src/test/isolation/expected/insert-conflict-do-nothing-2.out
@@ -0,0 +1,29 @@
+Parsed test spec with 2 sessions
+
+starting permutation: donothing1 c1 donothing2 c2
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step c1: COMMIT;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing2') ON CONFLICT DO NOTHING;
+step c2: COMMIT;
+
+starting permutation: donothing2 c2 donothing1 c1
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing2') ON CONFLICT DO NOTHING;
+step c2: COMMIT;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step c1: COMMIT;
+
+starting permutation: donothing1 donothing2 c1 c2
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING;
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing2') ON CONFLICT DO NOTHING; <waiting ...>
+step c1: COMMIT;
+step donothing2: <... completed>
+error in steps c1 donothing2: ERROR: could not serialize access due to concurrent update
+step c2: COMMIT;
+
+starting permutation: donothing2 donothing1 c2 c1
+step donothing2: INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing2') ON CONFLICT DO NOTHING;
+step donothing1: INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING; <waiting ...>
+step c2: COMMIT;
+step donothing1: <... completed>
+error in steps c2 donothing1: ERROR: could not serialize access due to concurrent update
+step c1: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index a96a318..2606a27 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -25,6 +25,7 @@ test: eval-plan-qual
test: lock-update-delete
test: lock-update-traversal
test: insert-conflict-do-nothing
+test: insert-conflict-do-nothing-2
test: insert-conflict-do-update
test: insert-conflict-do-update-2
test: insert-conflict-do-update-3
diff --git a/src/test/isolation/specs/insert-conflict-do-nothing-2.spec b/src/test/isolation/specs/insert-conflict-do-nothing-2.spec
new file mode 100644
index 0000000..bbe92f7
--- /dev/null
+++ b/src/test/isolation/specs/insert-conflict-do-nothing-2.spec
@@ -0,0 +1,34 @@
+# INSERT...ON CONFLICT DO NOTHING test with multiple rows in higher isolation
+
+setup
+{
+ CREATE TABLE ints (key int primary key, val text);
+}
+
+teardown
+{
+ DROP TABLE ints;
+}
+
+session "s1"
+setup
+{
+ BEGIN ISOLATION LEVEL REPEATABLE READ;
+}
+step "donothing1" { INSERT INTO ints(key, val) VALUES(1, 'donothing1') ON CONFLICT DO NOTHING; }
+step "c1" { COMMIT; }
+step "a1" { ABORT; }
+
+session "s2"
+setup
+{
+ BEGIN ISOLATION LEVEL REPEATABLE READ;
+}
+step "donothing2" { INSERT INTO ints(key, val) VALUES(1, 'donothing2'), (1, 'donothing2') ON CONFLICT DO NOTHING; }
+step "c2" { COMMIT; }
+step "a2" { ABORT; }
+
+permutation "donothing1" "c1" "donothing2" "c2"
+permutation "donothing2" "c2" "donothing1" "c1"
+permutation "donothing1" "donothing2" "c1" "c2"
+permutation "donothing2" "donothing1" "c2" "c1"
--
2.7.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
