diff --git a/doc/src/sgml/ref/alter_sequence.sgml b/doc/src/sgml/ref/alter_sequence.sgml index 30e5316b8c..3a04d07ecc 100644 --- a/doc/src/sgml/ref/alter_sequence.sgml +++ b/doc/src/sgml/ref/alter_sequence.sgml @@ -171,7 +171,7 @@ ALTER SEQUENCE [ IF EXISTS ] name S The optional clause RESTART [ WITH restart ] changes the - current value of the sequence. This is equivalent to calling the + current value of the sequence. This is similar to calling the setval function with is_called = false: the specified value will be returned by the next call of nextval. @@ -182,11 +182,11 @@ ALTER SEQUENCE [ IF EXISTS ] name S - Like a setval call, a RESTART - operation on a sequence is never rolled back, to avoid blocking of - concurrent transactions that obtain numbers from the same sequence. - (The other clauses cause ordinary catalog updates that can be rolled - back.) + In contrast to a setval call, + a RESTART operation on a sequence is transactional + and blocks concurrent transactions from obtaining numbers from the + same sequence. If that's not the desired mode of + operation, setval should be used. @@ -307,8 +307,7 @@ ALTER SEQUENCE [ IF EXISTS ] name S ALTER SEQUENCE blocks concurrent nextval, currval, - lastval, and setval calls, except - if only the RESTART clause is used. + lastval, and setval calls. diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c index 568b3022f2..4a56f03e8b 100644 --- a/src/backend/commands/sequence.c +++ b/src/backend/commands/sequence.c @@ -98,11 +98,9 @@ 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, - bool *changed_seqform, Form_pg_sequence_data seqdataform, List **owned_by); static void do_setval(Oid relid, int64 next, bool iscalled); static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity); @@ -117,7 +115,6 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq) { FormData_pg_sequence seqform; FormData_pg_sequence_data seqdataform; - bool changed_seqform = false; /* not used here */ List *owned_by; CreateStmt *stmt = makeNode(CreateStmt); Oid seqoid; @@ -156,7 +153,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq) } /* Check and set all option values */ - init_params(pstate, seq->options, seq->for_identity, true, &seqform, &changed_seqform, &seqdataform, &owned_by); + init_params(pstate, seq->options, seq->for_identity, true, &seqform, &seqdataform, &owned_by); /* * Create relation (and fill value[] and null[] for the tuple) @@ -417,19 +414,18 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) SeqTable elm; Relation seqrel; Buffer buf; - HeapTupleData seqdatatuple; + HeapTupleData datatuple; Form_pg_sequence seqform; - Form_pg_sequence_data seqdata; - FormData_pg_sequence_data newseqdata; - bool changed_seqform = false; + Form_pg_sequence_data newdataform; List *owned_by; ObjectAddress address; Relation rel; - HeapTuple tuple; + HeapTuple seqtuple; + HeapTuple newdatatuple; /* Open and lock sequence. */ relid = RangeVarGetRelid(stmt->sequence, - alter_sequence_get_lock_level(stmt->options), + ShareRowExclusiveLock, stmt->missing_ok); if (relid == InvalidOid) { @@ -447,22 +443,26 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) stmt->sequence->relname); rel = heap_open(SequenceRelationId, RowExclusiveLock); - tuple = SearchSysCacheCopy1(SEQRELID, - ObjectIdGetDatum(relid)); - if (!HeapTupleIsValid(tuple)) + seqtuple = SearchSysCacheCopy1(SEQRELID, + ObjectIdGetDatum(relid)); + if (!HeapTupleIsValid(seqtuple)) elog(ERROR, "cache lookup failed for sequence %u", relid); - seqform = (Form_pg_sequence) GETSTRUCT(tuple); + seqform = (Form_pg_sequence) GETSTRUCT(seqtuple); /* lock page's buffer and read tuple into new sequence structure */ - seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple); + (void) read_seq_tuple(seqrel, &buf, &datatuple); - /* Copy old sequence data into workspace */ - memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data)); + /* copy the existing sequence data tuple, so it can be modified localy */ + newdatatuple = heap_copytuple(&datatuple); + newdataform = (Form_pg_sequence_data) GETSTRUCT(newdatatuple); + + UnlockReleaseBuffer(buf); /* Check and set new values */ - init_params(pstate, stmt->options, stmt->for_identity, false, seqform, &changed_seqform, &newseqdata, &owned_by); + init_params(pstate, stmt->options, stmt->for_identity, false, seqform, + newdataform, &owned_by); /* Clear local cache so that we don't think we have cached numbers */ /* Note that we do not change the currval() state */ @@ -472,36 +472,19 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) if (RelationNeedsWAL(seqrel)) GetTopTransactionId(); - /* Now okay to update the on-disk tuple */ - START_CRIT_SECTION(); + /* + * Create a new storage file for the sequence, making the state changes + * transactional. We want to keep the sequence's relfrozenxid at 0, since + * it won't contain any unfrozen XIDs. Same with relminmxid, since a + * sequence will never contain multixacts. + */ + RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence, + InvalidTransactionId, InvalidMultiXactId); - memcpy(seqdata, &newseqdata, sizeof(FormData_pg_sequence_data)); - - MarkBufferDirty(buf); - - /* XLOG stuff */ - if (RelationNeedsWAL(seqrel)) - { - xl_seq_rec xlrec; - XLogRecPtr recptr; - Page page = BufferGetPage(buf); - - XLogBeginInsert(); - XLogRegisterBuffer(0, buf, REGBUF_WILL_INIT); - - xlrec.node = seqrel->rd_node; - XLogRegisterData((char *) &xlrec, sizeof(xl_seq_rec)); - - XLogRegisterData((char *) seqdatatuple.t_data, seqdatatuple.t_len); - - recptr = XLogInsert(RM_SEQ_ID, XLOG_SEQ_LOG); - - PageSetLSN(page, recptr); - } - - END_CRIT_SECTION(); - - UnlockReleaseBuffer(buf); + /* + * Insert the modified tuple into the new storage file. + */ + fill_seq_with_data(seqrel, newdatatuple); /* process OWNED BY if given */ if (owned_by) @@ -511,10 +494,9 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) ObjectAddressSet(address, RelationRelationId, relid); - if (changed_seqform) - CatalogTupleUpdate(rel, &tuple->t_self, tuple); - heap_close(rel, RowExclusiveLock); + CatalogTupleUpdate(rel, &seqtuple->t_self, seqtuple); + heap_close(rel, RowExclusiveLock); relation_close(seqrel, NoLock); return address; @@ -1219,30 +1201,6 @@ 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 @@ -1258,7 +1216,6 @@ static void init_params(ParseState *pstate, List *options, bool for_identity, bool isInit, Form_pg_sequence seqform, - bool *changed_seqform, Form_pg_sequence_data seqdataform, List **owned_by) { @@ -1378,8 +1335,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, defel->defname); } - *changed_seqform = false; - /* * We must reset log_cnt when isInit or when changing any parameters that * would affect future nextval allocations. @@ -1420,19 +1375,16 @@ init_params(ParseState *pstate, List *options, bool for_identity, } seqform->seqtypid = newtypid; - *changed_seqform = true; } else if (isInit) { seqform->seqtypid = INT8OID; - *changed_seqform = true; } /* INCREMENT BY */ if (increment_by != NULL) { seqform->seqincrement = defGetInt64(increment_by); - *changed_seqform = true; if (seqform->seqincrement == 0) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), @@ -1442,28 +1394,24 @@ init_params(ParseState *pstate, List *options, bool for_identity, else if (isInit) { seqform->seqincrement = 1; - *changed_seqform = true; } /* CYCLE */ if (is_cycled != NULL) { seqform->seqcycle = intVal(is_cycled->arg); - *changed_seqform = true; Assert(BoolIsValid(seqform->seqcycle)); seqdataform->log_cnt = 0; } else if (isInit) { seqform->seqcycle = false; - *changed_seqform = true; } /* MAXVALUE (null arg means NO MAXVALUE) */ if (max_value != NULL && max_value->arg) { seqform->seqmax = defGetInt64(max_value); - *changed_seqform = true; seqdataform->log_cnt = 0; } else if (isInit || max_value != NULL || reset_max_value) @@ -1480,7 +1428,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, } else seqform->seqmax = -1; /* descending seq */ - *changed_seqform = true; seqdataform->log_cnt = 0; } @@ -1502,7 +1449,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, if (min_value != NULL && min_value->arg) { seqform->seqmin = defGetInt64(min_value); - *changed_seqform = true; seqdataform->log_cnt = 0; } else if (isInit || min_value != NULL || reset_min_value) @@ -1519,7 +1465,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, } else seqform->seqmin = 1; /* ascending seq */ - *changed_seqform = true; seqdataform->log_cnt = 0; } @@ -1555,7 +1500,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, if (start_value != NULL) { seqform->seqstart = defGetInt64(start_value); - *changed_seqform = true; } else if (isInit) { @@ -1563,7 +1507,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, seqform->seqstart = seqform->seqmin; /* ascending seq */ else seqform->seqstart = seqform->seqmax; /* descending seq */ - *changed_seqform = true; } /* crosscheck START */ @@ -1638,7 +1581,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, if (cache_value != NULL) { seqform->seqcache = defGetInt64(cache_value); - *changed_seqform = true; if (seqform->seqcache <= 0) { char buf[100]; @@ -1654,7 +1596,6 @@ init_params(ParseState *pstate, List *options, bool for_identity, else if (isInit) { seqform->seqcache = 1; - *changed_seqform = true; } } diff --git a/src/test/isolation/expected/sequence-ddl.out b/src/test/isolation/expected/sequence-ddl.out index 6b7119738f..6766c0aff6 100644 --- a/src/test/isolation/expected/sequence-ddl.out +++ b/src/test/isolation/expected/sequence-ddl.out @@ -13,6 +13,52 @@ step s1commit: COMMIT; step s2nv: <... completed> error in steps s1commit s2nv: ERROR: nextval: reached maximum value of sequence "seq1" (10) +starting permutation: s1restart s2nv s1commit +step s1restart: ALTER SEQUENCE seq1 RESTART WITH 5; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +step s1commit: COMMIT; +step s2nv: <... completed> +nextval + +5 +6 +7 +8 +9 +10 +11 +12 +13 +14 +15 +16 +17 +18 +19 + +starting permutation: s1restart s2nv s1commit +step s1restart: ALTER SEQUENCE seq1 RESTART WITH 5; +step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); +step s1commit: COMMIT; +step s2nv: <... completed> +nextval + +5 +6 +7 +8 +9 +10 +11 +12 +13 +14 +15 +16 +17 +18 +19 + starting permutation: s2begin s2nv s1alter2 s2commit s1commit step s2begin: BEGIN; step s2nv: SELECT nextval('seq1') FROM generate_series(1, 15); @@ -37,49 +83,3 @@ 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/specs/sequence-ddl.spec b/src/test/isolation/specs/sequence-ddl.spec index 42ee3b0615..5c51fcdae6 100644 --- a/src/test/isolation/specs/sequence-ddl.spec +++ b/src/test/isolation/specs/sequence-ddl.spec @@ -15,6 +15,7 @@ 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 "s1setval" { SELECT setval('seq1', 5); } step "s1commit" { COMMIT; } session "s2" @@ -24,16 +25,18 @@ step "s2commit" { COMMIT; } permutation "s1alter" "s1commit" "s2nv" -# Prior to PG10, the s2nv would see the uncommitted s1alter change, -# but now it waits. +# Prior to PG10, the s2nv step would see the uncommitted s1alter +# change, but now it waits. permutation "s1alter" "s2nv" "s1commit" +# Prior to PG10, the s2nv step would see the uncommitted s1reset +# change, but now it waits. +permutation "s1restart" "s2nv" "s1commit" + +# In contrast to ALTER setval() is non-transactional, so it doesn't +# have to wait. +permutation "s1restart" "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"