Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 4f335a3

Browse files
committed
Repair two related errors in heap_lock_tuple: it was failing to recognize
cases where we already hold the desired lock "indirectly", either via membership in a MultiXact or because the lock was originally taken by a different subtransaction of the current transaction. These cases must be accounted for to avoid needless deadlocks and/or inappropriate replacement of an exclusive lock with a shared lock. Per report from Clarence Gardner and subsequent investigation.
1 parent b6b5aa1 commit 4f335a3

File tree

4 files changed

+115
-46
lines changed

4 files changed

+115
-46
lines changed

src/backend/access/heap/heapam.c

Lines changed: 63 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.221 2006/11/05 22:42:07 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/access/heap/heapam.c,v 1.222 2006/11/17 18:00:14 tgl Exp $
1212
*
1313
*
1414
* INTERFACE ROUTINES
@@ -2359,6 +2359,8 @@ heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer,
23592359
ItemId lp;
23602360
PageHeader dp;
23612361
TransactionId xid;
2362+
TransactionId xmax;
2363+
uint16 old_infomask;
23622364
uint16 new_infomask;
23632365
LOCKMODE tuple_lock_type;
23642366
bool have_tuple_lock = false;
@@ -2395,6 +2397,25 @@ heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer,
23952397

23962398
LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
23972399

2400+
/*
2401+
* If we wish to acquire share lock, and the tuple is already
2402+
* share-locked by a multixact that includes any subtransaction of the
2403+
* current top transaction, then we effectively hold the desired lock
2404+
* already. We *must* succeed without trying to take the tuple lock,
2405+
* else we will deadlock against anyone waiting to acquire exclusive
2406+
* lock. We don't need to make any state changes in this case.
2407+
*/
2408+
if (mode == LockTupleShared &&
2409+
(infomask & HEAP_XMAX_IS_MULTI) &&
2410+
MultiXactIdIsCurrent((MultiXactId) xwait))
2411+
{
2412+
Assert(infomask & HEAP_XMAX_SHARED_LOCK);
2413+
/* Probably can't hold tuple lock here, but may as well check */
2414+
if (have_tuple_lock)
2415+
UnlockTuple(relation, tid, tuple_lock_type);
2416+
return HeapTupleMayBeUpdated;
2417+
}
2418+
23982419
/*
23992420
* Acquire tuple lock to establish our priority for the tuple.
24002421
* LockTuple will release us when we are next-in-line for the tuple.
@@ -2532,26 +2553,50 @@ heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer,
25322553
return result;
25332554
}
25342555

2556+
/*
2557+
* We might already hold the desired lock (or stronger), possibly under
2558+
* a different subtransaction of the current top transaction. If so,
2559+
* there is no need to change state or issue a WAL record. We already
2560+
* handled the case where this is true for xmax being a MultiXactId,
2561+
* so now check for cases where it is a plain TransactionId.
2562+
*
2563+
* Note in particular that this covers the case where we already hold
2564+
* exclusive lock on the tuple and the caller only wants shared lock.
2565+
* It would certainly not do to give up the exclusive lock.
2566+
*/
2567+
xmax = HeapTupleHeaderGetXmax(tuple->t_data);
2568+
old_infomask = tuple->t_data->t_infomask;
2569+
2570+
if (!(old_infomask & (HEAP_XMAX_INVALID |
2571+
HEAP_XMAX_COMMITTED |
2572+
HEAP_XMAX_IS_MULTI)) &&
2573+
(mode == LockTupleShared ?
2574+
(old_infomask & HEAP_IS_LOCKED) :
2575+
(old_infomask & HEAP_XMAX_EXCL_LOCK)) &&
2576+
TransactionIdIsCurrentTransactionId(xmax))
2577+
{
2578+
LockBuffer(*buffer, BUFFER_LOCK_UNLOCK);
2579+
/* Probably can't hold tuple lock here, but may as well check */
2580+
if (have_tuple_lock)
2581+
UnlockTuple(relation, tid, tuple_lock_type);
2582+
return HeapTupleMayBeUpdated;
2583+
}
2584+
25352585
/*
25362586
* Compute the new xmax and infomask to store into the tuple. Note we do
25372587
* not modify the tuple just yet, because that would leave it in the wrong
25382588
* state if multixact.c elogs.
25392589
*/
25402590
xid = GetCurrentTransactionId();
25412591

2542-
new_infomask = tuple->t_data->t_infomask;
2543-
2544-
new_infomask &= ~(HEAP_XMAX_COMMITTED |
2545-
HEAP_XMAX_INVALID |
2546-
HEAP_XMAX_IS_MULTI |
2547-
HEAP_IS_LOCKED |
2548-
HEAP_MOVED);
2592+
new_infomask = old_infomask & ~(HEAP_XMAX_COMMITTED |
2593+
HEAP_XMAX_INVALID |
2594+
HEAP_XMAX_IS_MULTI |
2595+
HEAP_IS_LOCKED |
2596+
HEAP_MOVED);
25492597

25502598
if (mode == LockTupleShared)
25512599
{
2552-
TransactionId xmax = HeapTupleHeaderGetXmax(tuple->t_data);
2553-
uint16 old_infomask = tuple->t_data->t_infomask;
2554-
25552600
/*
25562601
* If this is the first acquisition of a shared lock in the current
25572602
* transaction, set my per-backend OldestMemberMXactId setting. We can
@@ -2592,32 +2637,13 @@ heap_lock_tuple(Relation relation, HeapTuple tuple, Buffer *buffer,
25922637
}
25932638
else if (TransactionIdIsInProgress(xmax))
25942639
{
2595-
if (TransactionIdEquals(xmax, xid))
2596-
{
2597-
/*
2598-
* If the old locker is ourselves, we'll just mark the
2599-
* tuple again with our own TransactionId. However we
2600-
* have to consider the possibility that we had exclusive
2601-
* rather than shared lock before --- if so, be careful to
2602-
* preserve the exclusivity of the lock.
2603-
*/
2604-
if (!(old_infomask & HEAP_XMAX_SHARED_LOCK))
2605-
{
2606-
new_infomask &= ~HEAP_XMAX_SHARED_LOCK;
2607-
new_infomask |= HEAP_XMAX_EXCL_LOCK;
2608-
mode = LockTupleExclusive;
2609-
}
2610-
}
2611-
else
2612-
{
2613-
/*
2614-
* If the Xmax is a valid TransactionId, then we need to
2615-
* create a new MultiXactId that includes both the old
2616-
* locker and our own TransactionId.
2617-
*/
2618-
xid = MultiXactIdCreate(xmax, xid);
2619-
new_infomask |= HEAP_XMAX_IS_MULTI;
2620-
}
2640+
/*
2641+
* If the XMAX is a valid TransactionId, then we need to
2642+
* create a new MultiXactId that includes both the old
2643+
* locker and our own TransactionId.
2644+
*/
2645+
xid = MultiXactIdCreate(xmax, xid);
2646+
new_infomask |= HEAP_XMAX_IS_MULTI;
26212647
}
26222648
else
26232649
{

src/backend/access/transam/multixact.c

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@
4242
* Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
4343
* Portions Copyright (c) 1994, Regents of the University of California
4444
*
45-
* $PostgreSQL: pgsql/src/backend/access/transam/multixact.c,v 1.21 2006/10/04 00:29:49 momjian Exp $
45+
* $PostgreSQL: pgsql/src/backend/access/transam/multixact.c,v 1.22 2006/11/17 18:00:15 tgl Exp $
4646
*
4747
*-------------------------------------------------------------------------
4848
*/
@@ -366,7 +366,6 @@ bool
366366
MultiXactIdIsRunning(MultiXactId multi)
367367
{
368368
TransactionId *members;
369-
TransactionId myXid;
370369
int nmembers;
371370
int i;
372371

@@ -380,12 +379,14 @@ MultiXactIdIsRunning(MultiXactId multi)
380379
return false;
381380
}
382381

383-
/* checking for myself is cheap */
384-
myXid = GetTopTransactionId();
385-
382+
/*
383+
* Checking for myself is cheap compared to looking in shared memory,
384+
* so first do the equivalent of MultiXactIdIsCurrent(). This is not
385+
* needed for correctness, it's just a fast path.
386+
*/
386387
for (i = 0; i < nmembers; i++)
387388
{
388-
if (TransactionIdEquals(members[i], myXid))
389+
if (TransactionIdIsCurrentTransactionId(members[i]))
389390
{
390391
debug_elog3(DEBUG2, "IsRunning: I (%d) am running!", i);
391392
pfree(members);
@@ -416,6 +417,44 @@ MultiXactIdIsRunning(MultiXactId multi)
416417
return false;
417418
}
418419

420+
/*
421+
* MultiXactIdIsCurrent
422+
* Returns true if the current transaction is a member of the MultiXactId.
423+
*
424+
* We return true if any live subtransaction of the current top-level
425+
* transaction is a member. This is appropriate for the same reason that a
426+
* lock held by any such subtransaction is globally equivalent to a lock
427+
* held by the current subtransaction: no such lock could be released without
428+
* aborting this subtransaction, and hence releasing its locks. So it's not
429+
* necessary to add the current subxact to the MultiXact separately.
430+
*/
431+
bool
432+
MultiXactIdIsCurrent(MultiXactId multi)
433+
{
434+
bool result = false;
435+
TransactionId *members;
436+
int nmembers;
437+
int i;
438+
439+
nmembers = GetMultiXactIdMembers(multi, &members);
440+
441+
if (nmembers < 0)
442+
return false;
443+
444+
for (i = 0; i < nmembers; i++)
445+
{
446+
if (TransactionIdIsCurrentTransactionId(members[i]))
447+
{
448+
result = true;
449+
break;
450+
}
451+
}
452+
453+
pfree(members);
454+
455+
return result;
456+
}
457+
419458
/*
420459
* MultiXactIdSetOldestMember
421460
* Save the oldest MultiXactId this transaction could be a member of.

src/backend/utils/time/tqual.c

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
* Portions Copyright (c) 1994, Regents of the University of California
3333
*
3434
* IDENTIFICATION
35-
* $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.99 2006/11/05 22:42:09 tgl Exp $
35+
* $PostgreSQL: pgsql/src/backend/utils/time/tqual.c,v 1.100 2006/11/17 18:00:15 tgl Exp $
3636
*
3737
*-------------------------------------------------------------------------
3838
*/
@@ -511,7 +511,10 @@ HeapTupleSatisfiesToast(HeapTupleHeader tuple, Buffer buffer)
511511
* HeapTupleUpdated: The tuple was updated by a committed transaction.
512512
*
513513
* HeapTupleBeingUpdated: The tuple is being updated by an in-progress
514-
* transaction other than the current transaction.
514+
* transaction other than the current transaction. (Note: this includes
515+
* the case where the tuple is share-locked by a MultiXact, even if the
516+
* MultiXact includes the current transaction. Callers that want to
517+
* distinguish that case must test for it themselves.)
515518
*/
516519
HTSU_Result
517520
HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid,

src/include/access/multixact.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* Portions Copyright (c) 1996-2006, PostgreSQL Global Development Group
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
9-
* $PostgreSQL: pgsql/src/include/access/multixact.h,v 1.10 2006/03/24 04:32:13 tgl Exp $
9+
* $PostgreSQL: pgsql/src/include/access/multixact.h,v 1.11 2006/11/17 18:00:15 tgl Exp $
1010
*/
1111
#ifndef MULTIXACT_H
1212
#define MULTIXACT_H
@@ -45,6 +45,7 @@ typedef struct xl_multixact_create
4545
extern MultiXactId MultiXactIdCreate(TransactionId xid1, TransactionId xid2);
4646
extern MultiXactId MultiXactIdExpand(MultiXactId multi, TransactionId xid);
4747
extern bool MultiXactIdIsRunning(MultiXactId multi);
48+
extern bool MultiXactIdIsCurrent(MultiXactId multi);
4849
extern void MultiXactIdWait(MultiXactId multi);
4950
extern bool ConditionalMultiXactIdWait(MultiXactId multi);
5051
extern void MultiXactIdSetOldestMember(void);

0 commit comments

Comments
 (0)