Skip to content

Commit a475e46

Browse files
tglsfdctglsfdc
committed
Fix ALTER SEQUENCE OWNED BY to not rewrite the sequence relation.
It's not necessary for it to do that, since OWNED BY requires only ordinary catalog updates and doesn't affect future sequence values. And pg_upgrade needs to use OWNED BY without having it change the sequence's relfilenode. Commit 3d79013 broke this by making all forms of ALTER SEQUENCE change the relfilenode; that seems to be the explanation for the hard-to-reproduce buildfarm failures we've been seeing since then. Discussion: https://postgr.es/m/19785.1497215827@sss.pgh.pa.us
1 parent 5d8beac commit a475e46

File tree

1 file changed

+53
-23
lines changed

1 file changed

+53
-23
lines changed

‎src/backend/commands/sequence.c

Lines changed: 53 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,9 @@ static Form_pg_sequence_data read_seq_tuple(Relation rel,
101101
static void init_params(ParseState *pstate, List *options, bool for_identity,
102102
bool isInit,
103103
Form_pg_sequence seqform,
104-
Form_pg_sequence_data seqdataform, List **owned_by);
104+
Form_pg_sequence_data seqdataform,
105+
bool *need_seq_rewrite,
106+
List **owned_by);
105107
static void do_setval(Oid relid, int64 next, bool iscalled);
106108
static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);
107109

@@ -115,6 +117,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
115117
{
116118
FormData_pg_sequence seqform;
117119
FormData_pg_sequence_data seqdataform;
120+
bool need_seq_rewrite;
118121
List *owned_by;
119122
CreateStmt *stmt = makeNode(CreateStmt);
120123
Oid seqoid;
@@ -153,7 +156,9 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
153156
}
154157

155158
/* Check and set all option values */
156-
init_params(pstate, seq->options, seq->for_identity, true, &seqform, &seqdataform, &owned_by);
159+
init_params(pstate, seq->options, seq->for_identity, true,
160+
&seqform, &seqdataform,
161+
&need_seq_rewrite, &owned_by);
157162

158163
/*
159164
* Create relation (and fill value[] and null[] for the tuple)
@@ -417,6 +422,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
417422
HeapTupleData datatuple;
418423
Form_pg_sequence seqform;
419424
Form_pg_sequence_data newdataform;
425+
bool need_seq_rewrite;
420426
List *owned_by;
421427
ObjectAddress address;
422428
Relation rel;
@@ -461,35 +467,41 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
461467
UnlockReleaseBuffer(buf);
462468

463469
/* Check and set new values */
464-
init_params(pstate, stmt->options, stmt->for_identity, false, seqform,
465-
newdataform, &owned_by);
470+
init_params(pstate, stmt->options, stmt->for_identity, false,
471+
seqform, newdataform,
472+
&need_seq_rewrite, &owned_by);
466473

467474
/* Clear local cache so that we don't think we have cached numbers */
468475
/* Note that we do not change the currval() state */
469476
elm->cached = elm->last;
470477

471-
/* check the comment above nextval_internal()'s equivalent call. */
472-
if (RelationNeedsWAL(seqrel))
473-
GetTopTransactionId();
478+
/* If needed, rewrite the sequence relation itself */
479+
if (need_seq_rewrite)
480+
{
481+
/* check the comment above nextval_internal()'s equivalent call. */
482+
if (RelationNeedsWAL(seqrel))
483+
GetTopTransactionId();
474484

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);
485+
/*
486+
* Create a new storage file for the sequence, making the state
487+
* changes transactional. We want to keep the sequence's relfrozenxid
488+
* at 0, since it won't contain any unfrozen XIDs. Same with
489+
* relminmxid, since a sequence will never contain multixacts.
490+
*/
491+
RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
492+
InvalidTransactionId, InvalidMultiXactId);
483493

484-
/*
485-
* Insert the modified tuple into the new storage file.
486-
*/
487-
fill_seq_with_data(seqrel, newdatatuple);
494+
/*
495+
* Insert the modified tuple into the new storage file.
496+
*/
497+
fill_seq_with_data(seqrel, newdatatuple);
498+
}
488499

489500
/* process OWNED BY if given */
490501
if (owned_by)
491502
process_owned_by(seqrel, owned_by, stmt->for_identity);
492503

504+
/* update the pg_sequence tuple (we could skip this in some cases...) */
493505
CatalogTupleUpdate(rel, &seqtuple->t_self, seqtuple);
494506

495507
InvokeObjectPostAlterHook(RelationRelationId, relid, 0);
@@ -1204,19 +1216,28 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
12041216
/*
12051217
* init_params: process the options list of CREATE or ALTER SEQUENCE, and
12061218
* store the values into appropriate fields of seqform, for changes that go
1207-
* into the pg_sequence catalog, and seqdataform for changes to the sequence
1208-
* relation itself. Set *changed_seqform to true if seqform was changed
1209-
* (interesting for ALTER SEQUENCE). Also set *owned_by to any OWNED BY
1210-
* option, or to NIL if there is none.
1219+
* into the pg_sequence catalog, and fields of seqdataform for changes to the
1220+
* sequence relation itself. Set *need_seq_rewrite to true if we changed any
1221+
* parameters that require rewriting the sequence's relation (interesting for
1222+
* ALTER SEQUENCE). Also set *owned_by to any OWNED BY option, or to NIL if
1223+
* there is none.
12111224
*
12121225
* If isInit is true, fill any unspecified options with default values;
12131226
* otherwise, do not change existing options that aren't explicitly overridden.
1227+
*
1228+
* Note: we force a sequence rewrite whenever we change parameters that affect
1229+
* generation of future sequence values, even if the seqdataform per se is not
1230+
* changed. This allows ALTER SEQUENCE to behave transactionally. Currently,
1231+
* the only option that doesn't cause that is OWNED BY. It's *necessary* for
1232+
* ALTER SEQUENCE OWNED BY to not rewrite the sequence, because that would
1233+
* break pg_upgrade by causing unwanted changes in the sequence's relfilenode.
12141234
*/
12151235
static void
12161236
init_params(ParseState *pstate, List *options, bool for_identity,
12171237
bool isInit,
12181238
Form_pg_sequence seqform,
12191239
Form_pg_sequence_data seqdataform,
1240+
bool *need_seq_rewrite,
12201241
List **owned_by)
12211242
{
12221243
DefElem *as_type = NULL;
@@ -1231,6 +1252,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12311252
bool reset_max_value = false;
12321253
bool reset_min_value = false;
12331254

1255+
*need_seq_rewrite = false;
12341256
*owned_by = NIL;
12351257

12361258
foreach(option, options)
@@ -1245,6 +1267,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12451267
errmsg("conflicting or redundant options"),
12461268
parser_errposition(pstate, defel->location)));
12471269
as_type = defel;
1270+
*need_seq_rewrite = true;
12481271
}
12491272
else if (strcmp(defel->defname, "increment") == 0)
12501273
{
@@ -1254,6 +1277,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12541277
errmsg("conflicting or redundant options"),
12551278
parser_errposition(pstate, defel->location)));
12561279
increment_by = defel;
1280+
*need_seq_rewrite = true;
12571281
}
12581282
else if (strcmp(defel->defname, "start") == 0)
12591283
{
@@ -1263,6 +1287,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12631287
errmsg("conflicting or redundant options"),
12641288
parser_errposition(pstate, defel->location)));
12651289
start_value = defel;
1290+
*need_seq_rewrite = true;
12661291
}
12671292
else if (strcmp(defel->defname, "restart") == 0)
12681293
{
@@ -1272,6 +1297,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12721297
errmsg("conflicting or redundant options"),
12731298
parser_errposition(pstate, defel->location)));
12741299
restart_value = defel;
1300+
*need_seq_rewrite = true;
12751301
}
12761302
else if (strcmp(defel->defname, "maxvalue") == 0)
12771303
{
@@ -1281,6 +1307,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12811307
errmsg("conflicting or redundant options"),
12821308
parser_errposition(pstate, defel->location)));
12831309
max_value = defel;
1310+
*need_seq_rewrite = true;
12841311
}
12851312
else if (strcmp(defel->defname, "minvalue") == 0)
12861313
{
@@ -1290,6 +1317,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12901317
errmsg("conflicting or redundant options"),
12911318
parser_errposition(pstate, defel->location)));
12921319
min_value = defel;
1320+
*need_seq_rewrite = true;
12931321
}
12941322
else if (strcmp(defel->defname, "cache") == 0)
12951323
{
@@ -1299,6 +1327,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
12991327
errmsg("conflicting or redundant options"),
13001328
parser_errposition(pstate, defel->location)));
13011329
cache_value = defel;
1330+
*need_seq_rewrite = true;
13021331
}
13031332
else if (strcmp(defel->defname, "cycle") == 0)
13041333
{
@@ -1308,6 +1337,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
13081337
errmsg("conflicting or redundant options"),
13091338
parser_errposition(pstate, defel->location)));
13101339
is_cycled = defel;
1340+
*need_seq_rewrite = true;
13111341
}
13121342
else if (strcmp(defel->defname, "owned_by") == 0)
13131343
{

0 commit comments

Comments
 (0)