Skip to content

Commit 3d79013

Browse files
anarazelanarazel
committed
Make ALTER SEQUENCE, including RESTART, fully transactional.
Previously the changes to the "data" part of the sequence, i.e. the one containing the current value, were not transactional, whereas the definition, including minimum and maximum value were. That leads to odd behaviour if a schema change is rolled back, with the potential that out-of-bound sequence values can be returned. To avoid the issue create a new relfilenode fork whenever ALTER SEQUENCE is executed, similar to how TRUNCATE ... RESTART IDENTITY already is already handled. This commit also makes ALTER SEQUENCE RESTART transactional, as it seems to be too confusing to have some forms of ALTER SEQUENCE behave transactionally, some forms not. This way setval() and nextval() are not transactional, but DDL is, which seems to make sense. This commit also rolls back parts of the changes made in 3d092fe and f8dc198 as they're now not needed anymore. Author: Andres Freund Discussion: https://postgr.es/m/20170522154227.nvafbsm62sjpbxvd@alap3.anarazel.de Backpatch: Bug is in master/v10 only
1 parent e9a3c04 commit 3d79013

File tree

4 files changed

+65
-122
lines changed

4 files changed

+65
-122
lines changed

‎doc/src/sgml/ref/alter_sequence.sgml

+7-8
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ ALTER SEQUENCE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> S
171171
<para>
172172
The optional clause <literal>RESTART [ WITH <replaceable
173173
class="parameter">restart</replaceable> ]</literal> changes the
174-
current value of the sequence. This is equivalent to calling the
174+
current value of the sequence. This is similar to calling the
175175
<function>setval</> function with <literal>is_called</literal> =
176176
<literal>false</>: the specified value will be returned by the
177177
<emphasis>next</> call of <function>nextval</>.
@@ -182,11 +182,11 @@ ALTER SEQUENCE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> S
182182
</para>
183183

184184
<para>
185-
Like a <function>setval</function> call, a <literal>RESTART</literal>
186-
operation on a sequence is never rolled back, to avoid blocking of
187-
concurrent transactions that obtain numbers from the same sequence.
188-
(The other clauses cause ordinary catalog updates that can be rolled
189-
back.)
185+
In contrast to a <function>setval</function> call,
186+
a <literal>RESTART</literal> operation on a sequence is transactional
187+
and blocks concurrent transactions from obtaining numbers from the
188+
same sequence. If that's not the desired mode of
189+
operation, <function>setval</> should be used.
190190
</para>
191191
</listitem>
192192
</varlistentry>
@@ -307,8 +307,7 @@ ALTER SEQUENCE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> S
307307
<para>
308308
<command>ALTER SEQUENCE</command> blocks
309309
concurrent <function>nextval</function>, <function>currval</function>,
310-
<function>lastval</function>, and <command>setval</command> calls, except
311-
if only the <literal>RESTART</literal> clause is used.
310+
<function>lastval</function>, and <command>setval</command> calls.
312311
</para>
313312

314313
<para>

‎src/backend/commands/sequence.c

+32-91
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,9 @@ static void create_seq_hashtable(void);
9898
static void init_sequence(Oid relid, SeqTable *p_elm, Relation *p_rel);
9999
static Form_pg_sequence_data read_seq_tuple(Relation rel,
100100
Buffer *buf, HeapTuple seqdatatuple);
101-
static LOCKMODE alter_sequence_get_lock_level(List *options);
102101
static void init_params(ParseState *pstate, List *options, bool for_identity,
103102
bool isInit,
104103
Form_pg_sequence seqform,
105-
bool *changed_seqform,
106104
Form_pg_sequence_data seqdataform, List **owned_by);
107105
static void do_setval(Oid relid, int64 next, bool iscalled);
108106
static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);
@@ -117,7 +115,6 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
117115
{
118116
FormData_pg_sequence seqform;
119117
FormData_pg_sequence_data seqdataform;
120-
bool changed_seqform = false; /* not used here */
121118
List *owned_by;
122119
CreateStmt *stmt = makeNode(CreateStmt);
123120
Oid seqoid;
@@ -156,7 +153,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
156153
}
157154

158155
/* Check and set all option values */
159-
init_params(pstate, seq->options, seq->for_identity, true, &seqform, &changed_seqform, &seqdataform, &owned_by);
156+
init_params(pstate, seq->options, seq->for_identity, true, &seqform, &seqdataform, &owned_by);
160157

161158
/*
162159
* Create relation (and fill value[] and null[] for the tuple)
@@ -417,19 +414,18 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
417414
SeqTable elm;
418415
Relation seqrel;
419416
Buffer buf;
420-
HeapTupleData seqdatatuple;
417+
HeapTupleData datatuple;
421418
Form_pg_sequence seqform;
422-
Form_pg_sequence_data seqdata;
423-
FormData_pg_sequence_data newseqdata;
424-
bool changed_seqform = false;
419+
Form_pg_sequence_data newdataform;
425420
List *owned_by;
426421
ObjectAddress address;
427422
Relation rel;
428-
HeapTuple tuple;
423+
HeapTuple seqtuple;
424+
HeapTuple newdatatuple;
429425

430426
/* Open and lock sequence. */
431427
relid = RangeVarGetRelid(stmt->sequence,
432-
alter_sequence_get_lock_level(stmt->options),
428+
ShareRowExclusiveLock,
433429
stmt->missing_ok);
434430
if (relid == InvalidOid)
435431
{
@@ -447,22 +443,26 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
447443
stmt->sequence->relname);
448444

449445
rel = heap_open(SequenceRelationId, RowExclusiveLock);
450-
tuple = SearchSysCacheCopy1(SEQRELID,
451-
ObjectIdGetDatum(relid));
452-
if (!HeapTupleIsValid(tuple))
446+
seqtuple = SearchSysCacheCopy1(SEQRELID,
447+
ObjectIdGetDatum(relid));
448+
if (!HeapTupleIsValid(seqtuple))
453449
elog(ERROR, "cache lookup failed for sequence %u",
454450
relid);
455451

456-
seqform = (Form_pg_sequence) GETSTRUCT(tuple);
452+
seqform = (Form_pg_sequence) GETSTRUCT(seqtuple);
457453

458454
/* lock page's buffer and read tuple into new sequence structure */
459-
seqdata = read_seq_tuple(seqrel, &buf, &seqdatatuple);
455+
(void) read_seq_tuple(seqrel, &buf, &datatuple);
460456

461-
/* Copy old sequence data into workspace */
462-
memcpy(&newseqdata, seqdata, sizeof(FormData_pg_sequence_data));
457+
/* copy the existing sequence data tuple, so it can be modified localy */
458+
newdatatuple = heap_copytuple(&datatuple);
459+
newdataform = (Form_pg_sequence_data) GETSTRUCT(newdatatuple);
460+
461+
UnlockReleaseBuffer(buf);
463462

464463
/* Check and set new values */
465-
init_params(pstate, stmt->options, stmt->for_identity, false, seqform, &changed_seqform, &newseqdata, &owned_by);
464+
init_params(pstate, stmt->options, stmt->for_identity, false, seqform,
465+
newdataform, &owned_by);
466466

467467
/* Clear local cache so that we don't think we have cached numbers */
468468
/* Note that we do not change the currval() state */
@@ -472,36 +472,19 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
472472
if (RelationNeedsWAL(seqrel))
473473
GetTopTransactionId();
474474

475-
/* Now okay to update the on-disk tuple */
476-
START_CRIT_SECTION();
477-
478-
memcpy(seqdata, &newseqdata, sizeof(FormData_pg_sequence_data));
479-
480-
MarkBufferDirty(buf);
481-
482-
/* XLOG stuff */
483-
if (RelationNeedsWAL(seqrel))
484-
{
485-
xl_seq_rec xlrec;
486-
XLogRecPtr recptr;
487-
Page page = BufferGetPage(buf);
488-
489-
XLogBeginInsert();
490-
XLogRegisterBuffer(0, buf, REGBUF_WILL_INIT);
491-
492-
xlrec.node = seqrel->rd_node;
493-
XLogRegisterData((char *) &xlrec, sizeof(xl_seq_rec));
494-
495-
XLogRegisterData((char *) seqdatatuple.t_data, seqdatatuple.t_len);
496-
497-
recptr = XLogInsert(RM_SEQ_ID, XLOG_SEQ_LOG);
498-
499-
PageSetLSN(page, recptr);
500-
}
501-
502-
END_CRIT_SECTION();
475+
/*
476+
* Create a new storage file for the sequence, making the state changes
477+
* transactional. We want to keep the sequence's relfrozenxid at 0, since
478+
* it won't contain any unfrozen XIDs. Same with relminmxid, since a
479+
* sequence will never contain multixacts.
480+
*/
481+
RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
482+
InvalidTransactionId, InvalidMultiXactId);
503483

504-
UnlockReleaseBuffer(buf);
484+
/*
485+
* Insert the modified tuple into the new storage file.
486+
*/
487+
fill_seq_with_data(seqrel, newdatatuple);
505488

506489
/* process OWNED BY if given */
507490
if (owned_by)
@@ -511,10 +494,9 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
511494

512495
ObjectAddressSet(address, RelationRelationId, relid);
513496

514-
if (changed_seqform)
515-
CatalogTupleUpdate(rel, &tuple->t_self, tuple);
516-
heap_close(rel, RowExclusiveLock);
497+
CatalogTupleUpdate(rel, &seqtuple->t_self, seqtuple);
517498

499+
heap_close(rel, RowExclusiveLock);
518500
relation_close(seqrel, NoLock);
519501

520502
return address;
@@ -1219,30 +1201,6 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
12191201
return seq;
12201202
}
12211203

1222-
/*
1223-
* Check the sequence options list and return the appropriate lock level for
1224-
* ALTER SEQUENCE.
1225-
*
1226-
* Most sequence option changes require a self-exclusive lock and should block
1227-
* concurrent nextval() et al. But RESTART does not, because it's not
1228-
* transactional. Also take a lower lock if no option at all is present.
1229-
*/
1230-
static LOCKMODE
1231-
alter_sequence_get_lock_level(List *options)
1232-
{
1233-
ListCell *option;
1234-
1235-
foreach(option, options)
1236-
{
1237-
DefElem *defel = (DefElem *) lfirst(option);
1238-
1239-
if (strcmp(defel->defname, "restart") != 0)
1240-
return ShareRowExclusiveLock;
1241-
}
1242-
1243-
return RowExclusiveLock;
1244-
}
1245-
12461204
/*
12471205
* init_params: process the options list of CREATE or ALTER SEQUENCE, and
12481206
* store the values into appropriate fields of seqform, for changes that go
@@ -1258,7 +1216,6 @@ static void
12581216
init_params(ParseState *pstate, List *options, bool for_identity,
12591217
bool isInit,
12601218
Form_pg_sequence seqform,
1261-
bool *changed_seqform,
12621219
Form_pg_sequence_data seqdataform,
12631220
List **owned_by)
12641221
{
@@ -1378,8 +1335,6 @@ init_params(ParseState *pstate, List *options, bool for_identity,
13781335
defel->defname);
13791336
}
13801337

1381-
*changed_seqform = false;
1382-
13831338
/*
13841339
* We must reset log_cnt when isInit or when changing any parameters that
13851340
* would affect future nextval allocations.
@@ -1420,19 +1375,16 @@ init_params(ParseState *pstate, List *options, bool for_identity,
14201375
}
14211376

14221377
seqform->seqtypid = newtypid;
1423-
*changed_seqform = true;
14241378
}
14251379
else if (isInit)
14261380
{
14271381
seqform->seqtypid = INT8OID;
1428-
*changed_seqform = true;
14291382
}
14301383

14311384
/* INCREMENT BY */
14321385
if (increment_by != NULL)
14331386
{
14341387
seqform->seqincrement = defGetInt64(increment_by);
1435-
*changed_seqform = true;
14361388
if (seqform->seqincrement == 0)
14371389
ereport(ERROR,
14381390
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -1442,28 +1394,24 @@ init_params(ParseState *pstate, List *options, bool for_identity,
14421394
else if (isInit)
14431395
{
14441396
seqform->seqincrement = 1;
1445-
*changed_seqform = true;
14461397
}
14471398

14481399
/* CYCLE */
14491400
if (is_cycled != NULL)
14501401
{
14511402
seqform->seqcycle = intVal(is_cycled->arg);
1452-
*changed_seqform = true;
14531403
Assert(BoolIsValid(seqform->seqcycle));
14541404
seqdataform->log_cnt = 0;
14551405
}
14561406
else if (isInit)
14571407
{
14581408
seqform->seqcycle = false;
1459-
*changed_seqform = true;
14601409
}
14611410

14621411
/* MAXVALUE (null arg means NO MAXVALUE) */
14631412
if (max_value != NULL && max_value->arg)
14641413
{
14651414
seqform->seqmax = defGetInt64(max_value);
1466-
*changed_seqform = true;
14671415
seqdataform->log_cnt = 0;
14681416
}
14691417
else if (isInit || max_value != NULL || reset_max_value)
@@ -1480,7 +1428,6 @@ init_params(ParseState *pstate, List *options, bool for_identity,
14801428
}
14811429
else
14821430
seqform->seqmax = -1; /* descending seq */
1483-
*changed_seqform = true;
14841431
seqdataform->log_cnt = 0;
14851432
}
14861433

@@ -1502,7 +1449,6 @@ init_params(ParseState *pstate, List *options, bool for_identity,
15021449
if (min_value != NULL && min_value->arg)
15031450
{
15041451
seqform->seqmin = defGetInt64(min_value);
1505-
*changed_seqform = true;
15061452
seqdataform->log_cnt = 0;
15071453
}
15081454
else if (isInit || min_value != NULL || reset_min_value)
@@ -1519,7 +1465,6 @@ init_params(ParseState *pstate, List *options, bool for_identity,
15191465
}
15201466
else
15211467
seqform->seqmin = 1; /* ascending seq */
1522-
*changed_seqform = true;
15231468
seqdataform->log_cnt = 0;
15241469
}
15251470

@@ -1555,15 +1500,13 @@ init_params(ParseState *pstate, List *options, bool for_identity,
15551500
if (start_value != NULL)
15561501
{
15571502
seqform->seqstart = defGetInt64(start_value);
1558-
*changed_seqform = true;
15591503
}
15601504
else if (isInit)
15611505
{
15621506
if (seqform->seqincrement > 0)
15631507
seqform->seqstart = seqform->seqmin; /* ascending seq */
15641508
else
15651509
seqform->seqstart = seqform->seqmax; /* descending seq */
1566-
*changed_seqform = true;
15671510
}
15681511

15691512
/* crosscheck START */
@@ -1638,7 +1581,6 @@ init_params(ParseState *pstate, List *options, bool for_identity,
16381581
if (cache_value != NULL)
16391582
{
16401583
seqform->seqcache = defGetInt64(cache_value);
1641-
*changed_seqform = true;
16421584
if (seqform->seqcache <= 0)
16431585
{
16441586
char buf[100];
@@ -1654,7 +1596,6 @@ init_params(ParseState *pstate, List *options, bool for_identity,
16541596
else if (isInit)
16551597
{
16561598
seqform->seqcache = 1;
1657-
*changed_seqform = true;
16581599
}
16591600
}
16601601

0 commit comments

Comments
 (0)