Skip to content

Commit d1ab4bf

Browse files
nmischnmisch
committed
Fix CREATE INDEX CONCURRENTLY for simultaneous prepared transactions.
In a cluster having used CREATE INDEX CONCURRENTLY while having enabled prepared transactions, queries that use the resulting index can silently fail to find rows. Fix this for future CREATE INDEX CONCURRENTLY by making it wait for prepared transactions like it waits for ordinary transactions. This expands the VirtualTransactionId structure domain to admit prepared transactions. It may be necessary to reindex to recover from past occurrences. Back-patch to 9.5 (all supported versions). Andrey Borodin, reviewed (in earlier versions) by Tom Lane and Michael Paquier. Discussion: https://postgr.es/m/2E712143-97F7-4890-B470-4A35142ABC82@yandex-team.ru
1 parent 289ef38 commit d1ab4bf

File tree

7 files changed

+98
-37
lines changed

7 files changed

+98
-37
lines changed

‎src/backend/storage/lmgr/lmgr.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -846,8 +846,7 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode)
846846

847847
/*
848848
* Note: GetLockConflicts() never reports our own xid, hence we need not
849-
* check for that. Also, prepared xacts are not reported, which is fine
850-
* since they certainly aren't going to do anything anymore.
849+
* check for that. Also, prepared xacts are reported and awaited.
851850
*/
852851

853852
/* Finally wait for each such transaction to complete */

‎src/backend/storage/lmgr/lock.c

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2788,9 +2788,7 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock)
27882788
* so use of this function has to be thought about carefully.
27892789
*
27902790
* Note we never include the current xact's vxid in the result array,
2791-
* since an xact never blocks itself. Also, prepared transactions are
2792-
* ignored, which is a bit more debatable but is appropriate for current
2793-
* uses of the result.
2791+
* since an xact never blocks itself.
27942792
*/
27952793
VirtualTransactionId *
27962794
GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
@@ -2815,19 +2813,21 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
28152813

28162814
/*
28172815
* Allocate memory to store results, and fill with InvalidVXID. We only
2818-
* need enough space for MaxBackends + a terminator, since prepared xacts
2819-
* don't count. InHotStandby allocate once in TopMemoryContext.
2816+
* need enough space for MaxBackends + max_prepared_xacts + a terminator.
2817+
* InHotStandby allocate once in TopMemoryContext.
28202818
*/
28212819
if (InHotStandby)
28222820
{
28232821
if (vxids == NULL)
28242822
vxids = (VirtualTransactionId *)
28252823
MemoryContextAlloc(TopMemoryContext,
2826-
sizeof(VirtualTransactionId) * (MaxBackends + 1));
2824+
sizeof(VirtualTransactionId) *
2825+
(MaxBackends + max_prepared_xacts + 1));
28272826
}
28282827
else
28292828
vxids = (VirtualTransactionId *)
2830-
palloc0(sizeof(VirtualTransactionId) * (MaxBackends + 1));
2829+
palloc0(sizeof(VirtualTransactionId) *
2830+
(MaxBackends + max_prepared_xacts + 1));
28312831

28322832
/* Compute hash code and partition lock, and look up conflicting modes. */
28332833
hashcode = LockTagHashCode(locktag);
@@ -2902,13 +2902,9 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
29022902
/* Conflict! */
29032903
GET_VXID_FROM_PGPROC(vxid, *proc);
29042904

2905-
/*
2906-
* If we see an invalid VXID, then either the xact has already
2907-
* committed (or aborted), or it's a prepared xact. In either
2908-
* case we may ignore it.
2909-
*/
29102905
if (VirtualTransactionIdIsValid(vxid))
29112906
vxids[count++] = vxid;
2907+
/* else, xact already committed or aborted */
29122908

29132909
/* No need to examine remaining slots. */
29142910
break;
@@ -2965,11 +2961,6 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
29652961

29662962
GET_VXID_FROM_PGPROC(vxid, *proc);
29672963

2968-
/*
2969-
* If we see an invalid VXID, then either the xact has already
2970-
* committed (or aborted), or it's a prepared xact. In either
2971-
* case we may ignore it.
2972-
*/
29732964
if (VirtualTransactionIdIsValid(vxid))
29742965
{
29752966
int i;
@@ -2981,6 +2972,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
29812972
if (i >= fast_count)
29822973
vxids[count++] = vxid;
29832974
}
2975+
/* else, xact already committed or aborted */
29842976
}
29852977
}
29862978

@@ -2990,7 +2982,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode)
29902982

29912983
LWLockRelease(partitionLock);
29922984

2993-
if (count > MaxBackends) /* should never happen */
2985+
if (count > MaxBackends + max_prepared_xacts) /* should never happen */
29942986
elog(PANIC, "too many conflicting locks found");
29952987

29962988
vxids[count].backendId = InvalidBackendId;
@@ -4346,6 +4338,21 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
43464338

43474339
Assert(VirtualTransactionIdIsValid(vxid));
43484340

4341+
if (VirtualTransactionIdIsPreparedXact(vxid))
4342+
{
4343+
LockAcquireResult lar;
4344+
4345+
/*
4346+
* Prepared transactions don't hold vxid locks. The
4347+
* LocalTransactionId is always a normal, locked XID.
4348+
*/
4349+
SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
4350+
lar = LockAcquire(&tag, ShareLock, false, !wait);
4351+
if (lar != LOCKACQUIRE_NOT_AVAIL)
4352+
LockRelease(&tag, ShareLock, false);
4353+
return lar != LOCKACQUIRE_NOT_AVAIL;
4354+
}
4355+
43494356
SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
43504357

43514358
/*

‎src/include/storage/lock.h

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,10 @@ extern bool Debug_deadlocks;
4747

4848
/*
4949
* Top-level transactions are identified by VirtualTransactionIDs comprising
50-
* the BackendId of the backend running the xact, plus a locally-assigned
51-
* LocalTransactionId. These are guaranteed unique over the short term,
52-
* but will be reused after a database restart; hence they should never
53-
* be stored on disk.
50+
* PGPROC fields backendId and lxid. For prepared transactions, the
51+
* LocalTransactionId is an ordinary XID. These are guaranteed unique over
52+
* the short term, but will be reused after a database restart or XID
53+
* wraparound; hence they should never be stored on disk.
5454
*
5555
* Note that struct VirtualTransactionId can not be assumed to be atomically
5656
* assignable as a whole. However, type LocalTransactionId is assumed to
@@ -62,15 +62,16 @@ extern bool Debug_deadlocks;
6262
*/
6363
typedef struct
6464
{
65-
BackendId backendId; /* determined at backend startup */
66-
LocalTransactionId localTransactionId; /* backend-local transaction id */
65+
BackendId backendId; /* backendId from PGPROC */
66+
LocalTransactionId localTransactionId; /* lxid from PGPROC */
6767
} VirtualTransactionId;
6868

6969
#define InvalidLocalTransactionId 0
7070
#define LocalTransactionIdIsValid(lxid) ((lxid) != InvalidLocalTransactionId)
7171
#define VirtualTransactionIdIsValid(vxid) \
72-
(((vxid).backendId != InvalidBackendId) && \
73-
LocalTransactionIdIsValid((vxid).localTransactionId))
72+
(LocalTransactionIdIsValid((vxid).localTransactionId))
73+
#define VirtualTransactionIdIsPreparedXact(vxid) \
74+
((vxid).backendId == InvalidBackendId)
7475
#define VirtualTransactionIdEquals(vxid1, vxid2) \
7576
((vxid1).backendId == (vxid2).backendId && \
7677
(vxid1).localTransactionId == (vxid2).localTransactionId)

‎src/test/isolation/Makefile

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,11 @@ installcheck: all
5555
check: all
5656
$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule
5757

58-
# Versions of the check tests that include the prepared_transactions test
59-
# It only makes sense to run these if set up to use prepared transactions,
60-
# via TEMP_CONFIG for the check case, or via the postgresql.conf for the
61-
# installcheck case.
58+
# Non-default tests. It only makes sense to run these if set up to use
59+
# prepared transactions, via TEMP_CONFIG for the check case, or via the
60+
# postgresql.conf for the installcheck case.
6261
installcheck-prepared-txns: all temp-install
63-
$(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule prepared-transactions
62+
$(pg_isolation_regress_installcheck) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic
6463

6564
check-prepared-txns: all temp-install
66-
$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions
65+
$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic

‎src/test/isolation/README

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ you can do something like
2323
./pg_isolation_regress fk-contention fk-deadlock
2424
(look into the specs/ subdirectory to see the available tests).
2525

26-
The prepared-transactions test requires the server's
27-
max_prepared_transactions parameter to be set to at least 3; therefore it
28-
is not run by default. To include it in the test run, use
26+
Certain tests require the server's max_prepared_transactions parameter to be
27+
set to at least 3; therefore they are not run by default. To include them in
28+
the test run, use
2929
make check-prepared-txns
3030
or
3131
make installcheck-prepared-txns

‎src/test/isolation/expected/prepared-transactions-cic.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
Parsed test spec with 2 sessions
2+
3+
starting permutation: w1 p1 cic2 c1 r2
4+
step w1: BEGIN; INSERT INTO cic_test VALUES (1);
5+
step p1: PREPARE TRANSACTION 's1';
6+
step cic2:
7+
CREATE INDEX CONCURRENTLY on cic_test(a);
8+
9+
ERROR: canceling statement due to lock timeout
10+
step c1: COMMIT PREPARED 's1';
11+
step r2:
12+
SET enable_seqscan to off;
13+
SET enable_bitmapscan to off;
14+
SELECT * FROM cic_test WHERE a = 1;
15+
16+
a
17+
18+
1

‎src/test/isolation/specs/prepared-transactions-cic.spec

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
# This test verifies that CREATE INDEX CONCURRENTLY interacts with prepared
2+
# transactions correctly.
3+
setup
4+
{
5+
CREATE TABLE cic_test (a int);
6+
}
7+
8+
teardown
9+
{
10+
DROP TABLE cic_test;
11+
}
12+
13+
14+
# Sessions for CREATE INDEX CONCURRENTLY test
15+
session "s1"
16+
step "w1" { BEGIN; INSERT INTO cic_test VALUES (1); }
17+
step "p1" { PREPARE TRANSACTION 's1'; }
18+
step "c1" { COMMIT PREPARED 's1'; }
19+
20+
session "s2"
21+
# The isolation tester never recognizes that a lock of s1 blocks s2, because a
22+
# prepared transaction's locks have no pid associated. While there's a slight
23+
# chance of timeout while waiting for an autovacuum-held lock, that wouldn't
24+
# change the output. Hence, no timeout is too short.
25+
setup { SET lock_timeout = 10; }
26+
step "cic2"
27+
{
28+
CREATE INDEX CONCURRENTLY on cic_test(a);
29+
}
30+
step "r2"
31+
{
32+
SET enable_seqscan to off;
33+
SET enable_bitmapscan to off;
34+
SELECT * FROM cic_test WHERE a = 1;
35+
}
36+
37+
permutation "w1" "p1" "cic2" "c1" "r2"

0 commit comments

Comments
 (0)