diff options
| -rw-r--r-- | src/backend/storage/lmgr/lmgr.c | 3 | ||||
| -rw-r--r-- | src/backend/storage/lmgr/lock.c | 43 | ||||
| -rw-r--r-- | src/include/storage/lock.h | 18 | ||||
| -rw-r--r-- | src/test/isolation/Makefile | 11 | ||||
| -rw-r--r-- | src/test/isolation/README | 6 | ||||
| -rw-r--r-- | src/test/isolation/expected/prepared-transactions-cic.out | 18 | ||||
| -rw-r--r-- | src/test/isolation/specs/prepared-transactions-cic.spec | 37 |
7 files changed, 98 insertions, 38 deletions
diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index 7b87002d7ce..f049f26ca96 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -846,8 +846,7 @@ WaitForLockersMultiple(List *locktags, LOCKMODE lockmode) /* * Note: GetLockConflicts() never reports our own xid, hence we need not - * check for that. Also, prepared xacts are not reported, which is fine - * since they certainly aren't going to do anything anymore. + * check for that. Also, prepared xacts are reported and awaited. */ /* Finally wait for each such transaction to complete */ diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 98a2f366b4d..1c5ab8c0d97 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -2791,9 +2791,7 @@ FastPathGetRelationLockEntry(LOCALLOCK *locallock) * so use of this function has to be thought about carefully. * * Note we never include the current xact's vxid in the result array, - * since an xact never blocks itself. Also, prepared transactions are - * ignored, which is a bit more debatable but is appropriate for current - * uses of the result. + * since an xact never blocks itself. */ VirtualTransactionId * GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode) @@ -2818,19 +2816,21 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode) /* * Allocate memory to store results, and fill with InvalidVXID. We only - * need enough space for MaxBackends + a terminator, since prepared xacts - * don't count. InHotStandby allocate once in TopMemoryContext. + * need enough space for MaxBackends + max_prepared_xacts + a terminator. + * InHotStandby allocate once in TopMemoryContext. */ if (InHotStandby) { if (vxids == NULL) vxids = (VirtualTransactionId *) MemoryContextAlloc(TopMemoryContext, - sizeof(VirtualTransactionId) * (MaxBackends + 1)); + sizeof(VirtualTransactionId) * + (MaxBackends + max_prepared_xacts + 1)); } else vxids = (VirtualTransactionId *) - palloc0(sizeof(VirtualTransactionId) * (MaxBackends + 1)); + palloc0(sizeof(VirtualTransactionId) * + (MaxBackends + max_prepared_xacts + 1)); /* Compute hash code and partition lock, and look up conflicting modes. */ hashcode = LockTagHashCode(locktag); @@ -2905,13 +2905,9 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode) /* Conflict! */ GET_VXID_FROM_PGPROC(vxid, *proc); - /* - * If we see an invalid VXID, then either the xact has already - * committed (or aborted), or it's a prepared xact. In either - * case we may ignore it. - */ if (VirtualTransactionIdIsValid(vxid)) vxids[count++] = vxid; + /* else, xact already committed or aborted */ /* No need to examine remaining slots. */ break; @@ -2968,11 +2964,6 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode) GET_VXID_FROM_PGPROC(vxid, *proc); - /* - * If we see an invalid VXID, then either the xact has already - * committed (or aborted), or it's a prepared xact. In either - * case we may ignore it. - */ if (VirtualTransactionIdIsValid(vxid)) { int i; @@ -2984,6 +2975,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode) if (i >= fast_count) vxids[count++] = vxid; } + /* else, xact already committed or aborted */ } } @@ -2993,7 +2985,7 @@ GetLockConflicts(const LOCKTAG *locktag, LOCKMODE lockmode) LWLockRelease(partitionLock); - if (count > MaxBackends) /* should never happen */ + if (count > MaxBackends + max_prepared_xacts) /* should never happen */ elog(PANIC, "too many conflicting locks found"); vxids[count].backendId = InvalidBackendId; @@ -4349,6 +4341,21 @@ VirtualXactLock(VirtualTransactionId vxid, bool wait) Assert(VirtualTransactionIdIsValid(vxid)); + if (VirtualTransactionIdIsPreparedXact(vxid)) + { + LockAcquireResult lar; + + /* + * Prepared transactions don't hold vxid locks. The + * LocalTransactionId is always a normal, locked XID. + */ + SET_LOCKTAG_TRANSACTION(tag, vxid.localTransactionId); + lar = LockAcquire(&tag, ShareLock, false, !wait); + if (lar != LOCKACQUIRE_NOT_AVAIL) + LockRelease(&tag, ShareLock, false); + return lar != LOCKACQUIRE_NOT_AVAIL; + } + SET_LOCKTAG_VIRTUALTRANSACTION(tag, vxid); /* diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h index 6071a756006..573c91941a0 100644 --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -47,10 +47,10 @@ extern bool Debug_deadlocks; /* * Top-level transactions are identified by VirtualTransactionIDs comprising - * the BackendId of the backend running the xact, plus a locally-assigned - * LocalTransactionId. These are guaranteed unique over the short term, - * but will be reused after a database restart; hence they should never - * be stored on disk. + * PGPROC fields backendId and lxid. For prepared transactions, the + * LocalTransactionId is an ordinary XID. These are guaranteed unique over + * the short term, but will be reused after a database restart or XID + * wraparound; hence they should never be stored on disk. * * Note that struct VirtualTransactionId can not be assumed to be atomically * assignable as a whole. However, type LocalTransactionId is assumed to @@ -62,16 +62,16 @@ extern bool Debug_deadlocks; */ typedef struct { - BackendId backendId; /* determined at backend startup */ - LocalTransactionId localTransactionId; /* backend-local transaction - * id */ + BackendId backendId; /* backendId from PGPROC */ + LocalTransactionId localTransactionId; /* lxid from PGPROC */ } VirtualTransactionId; #define InvalidLocalTransactionId 0 #define LocalTransactionIdIsValid(lxid) ((lxid) != InvalidLocalTransactionId) #define VirtualTransactionIdIsValid(vxid) \ - (((vxid).backendId != InvalidBackendId) && \ - LocalTransactionIdIsValid((vxid).localTransactionId)) + (LocalTransactionIdIsValid((vxid).localTransactionId)) +#define VirtualTransactionIdIsPreparedXact(vxid) \ + ((vxid).backendId == InvalidBackendId) #define VirtualTransactionIdEquals(vxid1, vxid2) \ ((vxid1).backendId == (vxid2).backendId && \ (vxid1).localTransactionId == (vxid2).localTransactionId) diff --git a/src/test/isolation/Makefile b/src/test/isolation/Makefile index d87e19adfb9..ab80409e12c 100644 --- a/src/test/isolation/Makefile +++ b/src/test/isolation/Makefile @@ -48,12 +48,11 @@ installcheck: all check: all $(with_temp_install) ./pg_isolation_regress --temp-instance=./tmp_check $(TEMP_CONF) --inputdir=$(srcdir) --bindir= $(EXTRA_REGRESS_OPTS) --schedule=$(srcdir)/isolation_schedule -# Versions of the check tests that include the prepared_transactions test -# It only makes sense to run these if set up to use prepared transactions, -# via TEMP_CONFIG for the check case, or via the postgresql.conf for the -# installcheck case. +# Non-default tests. It only makes sense to run these if set up to use +# prepared transactions, via TEMP_CONFIG for the check case, or via the +# postgresql.conf for the installcheck case. installcheck-prepared-txns: all temp-install - ./pg_isolation_regress --bindir='$(bindir)' $(EXTRA_REGRESS_OPTS) --inputdir=$(srcdir) --schedule=$(srcdir)/isolation_schedule prepared-transactions + ./pg_isolation_regress --bindir='$(bindir)' $(EXTRA_REGRESS_OPTS) --inputdir=$(srcdir) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic check-prepared-txns: all temp-install - $(with_temp_install) ./pg_isolation_regress --temp-instance=./tmp_check $(TEMP_CONF) --inputdir=$(srcdir) --bindir= $(EXTRA_REGRESS_OPTS) --schedule=$(srcdir)/isolation_schedule prepared-transactions + $(with_temp_install) ./pg_isolation_regress --temp-instance=./tmp_check $(TEMP_CONF) --inputdir=$(srcdir) --bindir= $(EXTRA_REGRESS_OPTS) --schedule=$(srcdir)/isolation_schedule prepared-transactions prepared-transactions-cic diff --git a/src/test/isolation/README b/src/test/isolation/README index 217953d1834..6ae71523258 100644 --- a/src/test/isolation/README +++ b/src/test/isolation/README @@ -23,9 +23,9 @@ you can do something like ./pg_isolation_regress fk-contention fk-deadlock (look into the specs/ subdirectory to see the available tests). -The prepared-transactions test requires the server's -max_prepared_transactions parameter to be set to at least 3; therefore it -is not run by default. To include it in the test run, use +Certain tests require the server's max_prepared_transactions parameter to be +set to at least 3; therefore they are not run by default. To include them in +the test run, use make check-prepared-txns or make installcheck-prepared-txns diff --git a/src/test/isolation/expected/prepared-transactions-cic.out b/src/test/isolation/expected/prepared-transactions-cic.out new file mode 100644 index 00000000000..043ec3c3636 --- /dev/null +++ b/src/test/isolation/expected/prepared-transactions-cic.out @@ -0,0 +1,18 @@ +Parsed test spec with 2 sessions + +starting permutation: w1 p1 cic2 c1 r2 +step w1: BEGIN; INSERT INTO cic_test VALUES (1); +step p1: PREPARE TRANSACTION 's1'; +step cic2: + CREATE INDEX CONCURRENTLY on cic_test(a); + +ERROR: canceling statement due to lock timeout +step c1: COMMIT PREPARED 's1'; +step r2: + SET enable_seqscan to off; + SET enable_bitmapscan to off; + SELECT * FROM cic_test WHERE a = 1; + +a + +1 diff --git a/src/test/isolation/specs/prepared-transactions-cic.spec b/src/test/isolation/specs/prepared-transactions-cic.spec new file mode 100644 index 00000000000..c39eaf5ad0c --- /dev/null +++ b/src/test/isolation/specs/prepared-transactions-cic.spec @@ -0,0 +1,37 @@ +# This test verifies that CREATE INDEX CONCURRENTLY interacts with prepared +# transactions correctly. +setup +{ + CREATE TABLE cic_test (a int); +} + +teardown +{ + DROP TABLE cic_test; +} + + +# Sessions for CREATE INDEX CONCURRENTLY test +session "s1" +step "w1" { BEGIN; INSERT INTO cic_test VALUES (1); } +step "p1" { PREPARE TRANSACTION 's1'; } +step "c1" { COMMIT PREPARED 's1'; } + +session "s2" +# The isolation tester never recognizes that a lock of s1 blocks s2, because a +# prepared transaction's locks have no pid associated. While there's a slight +# chance of timeout while waiting for an autovacuum-held lock, that wouldn't +# change the output. Hence, no timeout is too short. +setup { SET lock_timeout = 10; } +step "cic2" +{ + CREATE INDEX CONCURRENTLY on cic_test(a); +} +step "r2" +{ + SET enable_seqscan to off; + SET enable_bitmapscan to off; + SELECT * FROM cic_test WHERE a = 1; +} + +permutation "w1" "p1" "cic2" "c1" "r2" |