pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
9+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2016-07-15 533e9c6b06 649dd1b58b 166873dd0c 6c243f90ab Avoid serializability errors when locking a tuple with a committed update
* BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-10-14 16:48 odo@odoo.com
0 siblings, 1 reply; 9+ messages in thread
From: odo@odoo.com @ 2015-10-14 16:48 UTC (permalink / raw)
To: pgsql-bugs
The following bug has been logged on the website:
Bug reference: 13681
Logged by: Olivier Dony
Email address: odo@odoo.com
PostgreSQL version: 9.3.10
Operating system: Ubuntu 14.04 LTS
Description:
This is a back-patch request of 05315498012530d44cd89a209242a243374e274d to
9.3 and 9.4.
As discussed in the -general list[1], both 9.3 and 9.4 show spurious
serialization failures when faced with the use case included below.
In 9.2, T2 used to block until T1's commit, but then continued without
error, and in 9.5 both T1 and T2 proceed without blocking nor error.
Kevin Grittner located[2] the root cause as a regression that was fixed by
Álvaro at 0531549 [3].
For what it's worth, our system uses many long-running transactions
(background jobs, batch data imports, etc.) that are frequently interrupted
and rolled back by micro-transactions coming from users who just happen to
update minor data on their records (such as their last login date). So this
bug appears to cause more than just a performance regression.
Let me know if there is anything I can do to help the back-patch happen...
# The use case
-- 1. Setup tables
CREATE TABLE users (id serial PRIMARY KEY,
name varchar,
date timestamp );
CREATE TABLE orders (id serial PRIMARY KEY,
name varchar,
user_id int REFERENCES users (id) );
INSERT INTO users (id, name) VALUES (1, 'foo');
INSERT INTO orders (id, name) VALUES (1, 'order 1');
-- 2. Run 2 concurrent transactions: T1 and T2
T1 T2
|-----------------------------|----------------------------------|
BEGIN ISOLATION LEVEL
REPEATABLE READ;
UPDATE orders
SET name = 'order of foo',
user_id = 1
WHERE id = 1;
BEGIN ISOLATION LEVEL
REPEATABLE READ;
UPDATE users
SET date = now()
WHERE id = 1;
COMMIT;
UPDATE orders
SET name = 'order of foo (2)',
user_id = 1
WHERE id = 1;
T1 fails with:
ERROR: could not serialize access due to concurrent update
CONTEXT: SQL statement "SELECT 1 FROM ONLY "public"."users" x WHERE "id"
OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x"
# --
Many thanks,
Olivier
[1] http://www.postgresql.org/message-id/flat/560AA479.4080807@odoo.com
[2]
http://www.postgresql.org/message-id/flat/560AA479.4080807@odoo.com#1354271993.744124.1444079872314....
[3]
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=0531549
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-12-17 18:31 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: odo@odoo.com
0 siblings, 1 reply; 9+ messages in thread
From: Alvaro Herrera @ 2015-12-17 18:31 UTC (permalink / raw)
To: odo@odoo.com; +Cc: pgsql-bugs
odo@odoo.com wrote:
> This is a back-patch request of 05315498012530d44cd89a209242a243374e274d to
> 9.3 and 9.4.
>
> As discussed in the -general list[1], both 9.3 and 9.4 show spurious
> serialization failures when faced with the use case included below.
>
> In 9.2, T2 used to block until T1's commit, but then continued without
> error, and in 9.5 both T1 and T2 proceed without blocking nor error.
>
> Kevin Grittner located[2] the root cause as a regression that was fixed by
> Álvaro at 0531549 [3].
>
> For what it's worth, our system uses many long-running transactions
> (background jobs, batch data imports, etc.) that are frequently interrupted
> and rolled back by micro-transactions coming from users who just happen to
> update minor data on their records (such as their last login date). So this
> bug appears to cause more than just a performance regression.
I would like to understand why does that patch fix the problem -- maybe
it's spurious and the real reason is something different. The commit
message states:
Commit 0ac5ad5134f2 removed an optimization in multixact.c that skipped
fetching members of MultiXactId that were older than our
OldestVisibleMXactId value. The reason this was removed is that it is
possible for multixacts that contain updates to be older than that
value. However, if the caller is certain that the multi does not
contain an update (because the infomask bits say so), it can pass this
info down to GetMultiXactIdMembers, enabling it to use the old
optimization.
In your test case,
> T1 T2
> |-----------------------------|----------------------------------|
> BEGIN ISOLATION LEVEL
> REPEATABLE READ;
>
> UPDATE orders
> SET name = 'order of foo',
> user_id = 1
> WHERE id = 1;
>
> BEGIN ISOLATION LEVEL
> REPEATABLE READ;
>
> UPDATE users
> SET date = now()
> WHERE id = 1;
>
> COMMIT;
>
> UPDATE orders
> SET name = 'order of foo (2)',
> user_id = 1
> WHERE id = 1;
we have a transaction that takes a lock-only multi in
table users, and then when we do the second update we don't look it up
because ...?? And then this causes the test case not to fail because
..?
I would like to understand the mechanism for this fix before declaring
that the fix is correct.
The patch doesn't apply cleanly because of other changes in the area, so
I attach the backpatched version here, as well as a test case for
isolationtester in a separate commit (with which it's easy to confirm
that the problem does exist and that the patch indeed fixes it.)
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Attachments: download all (2 files, .tar.gz)
0001-Avoid-uselessly-looking-up-old-LOCK_ONLY-multixacts.patch (text/x-diff, 12.4K) download
From 29a7797721d14f96596c5b3f33f5cdb02b504752 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 29 Jul 2014 15:41:06 -0400
Subject: [PATCH 1/2] Avoid uselessly looking up old LOCK_ONLY multixacts
Commit 0ac5ad5134f2 removed an optimization in multixact.c that skipped
fetching members of MultiXactId that were older than our
OldestVisibleMXactId value. The reason this was removed is that it is
possible for multixacts that contain updates to be older than that
value. However, if the caller is certain that the multi does not
contain an update (because the infomask bits say so), it can pass this
info down to GetMultiXactIdMembers, enabling it to use the old
optimization.
Pointed out by Andres Freund in 20131121200517.GM7240@alap2.anarazel.de
This is a backpatch of 05315498012 to branches 9.3 and 9.4, as requested
in bug report #13681; per report by Olivier Dony, lacking this fix is
problematic for serialization.
Discussion: http://www.postgresql.org/message-id/flat/560AA479.4080807@odoo.com
---
contrib/pgrowlocks/pgrowlocks.c | 3 ++-
src/backend/access/heap/heapam.c | 32 +++++++++++++++++++++-----------
src/backend/access/transam/multixact.c | 31 +++++++++++++++++++++++++------
src/backend/utils/time/tqual.c | 16 +++++++++++-----
src/include/access/multixact.h | 4 ++--
5 files changed, 61 insertions(+), 25 deletions(-)
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 075d781..89f4c81 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -167,7 +167,8 @@ pgrowlocks(PG_FUNCTION_ARGS)
allow_old = !(infomask & HEAP_LOCK_MASK) &&
(infomask & HEAP_XMAX_LOCK_ONLY);
- nmembers = GetMultiXactIdMembers(xmax, &members, allow_old);
+ nmembers = GetMultiXactIdMembers(xmax, &members, allow_old,
+ false);
if (nmembers == -1)
{
values[Atnum_xids] = "{0}";
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 559970f..3a08868 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4039,7 +4039,9 @@ l3:
* the case, HeapTupleSatisfiesUpdate would have returned
* MayBeUpdated and we wouldn't be here.
*/
- nmembers = GetMultiXactIdMembers(xwait, &members, false);
+ nmembers =
+ GetMultiXactIdMembers(xwait, &members, false,
+ HEAP_XMAX_IS_LOCKED_ONLY(infomask));
for (i = 0; i < nmembers; i++)
{
@@ -4060,7 +4062,8 @@ l3:
}
}
- pfree(members);
+ if (members)
+ pfree(members);
}
/*
@@ -4661,7 +4664,7 @@ l5:
* MultiXactIdExpand if we weren't to do this, so this check is not
* incurring extra work anyhow.
*/
- if (!MultiXactIdIsRunning(xmax))
+ if (!MultiXactIdIsRunning(xmax, HEAP_XMAX_IS_LOCKED_ONLY(old_infomask)))
{
if (HEAP_XMAX_IS_LOCKED_ONLY(old_infomask) ||
TransactionIdDidAbort(MultiXactIdGetUpdateXid(xmax,
@@ -5001,7 +5004,8 @@ l4:
int i;
MultiXactMember *members;
- nmembers = GetMultiXactIdMembers(rawxmax, &members, false);
+ nmembers = GetMultiXactIdMembers(rawxmax, &members, false,
+ HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
for (i = 0; i < nmembers; i++)
{
HTSU_Result res;
@@ -5356,7 +5360,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
*/
Assert((!(t_infomask & HEAP_LOCK_MASK) &&
HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)) ||
- !MultiXactIdIsRunning(multi));
+ !MultiXactIdIsRunning(multi,
+ HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)));
if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))
{
*flags |= FRM_INVALIDATE_XMAX;
@@ -5399,7 +5404,9 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
allow_old = !(t_infomask & HEAP_LOCK_MASK) &&
HEAP_XMAX_IS_LOCKED_ONLY(t_infomask);
- nmembers = GetMultiXactIdMembers(multi, &members, allow_old);
+ nmembers =
+ GetMultiXactIdMembers(multi, &members, allow_old,
+ HEAP_XMAX_IS_LOCKED_ONLY(t_infomask));
if (nmembers <= 0)
{
/* Nothing worth keeping */
@@ -5825,7 +5832,7 @@ GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask,
* We only use this in multis we just created, so they cannot be values
* pre-pg_upgrade.
*/
- nmembers = GetMultiXactIdMembers(multi, &members, false);
+ nmembers = GetMultiXactIdMembers(multi, &members, false, false);
for (i = 0; i < nmembers; i++)
{
@@ -5904,7 +5911,7 @@ MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask)
* Since we know the LOCK_ONLY bit is not set, this cannot be a multi from
* pre-pg_upgrade.
*/
- nmembers = GetMultiXactIdMembers(xmax, &members, false);
+ nmembers = GetMultiXactIdMembers(xmax, &members, false, false);
if (nmembers > 0)
{
@@ -5966,7 +5973,8 @@ DoesMultiXactIdConflict(MultiXactId multi, uint16 infomask,
LOCKMODE wanted = tupleLockExtraInfo[lockmode].hwlock;
allow_old = !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(infomask);
- nmembers = GetMultiXactIdMembers(multi, &members, allow_old);
+ nmembers = GetMultiXactIdMembers(multi, &members, allow_old,
+ HEAP_XMAX_IS_LOCKED_ONLY(infomask));
if (nmembers >= 0)
{
int i;
@@ -6044,7 +6052,8 @@ Do_MultiXactIdWait(MultiXactId multi, MultiXactStatus status,
int remain = 0;
allow_old = !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(infomask);
- nmembers = GetMultiXactIdMembers(multi, &members, allow_old);
+ nmembers = GetMultiXactIdMembers(multi, &members, allow_old,
+ HEAP_XMAX_IS_LOCKED_ONLY(infomask));
if (nmembers >= 0)
{
@@ -6181,7 +6190,8 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid,
allow_old = !(tuple->t_infomask & HEAP_LOCK_MASK) &&
HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask);
- nmembers = GetMultiXactIdMembers(multi, &members, allow_old);
+ nmembers = GetMultiXactIdMembers(multi, &members, allow_old,
+ HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
for (i = 0; i < nmembers; i++)
{
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b10496..fb5c277 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -456,7 +456,7 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
* caller of this function does a check that the multixact is no longer
* running.
*/
- nmembers = GetMultiXactIdMembers(multi, &members, false);
+ nmembers = GetMultiXactIdMembers(multi, &members, false, false);
if (nmembers < 0)
{
@@ -545,7 +545,7 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
* a pg_upgraded share-locked tuple.
*/
bool
-MultiXactIdIsRunning(MultiXactId multi)
+MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly)
{
MultiXactMember *members;
int nmembers;
@@ -557,7 +557,7 @@ MultiXactIdIsRunning(MultiXactId multi)
* "false" here means we assume our callers have checked that the given
* multi cannot possibly come from a pg_upgraded database.
*/
- nmembers = GetMultiXactIdMembers(multi, &members, false);
+ nmembers = GetMultiXactIdMembers(multi, &members, false, isLockOnly);
if (nmembers < 0)
{
@@ -1202,10 +1202,15 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
* the value currently known as the next to assign, raise an error. Previously
* these also returned -1, but since this can lead to the wrong visibility
* results, it is dangerous to do that.
+ *
+ * onlyLock must be set to true if caller is certain that the given multi
+ * is used only to lock tuples; can be false without loss of correctness,
+ * but passing a true means we can return quickly without checking for
+ * old updates.
*/
int
GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
- bool allow_old)
+ bool allow_old, bool onlyLock)
{
int pageno;
int prev_pageno;
@@ -1240,6 +1245,19 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
MultiXactIdSetOldestVisible();
/*
+ * If we know the multi is used only for locking and not for updates,
+ * then we can skip checking if the value is older than our oldest
+ * visible multi. It cannot possibly still be running.
+ */
+ if (onlyLock &&
+ MultiXactIdPrecedes(multi, OldestVisibleMXactId[MyBackendId]))
+ {
+ debug_elog2(DEBUG2, "GetMembers: a locker-only multi is too old");
+ *members = NULL;
+ return -1;
+ }
+
+ /*
* We check known limits on MultiXact before resorting to the SLRU area.
*
* An ID older than MultiXactState->oldestMultiXactId cannot possibly be
@@ -1443,7 +1461,7 @@ MultiXactHasRunningRemoteMembers(MultiXactId multi)
int nmembers;
int i;
- nmembers = GetMultiXactIdMembers(multi, &members, true);
+ nmembers = GetMultiXactIdMembers(multi, &members, true, false);
if (nmembers <= 0)
return false;
@@ -3307,7 +3325,8 @@ pg_get_multixact_members(PG_FUNCTION_ARGS)
multi = palloc(sizeof(mxact));
/* no need to allow for old values here */
- multi->nmembers = GetMultiXactIdMembers(mxid, &multi->members, false);
+ multi->nmembers = GetMultiXactIdMembers(mxid, &multi->members, false,
+ false);
multi->iter = 0;
tupdesc = CreateTemplateTupleDesc(2, false);
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index ccbbf62..816e851 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -795,7 +795,7 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
*/
if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK |
HEAP_XMAX_KEYSHR_LOCK)) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), true))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
@@ -803,6 +803,11 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
}
xmax = HeapTupleGetUpdateXid(tuple);
+ if (!TransactionIdIsValid(xmax))
+ {
+ if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
+ return HeapTupleBeingUpdated;
+ }
/* not LOCKED_ONLY, so it has to have an xmax */
Assert(TransactionIdIsValid(xmax));
@@ -815,7 +820,7 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
return HeapTupleInvisible; /* updated before scan started */
}
- if (TransactionIdIsInProgress(xmax))
+ if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
return HeapTupleBeingUpdated;
if (TransactionIdDidCommit(xmax))
@@ -826,7 +831,7 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,
* what about the other members?
*/
- if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
+ if (!MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
{
/*
* There's no member, even just a locker, alive anymore, so we can
@@ -1416,7 +1421,8 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin,
*/
if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK |
HEAP_XMAX_KEYSHR_LOCK)) &&
- MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
+ MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+ true))
return HEAPTUPLE_LIVE;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
@@ -1443,7 +1449,7 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin,
{
TransactionId xmax;
- if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
+ if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), false))
{
/* already checked above */
Assert(!HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
diff --git a/src/include/access/multixact.h b/src/include/access/multixact.h
index ea37e92..0c92363 100644
--- a/src/include/access/multixact.h
+++ b/src/include/access/multixact.h
@@ -91,10 +91,10 @@ extern MultiXactId MultiXactIdCreateFromMembers(int nmembers,
MultiXactMember *members);
extern MultiXactId ReadNextMultiXactId(void);
-extern bool MultiXactIdIsRunning(MultiXactId multi);
+extern bool MultiXactIdIsRunning(MultiXactId multi, bool isLockOnly);
extern void MultiXactIdSetOldestMember(void);
extern int GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **xids,
- bool allow_old);
+ bool allow_old, bool isLockOnly);
extern bool MultiXactHasRunningRemoteMembers(MultiXactId multi);
extern bool MultiXactIdPrecedes(MultiXactId multi1, MultiXactId multi2);
extern bool MultiXactIdPrecedesOrEquals(MultiXactId multi1,
--
2.1.4
0002-Add-isolation-spec-for-13681.patch (text/x-diff, 2.7K) download
From 6d964bee2a4e4ee6ada8217a2df248fa0ba29f09 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 17 Dec 2015 15:30:09 -0300
Subject: [PATCH 2/2] Add isolation spec for #13681
---
src/test/isolation/expected/odoo-bug.out | 10 ++++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/odoo-bug.spec | 28 ++++++++++++++++++++++++++++
3 files changed, 39 insertions(+)
create mode 100644 src/test/isolation/expected/odoo-bug.out
create mode 100644 src/test/isolation/specs/odoo-bug.spec
diff --git a/src/test/isolation/expected/odoo-bug.out b/src/test/isolation/expected/odoo-bug.out
new file mode 100644
index 0000000..c3cf4c1
--- /dev/null
+++ b/src/test/isolation/expected/odoo-bug.out
@@ -0,0 +1,10 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1b s1u1 s2b s2u s2c s1u2 s1c
+step s1b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s1u1: UPDATE orders SET name = 'order of foo', user_id = 1 WHERE id = 1
+step s2b: BEGIN ISOLATION LEVEL REPEATABLE READ;
+step s2u: UPDATE users SET date = now() WHERE id = 1;
+step s2c: COMMIT;
+step s1u2: UPDATE orders SET name = 'order of foo (2)', user_id = 1 WHERE id = 1
+step s1c: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 60479c4..132a9c6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -23,5 +23,6 @@ test: multixact-no-deadlock
test: multixact-no-forget
test: propagate-lock-delete
test: tuplelock-conflict
+test: odoo-bug
test: drop-index-concurrently-1
test: timeouts
diff --git a/src/test/isolation/specs/odoo-bug.spec b/src/test/isolation/specs/odoo-bug.spec
new file mode 100644
index 0000000..8318b84
--- /dev/null
+++ b/src/test/isolation/specs/odoo-bug.spec
@@ -0,0 +1,28 @@
+setup
+{
+ DROP TABLE IF EXISTS users, orders;
+ CREATE TABLE users (id INTEGER PRIMARY KEY,
+ name varchar, date timestamp);
+ CREATE TABLE orders (id INTEGER PRIMARY KEY, name varchar,
+ user_id INTEGER REFERENCES users (id));
+ INSERT INTO users (id, name) VALUES (1, 'foo');
+ INSERT INTO orders (id, name) VALUES (1, 'first order');
+}
+
+teardown
+{
+ DROP TABLE users, orders;
+}
+
+session "s1"
+step "s1b" { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s1u1" { UPDATE orders SET name = 'order of foo', user_id = 1 WHERE id = 1 }
+step "s1u2" { UPDATE orders SET name = 'order of foo (2)', user_id = 1 WHERE id = 1}
+step "s1c" { COMMIT; }
+
+session "s2"
+step "s2b" { BEGIN ISOLATION LEVEL REPEATABLE READ; }
+step "s2u" { UPDATE users SET date = now() WHERE id = 1; }
+step "s2c" { COMMIT; }
+
+permutation "s1b" "s1u1" "s2b" "s2u" "s2c" "s1u2" "s1c"
--
2.1.4
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-12-17 19:34 Kevin Grittner <kgrittn@gmail.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Grittner @ 2015-12-17 19:34 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-bugs
On Thu, Dec 17, 2015 at 12:31 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>> [orders.user_id references users.id]
> In your test case,
>
>> T1 T2
>> |-----------------------------|----------------------------------|
>> BEGIN ISOLATION LEVEL
>> REPEATABLE READ;
>>
>> UPDATE orders
>> SET name = 'order of foo',
>> user_id = 1
>> WHERE id = 1;
>>
>> BEGIN ISOLATION LEVEL
>> REPEATABLE READ;
>>
>> UPDATE users
>> SET date = now()
>> WHERE id = 1;
>>
>> COMMIT;
>>
>> UPDATE orders
>> SET name = 'order of foo (2)',
>> user_id = 1
>> WHERE id = 1;
>
> we have a transaction that takes a lock-only multi in table
> users, and then when we do the second update we don't look it up
> because ...??
The referencing column value did not change. (We would not have
looked up on the first update either, since it also didn't change
there.)
> And then this causes the test case not to fail because ..?
The concurrent update of the referencing table is not seen as a
write conflict (because it didn't actually change).
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-12-18 17:24 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Kevin Grittner <kgrittn@gmail.com>
0 siblings, 1 reply; 9+ messages in thread
From: Alvaro Herrera @ 2015-12-18 17:24 UTC (permalink / raw)
To: Kevin Grittner <kgrittn@gmail.com>; +Cc: pgsql-bugs
Kevin Grittner wrote:
> On Thu, Dec 17, 2015 at 12:31 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > we have a transaction that takes a lock-only multi in table
> > users, and then when we do the second update we don't look it up
> > because ...??
>
> The referencing column value did not change. (We would not have
> looked up on the first update either, since it also didn't change
> there.)
>
> > And then this causes the test case not to fail because ..?
>
> The concurrent update of the referencing table is not seen as a
> write conflict (because it didn't actually change).
Okay, but I don't understand why that particular patch fixes the
problem. That patch was only supposed to skip looking up members of
multixacts when they were not interesting; so it's just a performance
optimization which shouldn't change the behavior of anything. However
in this case, it is changing behavior and I'm concerned that it might
have introduced other bugs.
Tracing through the test case, what happens is that s1's second update
calls SELECT FOR SHARE of the users tuple (via the RI code); this calls
ExecLockRows which calls heap_lock_tuple which calls
HeapTupleSatisfiesUpdate. The latter returns HeapTupleUpdated, because
it sees that the tuple has been modified by a transaction that already
committed (s2). But we already hold a for-key-share lock on the old
version of the tuple -- that HeapTupleUpdated result value should be
examined carefully by heap_lock_tuple to determine that, yes, it can
acquire the row lock without raising an error.
So, if I patch heap_lock_rows like below, the test passes too:
inline patch 1: src/backend/access/heap/heapam.c (+1/-1 lines) download inline-1.patch
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 559970f..aaf8e8e 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4005,7 +4005,7 @@ l3:
UnlockReleaseBuffer(*buffer);
elog(ERROR, "attempted to lock invisible tuple");
}
- else if (result == HeapTupleBeingUpdated)
+ else if (result == HeapTupleBeingUpdated || result == HeapTupleUpdated)
{
TransactionId xwait;
uint16 infomask;
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-12-18 18:53 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 2 replies; 9+ messages in thread
From: Alvaro Herrera @ 2015-12-18 18:53 UTC (permalink / raw)
To: Kevin Grittner <kgrittn@gmail.com>; +Cc: pgsql-bugs
Alvaro Herrera wrote:
> diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> index 559970f..aaf8e8e 100644
> --- a/src/backend/access/heap/heapam.c
> +++ b/src/backend/access/heap/heapam.c
> @@ -4005,7 +4005,7 @@ l3:
> UnlockReleaseBuffer(*buffer);
> elog(ERROR, "attempted to lock invisible tuple");
> }
> - else if (result == HeapTupleBeingUpdated)
> + else if (result == HeapTupleBeingUpdated || result == HeapTupleUpdated)
> {
> TransactionId xwait;
> uint16 infomask;
>
> I think heap_lock_rows had that shape (only consider BeingUpdated as a
> reason to check/wait) only because it was possible to lock a row that
> was being locked by someone else, but it wasn't possible to lock a row
> that had been updated by someone else -- which became possible in 9.3.
> So this patch is necessary, and not just to fix this one bug.
>
> I need to study the implications of this patch more closely, but I think
> in theory it is correct.
Hmm. So one thing against this patch is that some results of existing
isolation tests change. For instance, this one (lock-update-delete):
starting permutation: s2b s1l s2u s2_blocker3 s2c s2_unlock
pg_advisory_lock
step s2b: BEGIN;
step s1l: SELECT * FROM foo WHERE pg_advisory_xact_lock(0) IS NOT NULL AND key = 1 FOR KEY SHARE; <waiting ...>
step s2u: UPDATE foo SET value = 2 WHERE key = 1;
step s2_blocker3: UPDATE foo SET value = 2 WHERE key = 1;
step s2c: COMMIT;
step s2_unlock: SELECT pg_advisory_unlock(0);
pg_advisory_unlock
t
step s1l: <... completed>
key value
1 2
The initial SELECT is blocked and returns the updated tuple; if I patch
as proposed above, the returned tuple is the *original* tuple (i.e. it
returns value=1 instead). I think there's room for arguing that that is
the correct result; since the tuple lock acquired by FOR KEY SHARE does
not conflict with the UPDATE, what would be the reason to reject the
original tuple? In fact, in the permutation that does the
advisory_unlock before the commit, that's exactly what happens (value=1
is returned).
Also, in the repeatable read isolation level, the expected file has this
permutation as failing with "could not serialize" -- but with the patch,
it no longer fails and instead it returns the value=1 tuple instead.
Kevin, what's your opinion on that change? Is this case supposed to
raise an error or not?
I have a hard time convincing myself that it's acceptable to back-patch
such a change, in any case. I observed no other regression failure, but
what did change does make me a bit uncomfortable.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-12-18 21:00 Kevin Grittner <kgrittn@gmail.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 0 replies; 9+ messages in thread
From: Kevin Grittner @ 2015-12-18 21:00 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-bugs
On Fri, Dec 18, 2015 at 12:53 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Hmm. So one thing against this patch is that some results of existing
> isolation tests change. For instance, this one (lock-update-delete):
>
> starting permutation: s2b s1l s2u s2_blocker3 s2c s2_unlock
[I had trouble reading your summary -- redone, hopefully better]
session 2
---------
SELECT pg_advisory_lock(0);
BEGIN;
session 1
---------
SELECT * FROM foo
WHERE pg_advisory_xact_lock(0) IS NOT NULL
AND key = 1 FOR KEY SHARE;
<session 1 blocks>
UPDATE foo SET value = 2 WHERE key = 1;
UPDATE foo SET value = 2 WHERE key = 1;
SELECT pg_advisory_unlock(0);
<session 1 unblocks>
COMMIT;
> The initial SELECT is blocked and returns the updated tuple; if I patch
> as proposed above, the returned tuple is the *original* tuple (i.e. it
> returns value=1 instead). I think there's room for arguing that that is
> the correct result; since the tuple lock acquired by FOR KEY SHARE does
> not conflict with the UPDATE, what would be the reason to reject the
> original tuple?
I completely agree -- if SELECT ... FOR KEY SHARE is not blocked,
it should return the original value. I consider it a bug to do
otherwise. If it *is* blocked, then it should follow the chain and
show the latest version of the row (or not find the row visible if
it has been deleted).
> In fact, in the permutation that does the advisory_unlock before
> the commit, that's exactly what happens (value=1 is returned).
Right; why should it be different if the commit comes first?
> Also, in the repeatable read isolation level, the expected file has this
> permutation as failing with "could not serialize" -- but with the patch,
> it no longer fails and instead it returns the value=1 tuple instead.
> Kevin, what's your opinion on that change? Is this case supposed to
> raise an error or not?
At the REPEATABLE READ isolation level, you are only supposed to
get a serialization failure on a write conflict. I don't see why a
SELECT ... FOR KEY SHARE against the row should count as a write
for that purpose, unless there is a conflicting row lock (including
those acquired by DELETE or UPDATE statements). At the
SERIALIZABLE transaction isolation level there is no escaping the
fact that there can be some false positives (where a serialization
failure may occur even where it is not strictly necessary), and the
documentation mentions this; but at the REPEATABLE READ isolation
level it is arguably a bug to generate false positives. If it does
not throw an error, the only valid value to return would be 1.
I don't see a reason that a SERIALIZABLE transaction cannot behave
the same as a REPEATABLE READ transaction for this case, but at
least it would not necessarily be a bug to get a false positive
(unnecessary serialization failure) in that case.
> I have a hard time convincing myself that it's acceptable to back-patch
> such a change, in any case. I observed no other regression failure, but
> what did change does make me a bit uncomfortable.
I think it is fair to consider it a bug fix, but not necessarily
for a serious enough bug to back-patch, especially if it is
reasonable to think that people might be counting on the current
behavior. I'm not sure that I buy that anyone will be, though,
especially since a tiny shift in timing that would not generally be
controlled by the user will generate the post-patch value anyway.
It would be interesting to hear whether anyone else thinks that the
old behavior is something someone might be counting on.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2015-12-22 01:55 Olivier Dony <odo+pgbugs@odoo.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 1 reply; 9+ messages in thread
From: Olivier Dony @ 2015-12-22 01:55 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Kevin Grittner <kgrittn@gmail.com>; Olivier Dony <odo@odoo.com>; pgsql-bugs
On Fri, Dec 18, 2015 at 7:53 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
>
> Alvaro Herrera wrote:
>
> > diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
> > index 559970f..aaf8e8e 100644
> > --- a/src/backend/access/heap/heapam.c
> > +++ b/src/backend/access/heap/heapam.c
> > @@ -4005,7 +4005,7 @@ l3:
> > UnlockReleaseBuffer(*buffer);
> > elog(ERROR, "attempted to lock invisible tuple");
> > }
> > - else if (result == HeapTupleBeingUpdated)
> > + else if (result == HeapTupleBeingUpdated || result == HeapTupleUpdated)
> > {
> > TransactionId xwait;
> > uint16 infomask;
> >
> > I think heap_lock_rows had that shape (only consider BeingUpdated as a
> > reason to check/wait) only because it was possible to lock a row that
> > was being locked by someone else, but it wasn't possible to lock a row
> > that had been updated by someone else -- which became possible in 9.3.
> > So this patch is necessary, and not just to fix this one bug.
I was surprised as well to see that the initial patch, supposed to be
an optimization, would be fixing this bug.
It's starting to make more sense with your analysis.
> (...)
> I have a hard time convincing myself that it's acceptable to back-patch
> such a change, in any case. I observed no other regression failure, but
> what did change does make me a bit uncomfortable.
I'm afraid I won't be of much help to assess the consequences of the
patch, the PG source code and internal data structures are still
pretty new to me.
Would it count somehow in the balance that this use case worked fine
in 9.2, seems to be fine with regard to the documented behavior of
row-level locks and REPEATABLE READ isolation level, but suddenly
stopped working in 9.3/9.4? I suppose 9.3 is an old story, but 9.4 has
the same problem and will be around for a while, being the default
version in most LTS/stable distributions.
Thanks so much to both of you for looking further into this bug!
PS: I can confirm that the patch works, but you didn't need my confirmation ;-)
--
Olivier
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2016-07-05 22:00 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Olivier Dony <odo+pgbugs@odoo.com>
0 siblings, 1 reply; 9+ messages in thread
From: Alvaro Herrera @ 2016-07-05 22:00 UTC (permalink / raw)
To: Olivier Dony <odo+pgbugs@odoo.com>; +Cc: Kevin Grittner <kgrittn@gmail.com>; Olivier Dony <odo@odoo.com>; pgsql-bugs
I'm looking at this open bug again. I attach two commits; the first one
adds two test cases illustrating the current (broken) behavior, and the
second one adds the fix and changes the test cases to match the new
behavior. I spent a lot of time trying to find (a) any other
HeapTupleSatisfiesUpdate caller that needed to be changed, and (b)
additional test cases that showed misbehavior, and so far I haven't been
able to come up with any.
No other test case is affected AFAICS.
I intend to push this and backpatch to 9.3.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-test-cases.patch (text/x-diff, 0B) download
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
@ 2016-07-15 18:23 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 0 replies; 9+ messages in thread
From: Alvaro Herrera @ 2016-07-15 18:23 UTC (permalink / raw)
To: Olivier Dony <odo+pgbugs@odoo.com>; +Cc: Kevin Grittner <kgrittn@gmail.com>; Olivier Dony <odo@odoo.com>; pgsql-bugs
Alvaro Herrera wrote:
> I'm looking at this open bug again. I attach two commits; the first one
> adds two test cases illustrating the current (broken) behavior, and the
> second one adds the fix and changes the test cases to match the new
> behavior. I spent a lot of time trying to find (a) any other
> HeapTupleSatisfiesUpdate caller that needed to be changed, and (b)
> additional test cases that showed misbehavior, and so far I haven't been
> able to come up with any.
>
> No other test case is affected AFAICS.
>
> I intend to push this and backpatch to 9.3.
Pushed.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2016-07-15 18:23 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2015-10-14 16:48 BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request) odo@odoo.com
2015-12-17 18:31 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2015-12-17 19:34 ` Kevin Grittner <kgrittn@gmail.com>
2015-12-18 17:24 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2015-12-18 18:53 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2015-12-18 21:00 ` Kevin Grittner <kgrittn@gmail.com>
2015-12-22 01:55 ` Olivier Dony <odo+pgbugs@odoo.com>
2016-07-05 22:00 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2016-07-15 18:23 ` Alvaro Herrera <alvherre@2ndquadrant.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox