From f8dc1985fd390774aab4ab0ba71036d6d5e631a9 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Tue, 9 May 2017 23:35:31 -0400 Subject: [PATCH] Fix ALTER SEQUENCE locking In 1753b1b027035029c2a2a1649065762fafbf63f3, the pg_sequence system catalog was introduced. This made sequence metadata changes transactional, while the actual sequence values are still behaving nontransactionally. This requires some refinement in how ALTER SEQUENCE, which operates on both, locks the sequence and the catalog. The main problems were: - Concurrent ALTER SEQUENCE causes "tuple concurrently updated" error, caused by updates to pg_sequence catalog. - Sequence WAL writes and catalog updates are not protected by same lock, which could lead to inconsistent recovery order. - nextval() disregarding uncommitted ALTER SEQUENCE changes. To fix, nextval() and friends now lock the sequence using RowExclusiveLock instead of AccessShareLock. ALTER SEQUENCE locks the sequence using ShareRowExclusiveLock. This means that nextval() and ALTER SEQUENCE block each other, and ALTER SEQUENCE on the same sequence blocks itself. (This was already the case previously for the OWNER TO, RENAME, and SET SCHEMA variants.) Also, rearrange some code so that the entire AlterSequence is protected by the lock on the sequence. As an exception, use reduced locking for ALTER SEQUENCE ... RESTART. Since that is basically a setval(), it does not require the full locking of other ALTER SEQUENCE actions. So check whether we are only running a RESTART and run with less locking if so. Reviewed-by: Michael Paquier Reported-by: Jason Petersen Reported-by: Andres Freund --- doc/src/sgml/ref/alter_sequence.sgml | 7 ++ src/backend/commands/sequence.c | 69 +++++++++++----- src/test/isolation/expected/sequence-ddl.out | 85 ++++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/sequence-ddl.spec | 39 +++++++++ 5 files changed, 180 insertions(+), 21 deletions(-) create mode 100644 src/test/isolation/expected/sequence-ddl.out create mode 100644 src/test/isolation/specs/sequence-ddl.spec diff --git a/doc/src/sgml/ref/alter_sequence.sgml b/doc/src/sgml/ref/alter_sequence.sgml index 3fb3400d6b..30e5316b8c 100644 --- a/doc/src/sgml/ref/alter_sequence.sgml +++ b/doc/src/sgml/ref/alter_sequence.sgml @@ -304,6 +304,13 @@ ALTER SEQUENCE [ IF EXISTS ] name S 8.3, it sometimes did.) + + ALTER SEQUENCE blocks + concurrent nextval, currval, + lastval, and setval calls, except + if only the RESTART clause is used. + + For historical reasons, ALTER TABLE can be used with sequences too; but the only variants of ALTER TABLE diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index 4a56926b53..0f7cf1dce8 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -93,11 +93,12 @@ static HTAB *seqhashtab = NULL; /* hash table for SeqTable items */ static SeqTableData *last_used_seq = NULL; static void fill_seq_with_data(Relation rel, HeapTuple tuple); -static Relation open_share_lock(SeqTable seq); +static Relation lock_and_open_sequence(SeqTable seq); static void create_seq_hashtable(void); static void init_sequence(Oid relid, SeqTable *p_elm, Relation *p_rel); static Form_pg_sequence_data read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple); +static LOCKMODE alter_sequence_get_lock_level(List *options); static void init_params(ParseState *pstate, List *options, bool for_identity, bool isInit, Form_pg_sequence seqform, @@ -427,7 +428,9 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) HeapTuple tuple; /* Open and lock sequence. */ - relid = RangeVarGetRelid(stmt->sequence, AccessShareLock, stmt->missing_ok); + relid = RangeVarGetRelid(stmt->sequence, + alter_sequence_get_lock_level(stmt->options), + stmt->missing_ok); if (relid == InvalidOid) { ereport(NOTICE, @@ -443,12 +446,6 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS, stmt->sequence->relname); - /* lock page' buffer and read tuple into new sequence structure */ - seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple); - - /* Copy old sequence data into workspace */ - memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data)); - rel = heap_open(SequenceRelationId, RowExclusiveLock); tuple = SearchSysCacheCopy1(SEQRELID, ObjectIdGetDatum(relid)); @@ -458,6 +455,12 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) seqform = (Form_pg_sequence) GETSTRUCT(tuple); + /* lock page's buffer and read tuple into new sequence structure */ + seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple); + + /* Copy old sequence data into workspace */ + memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data)); + /* Check and set new values */ init_params(pstate, stmt->options, stmt->for_identity, false, seqform, &changed_seqform, &newseqdata, &owned_by); @@ -508,12 +511,12 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) ObjectAddressSet(address, RelationRelationId, relid); - relation_close(seqrel, NoLock); - if (changed_seqform) CatalogTupleUpdate(rel, &tuple->t_self, tuple); heap_close(rel, RowExclusiveLock); + relation_close(seqrel, NoLock); + return address; } @@ -594,7 +597,7 @@ nextval_internal(Oid relid, bool check_permissions) bool cycle; bool logit = false; - /* open and AccessShareLock sequence */ + /* open and lock sequence */ init_sequence(relid, &elm, &seqrel); if (check_permissions && @@ -829,7 +832,7 @@ currval_oid(PG_FUNCTION_ARGS) SeqTable elm; Relation seqrel; - /* open and AccessShareLock sequence */ + /* open and lock sequence */ init_sequence(relid, &elm, &seqrel); if (pg_class_aclcheck(elm->relid, GetUserId(), @@ -869,7 +872,7 @@ lastval(PG_FUNCTION_ARGS) (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg("lastval is not yet defined in this session"))); - seqrel = open_share_lock(last_used_seq); + seqrel = lock_and_open_sequence(last_used_seq); /* nextval() must have already been called for this sequence */ Assert(last_used_seq->last_valid); @@ -913,7 +916,7 @@ do_setval(Oid relid, int64 next, bool iscalled) int64 maxv, minv; - /* open and AccessShareLock sequence */ + /* open and lock sequence */ init_sequence(relid, &elm, &seqrel); if (pg_class_aclcheck(elm->relid, GetUserId(), ACL_UPDATE) != ACLCHECK_OK) @@ -1042,15 +1045,15 @@ setval3_oid(PG_FUNCTION_ARGS) /* - * Open the sequence and acquire AccessShareLock if needed + * Open the sequence and acquire lock if needed * * If we haven't touched the sequence already in this transaction, - * we need to acquire AccessShareLock. We arrange for the lock to + * we need to acquire a lock. We arrange for the lock to * be owned by the top transaction, so that we don't need to do it * more than once per xact. */ static Relation -open_share_lock(SeqTable seq) +lock_and_open_sequence(SeqTable seq) { LocalTransactionId thislxid = MyProc->lxid; @@ -1063,7 +1066,7 @@ open_share_lock(SeqTable seq) PG_TRY(); { CurrentResourceOwner = TopTransactionResourceOwner; - LockRelationOid(seq->relid, AccessShareLock); + LockRelationOid(seq->relid, RowExclusiveLock); } PG_CATCH(); { @@ -1078,7 +1081,7 @@ open_share_lock(SeqTable seq) seq->lxid = thislxid; } - /* We now know we have AccessShareLock, and can safely open the rel */ + /* We now know we have the lock, and can safely open the rel */ return relation_open(seq->relid, NoLock); } @@ -1135,7 +1138,7 @@ init_sequence(Oid relid, SeqTable *p_elm, Relation *p_rel) /* * Open the sequence relation. */ - seqrel = open_share_lock(elm); + seqrel = lock_and_open_sequence(elm); if (seqrel->rd_rel->relkind != RELKIND_SEQUENCE) ereport(ERROR, @@ -1216,6 +1219,30 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple) return seq; } +/* + * Check the sequence options list and return the appropriate lock level for + * ALTER SEQUENCE. + * + * Most sequence option changes require a self-exclusive lock and should block + * concurrent nextval() et al. But RESTART does not, because it's not + * transactional. Also take a lower lock if no option at all is present. + */ +static LOCKMODE +alter_sequence_get_lock_level(List *options) +{ + ListCell *option; + + foreach(option, options) + { + DefElem *defel = (DefElem *) lfirst(option); + + if (strcmp(defel->defname, "restart") != 0) + return ShareRowExclusiveLock; + } + + return RowExclusiveLock; +} + /* * init_params: process the options list of CREATE or ALTER SEQUENCE, and * store the values into appropriate fields of seqform, for changes that go @@ -1850,7 +1877,7 @@ pg_sequence_last_value(PG_FUNCTION_ARGS) bool is_called; int64 result; - /* open and AccessShareLock sequence */ + /* open and lock sequence */ init_sequence(relid, &elm, &seqrel); if (pg_class_aclcheck(relid, GetUserId(), ACL_SELECT | ACL_USAGE) != ACLCHECK_OK) diff --git a/src/test/isolation/expected/sequence-ddl.out b/src/test/isolation/expected/sequence-ddl.out new file mode 100644 index 0000000000..6b7119738f --- /dev/null +++ b/src/test/isolation/expected/sequence-ddl.out @@ -0,0 +1,85 @@ +Parsed test spec with 2 sessions + +starting permutation: s1alter s1commit s2nv +step s1alter: ALTER SEQUENCE seq1 MAXVALUE 10; +step s1commit: COMMIT; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +ERROR: nextval: reached maximum value of sequence "seq1" (10) + +starting permutation: s1alter s2nv s1commit +step s1alter: ALTER SEQUENCE seq1 MAXVALUE 10; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +step s1commit: COMMIT; +step s2nv: <... completed> +error in steps s1commit s2nv: ERROR: nextval: reached maximum value of sequence "seq1" (10) + +starting permutation: s2begin s2nv s1alter2 s2commit s1commit +step s2begin: BEGIN; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +nextval + +1 +2 +3 +4 +5 +6 +7 +8 +9 +10 +11 +12 +13 +14 +15 +step s1alter2: ALTER SEQUENCE seq1 MAXVALUE 20; +step s2commit: COMMIT; +step s1alter2: <... completed> +step s1commit: COMMIT; + +starting permutation: s1restart s2nv s1commit +step s1restart: ALTER SEQUENCE seq1 RESTART WITH 5; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +nextval + +5 +6 +7 +8 +9 +10 +11 +12 +13 +14 +15 +16 +17 +18 +19 +step s1commit: COMMIT; + +starting permutation: s2begin s2nv s1restart s2commit s1commit +step s2begin: BEGIN; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +nextval + +1 +2 +3 +4 +5 +6 +7 +8 +9 +10 +11 +12 +13 +14 +15 +step s1restart: ALTER SEQUENCE seq1 RESTART WITH 5; +step s2commit: COMMIT; +step s1commit: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index fc1918dedc..32c965b2a0 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -58,6 +58,7 @@ test: alter-table-1 test: alter-table-2 test: alter-table-3 test: create-trigger +test: sequence-ddl test: async-notify test: vacuum-reltuples test: timeouts diff --git a/src/test/isolation/specs/sequence-ddl.spec b/src/test/isolation/specs/sequence-ddl.spec new file mode 100644 index 0000000000..42ee3b0615 --- /dev/null +++ b/src/test/isolation/specs/sequence-ddl.spec @@ -0,0 +1,39 @@ +# Test sequence usage and concurrent sequence DDL + +setup +{ + CREATE SEQUENCE seq1; +} + +teardown +{ + DROP SEQUENCE seq1; +} + +session "s1" +setup { BEGIN; } +step "s1alter" { ALTER SEQUENCE seq1 MAXVALUE 10; } +step "s1alter2" { ALTER SEQUENCE seq1 MAXVALUE 20; } +step "s1restart" { ALTER SEQUENCE seq1 RESTART WITH 5; } +step "s1commit" { COMMIT; } + +session "s2" +step "s2begin" { BEGIN; } +step "s2nv" { SELECT nextval('seq1') FROM generate_series(1, 15); } +step "s2commit" { COMMIT; } + +permutation "s1alter" "s1commit" "s2nv" + +# Prior to PG10, the s2nv would see the uncommitted s1alter change, +# but now it waits. +permutation "s1alter" "s2nv" "s1commit" + +# nextval doesn't release lock until transaction end, so s1alter2 has +# to wait for s2commit. +permutation "s2begin" "s2nv" "s1alter2" "s2commit" "s1commit" + +# RESTART is nontransactional, so s2nv sees it right away +permutation "s1restart" "s2nv" "s1commit" + +# RESTART does not wait +permutation "s2begin" "s2nv" "s1restart" "s2commit" "s1commit"