From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: odo@odoo.com
Cc: pgsql-bugs@postgresql.org
Subject: Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
Date: Thu, 17 Dec 2015 15:31:53 -0300
Message-ID: <20151217183153.GO2618@alvherre.pgsql> (raw)
In-Reply-To: <20151014164844.3019.25750@wrigleys.postgresql.org>
References: <20151014164844.3019.25750@wrigleys.postgresql.org>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-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-bugsAttachments: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.cindex 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.cindex 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.cindex 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.cindex 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.hindex 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.outnew file mode 100644index 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_scheduleindex 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.specnew file mode 100644index 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: alvherre@2ndquadrant.com, odo@odoo.com
Subject: Re: BUG #13681: Serialization failures caused by new multixact code of 9.3 (back-patch request)
In-Reply-To: <20151217183153.GO2618@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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