Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 86a5b30

Browse filesBrowse 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 2a01bc2 commit 86a5b30
Copy full SHA for 86a5b30

File tree

Expand file treeCollapse file tree

7 files changed

+98
-37
lines changed
Filter options
Expand file treeCollapse file tree

7 files changed

+98
-37
lines changed

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

Copy file name to clipboardExpand all lines: src/backend/storage/lmgr/lmgr.c
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -904,8 +904,7 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode, bool progress)
904904

905905
/*
906906
* Note: GetLockConflicts() never reports our own xid, hence we need not
907-
* check for that. Also, prepared xacts are not reported, which is fine
908-
* since they certainly aren't going to do anything anymore.
907+
* check for that. Also, prepared xacts are reported and awaited.
909908
*/
910909

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

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

Copy file name to clipboardExpand all lines: src/backend/storage/lmgr/lock.c
+25-18Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2904,9 +2904,7 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock)
29042904
* so use of this function has to be thought about carefully.
29052905
*
29062906
* Note we never include the current xact's vxid in the result array,
2907-
* since an xact never blocks itself. Also, prepared transactions are
2908-
* ignored, which is a bit more debatable but is appropriate for current
2909-
* uses of the result.
2907+
* since an xact never blocks itself.
29102908
*/
29112909
VirtualTransactionId *
29122910
GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
@@ -2931,19 +2929,21 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
29312929

29322930
/*
29332931
* Allocate memory to store results, and fill with InvalidVXID. We only
2934-
* need enough space for MaxBackends + a terminator, since prepared xacts
2935-
* don't count. InHotStandby allocate once in TopMemoryContext.
2932+
* need enough space for MaxBackends + max_prepared_xacts + a terminator.
2933+
* InHotStandby allocate once in TopMemoryContext.
29362934
*/
29372935
if (InHotStandby)
29382936
{
29392937
if (vxids == NULL)
29402938
vxids = (VirtualTransactionId *)
29412939
MemoryContextAlloc(TopMemoryContext,
2942-
sizeof(VirtualTransactionId) * (MaxBackends + 1));
2940+
sizeof(VirtualTransactionId) *
2941+
(MaxBackends + max_prepared_xacts + 1));
29432942
}
29442943
else
29452944
vxids = (VirtualTransactionId *)
2946-
palloc0(sizeof(VirtualTransactionId) * (MaxBackends + 1));
2945+
palloc0(sizeof(VirtualTransactionId) *
2946+
(MaxBackends + max_prepared_xacts + 1));
29472947

29482948
/* Compute hash code and partition lock, and look up conflicting modes. */
29492949
hashcode = LockTagHashCode(locktag);
@@ -3018,13 +3018,9 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
30183018
/* Conflict! */
30193019
GET_VXID_FROM_PGPROC(vxid, *proc);
30203020

3021-
/*
3022-
* If we see an invalid VXID, then either the xact has already
3023-
* committed (or aborted), or it's a prepared xact. In either
3024-
* case we may ignore it.
3025-
*/
30263021
if (VirtualTransactionIdIsValid(vxid))
30273022
vxids[count++] = vxid;
3023+
/* else, xact already committed or aborted */
30283024

30293025
/* No need to examine remaining slots. */
30303026
break;
@@ -3083,11 +3079,6 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
30833079

30843080
GET_VXID_FROM_PGPROC(vxid, *proc);
30853081

3086-
/*
3087-
* If we see an invalid VXID, then either the xact has already
3088-
* committed (or aborted), or it's a prepared xact. In either
3089-
* case we may ignore it.
3090-
*/
30913082
if (VirtualTransactionIdIsValid(vxid))
30923083
{
30933084
int i;
@@ -3099,6 +3090,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
30993090
if (i >= fast_count)
31003091
vxids[count++] = vxid;
31013092
}
3093+
/* else, xact already committed or aborted */
31023094
}
31033095
}
31043096

@@ -3108,7 +3100,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode, int *countp)
31083100

31093101
LWLockRelease(partitionLock);
31103102

3111-
if (count > MaxBackends) /* should never happen */
3103+
if (count > MaxBackends + max_prepared_xacts) /* should never happen */
31123104
elog(PANIC, "too many conflicting locks found");
31133105

31143106
vxids[count].backendId = InvalidBackendId;
@@ -4466,6 +4458,21 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait)
44664458

44674459
Assert(VirtualTransactionIdIsValid(vxid));
44684460

4461+
if (VirtualTransactionIdIsPreparedXact(vxid))
4462+
{
4463+
LockAcquireResult lar;
4464+
4465+
/*
4466+
* Prepared transactions don't hold vxid locks. The
4467+
* LocalTransactionId is always a normal, locked XID.
4468+
*/
4469+
SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId);
4470+
lar = LockAcquire(&tag, ShareLock, false, !wait);
4471+
if (lar != LOCKACQUIRE_NOT_AVAIL)
4472+
LockRelease(&tag, ShareLock, false);
4473+
return lar != LOCKACQUIRE_NOT_AVAIL;
4474+
}
4475+
44694476
SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid);
44704477

44714478
/*

‎src/include/storage/lock.h

Copy file name to clipboardExpand all lines: src/include/storage/lock.h
+9-8Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,10 @@ extern bool Debug_deadlocks;
4646

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

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

‎src/test/isolation/Makefile

Copy file name to clipboardExpand all lines: src/test/isolation/Makefile
+5-6Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,11 @@ installcheck: all
5858
check: all
5959
$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule
6060

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

6867
check-prepared-txns: all temp-install
69-
$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions
68+
$(pg_isolation_regress_check) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic

‎src/test/isolation/README

Copy file name to clipboardExpand all lines: src/test/isolation/README
+3-3Lines 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

Copy file name to clipboard
+18Lines 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

Copy file name to clipboard
+37Lines 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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.