Commit a475e466 authored by Tom Lane's avatar Tom Lane

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 3d79013b 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
parent 5d8beac8
...@@ -101,7 +101,9 @@ static Form_pg_sequence_data read_seq_tuple(Relation rel, ...@@ -101,7 +101,9 @@ static Form_pg_sequence_data read_seq_tuple(Relation rel,
static void init_params(ParseState *pstate, List *options, bool for_identity, static void init_params(ParseState *pstate, List *options, bool for_identity,
bool isInit, bool isInit,
Form_pg_sequence seqform, Form_pg_sequence seqform,
Form_pg_sequence_data seqdataform, List **owned_by); Form_pg_sequence_data seqdataform,
bool *need_seq_rewrite,
List **owned_by);
static void do_setval(Oid relid, int64 next, bool iscalled); static void do_setval(Oid relid, int64 next, bool iscalled);
static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity); static void process_owned_by(Relation seqrel, List *owned_by, bool for_identity);
...@@ -115,6 +117,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq) ...@@ -115,6 +117,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
{ {
FormData_pg_sequence seqform; FormData_pg_sequence seqform;
FormData_pg_sequence_data seqdataform; FormData_pg_sequence_data seqdataform;
bool need_seq_rewrite;
List *owned_by; List *owned_by;
CreateStmt *stmt = makeNode(CreateStmt); CreateStmt *stmt = makeNode(CreateStmt);
Oid seqoid; Oid seqoid;
...@@ -153,7 +156,9 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq) ...@@ -153,7 +156,9 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
} }
/* Check and set all option values */ /* Check and set all option values */
init_params(pstate, seq->options, seq->for_identity, true, &seqform, &seqdataform, &owned_by); init_params(pstate, seq->options, seq->for_identity, true,
&seqform, &seqdataform,
&need_seq_rewrite, &owned_by);
/* /*
* Create relation (and fill value[] and null[] for the tuple) * Create relation (and fill value[] and null[] for the tuple)
...@@ -417,6 +422,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) ...@@ -417,6 +422,7 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
HeapTupleData datatuple; HeapTupleData datatuple;
Form_pg_sequence seqform; Form_pg_sequence seqform;
Form_pg_sequence_data newdataform; Form_pg_sequence_data newdataform;
bool need_seq_rewrite;
List *owned_by; List *owned_by;
ObjectAddress address; ObjectAddress address;
Relation rel; Relation rel;
...@@ -461,22 +467,26 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) ...@@ -461,22 +467,26 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
UnlockReleaseBuffer(buf); UnlockReleaseBuffer(buf);
/* Check and set new values */ /* Check and set new values */
init_params(pstate, stmt->options, stmt->for_identity, false, seqform, init_params(pstate, stmt->options, stmt->for_identity, false,
newdataform, &owned_by); seqform, newdataform,
&need_seq_rewrite, &owned_by);
/* Clear local cache so that we don't think we have cached numbers */ /* Clear local cache so that we don't think we have cached numbers */
/* Note that we do not change the currval() state */ /* Note that we do not change the currval() state */
elm->cached = elm->last; elm->cached = elm->last;
/* If needed, rewrite the sequence relation itself */
if (need_seq_rewrite)
{
/* check the comment above nextval_internal()'s equivalent call. */ /* check the comment above nextval_internal()'s equivalent call. */
if (RelationNeedsWAL(seqrel)) if (RelationNeedsWAL(seqrel))
GetTopTransactionId(); GetTopTransactionId();
/* /*
* Create a new storage file for the sequence, making the state changes * Create a new storage file for the sequence, making the state
* transactional. We want to keep the sequence's relfrozenxid at 0, since * changes transactional. We want to keep the sequence's relfrozenxid
* it won't contain any unfrozen XIDs. Same with relminmxid, since a * at 0, since it won't contain any unfrozen XIDs. Same with
* sequence will never contain multixacts. * relminmxid, since a sequence will never contain multixacts.
*/ */
RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence, RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
InvalidTransactionId, InvalidMultiXactId); InvalidTransactionId, InvalidMultiXactId);
...@@ -485,11 +495,13 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt) ...@@ -485,11 +495,13 @@ AlterSequence(ParseState *pstate, AlterSeqStmt *stmt)
* Insert the modified tuple into the new storage file. * Insert the modified tuple into the new storage file.
*/ */
fill_seq_with_data(seqrel, newdatatuple); fill_seq_with_data(seqrel, newdatatuple);
}
/* process OWNED BY if given */ /* process OWNED BY if given */
if (owned_by) if (owned_by)
process_owned_by(seqrel, owned_by, stmt->for_identity); process_owned_by(seqrel, owned_by, stmt->for_identity);
/* update the pg_sequence tuple (we could skip this in some cases...) */
CatalogTupleUpdate(rel, &seqtuple->t_self, seqtuple); CatalogTupleUpdate(rel, &seqtuple->t_self, seqtuple);
InvokeObjectPostAlterHook(RelationRelationId, relid, 0); InvokeObjectPostAlterHook(RelationRelationId, relid, 0);
...@@ -1204,19 +1216,28 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple) ...@@ -1204,19 +1216,28 @@ read_seq_tuple(Relation rel, Buffer *buf, HeapTuple seqdatatuple)
/* /*
* init_params: process the options list of CREATE or ALTER SEQUENCE, and * init_params: process the options list of CREATE or ALTER SEQUENCE, and
* store the values into appropriate fields of seqform, for changes that go * store the values into appropriate fields of seqform, for changes that go
* into the pg_sequence catalog, and seqdataform for changes to the sequence * into the pg_sequence catalog, and fields of seqdataform for changes to the
* relation itself. Set *changed_seqform to true if seqform was changed * sequence relation itself. Set *need_seq_rewrite to true if we changed any
* (interesting for ALTER SEQUENCE). Also set *owned_by to any OWNED BY * parameters that require rewriting the sequence's relation (interesting for
* option, or to NIL if there is none. * ALTER SEQUENCE). Also set *owned_by to any OWNED BY option, or to NIL if
* there is none.
* *
* If isInit is true, fill any unspecified options with default values; * If isInit is true, fill any unspecified options with default values;
* otherwise, do not change existing options that aren't explicitly overridden. * otherwise, do not change existing options that aren't explicitly overridden.
*
* Note: we force a sequence rewrite whenever we change parameters that affect
* generation of future sequence values, even if the seqdataform per se is not
* changed. This allows ALTER SEQUENCE to behave transactionally. Currently,
* the only option that doesn't cause that is OWNED BY. It's *necessary* for
* ALTER SEQUENCE OWNED BY to not rewrite the sequence, because that would
* break pg_upgrade by causing unwanted changes in the sequence's relfilenode.
*/ */
static void static void
init_params(ParseState *pstate, List *options, bool for_identity, init_params(ParseState *pstate, List *options, bool for_identity,
bool isInit, bool isInit,
Form_pg_sequence seqform, Form_pg_sequence seqform,
Form_pg_sequence_data seqdataform, Form_pg_sequence_data seqdataform,
bool *need_seq_rewrite,
List **owned_by) List **owned_by)
{ {
DefElem *as_type = NULL; DefElem *as_type = NULL;
...@@ -1231,6 +1252,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1231,6 +1252,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
bool reset_max_value = false; bool reset_max_value = false;
bool reset_min_value = false; bool reset_min_value = false;
*need_seq_rewrite = false;
*owned_by = NIL; *owned_by = NIL;
foreach(option, options) foreach(option, options)
...@@ -1245,6 +1267,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1245,6 +1267,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
as_type = defel; as_type = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "increment") == 0) else if (strcmp(defel->defname, "increment") == 0)
{ {
...@@ -1254,6 +1277,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1254,6 +1277,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
increment_by = defel; increment_by = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "start") == 0) else if (strcmp(defel->defname, "start") == 0)
{ {
...@@ -1263,6 +1287,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1263,6 +1287,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
start_value = defel; start_value = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "restart") == 0) else if (strcmp(defel->defname, "restart") == 0)
{ {
...@@ -1272,6 +1297,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1272,6 +1297,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
restart_value = defel; restart_value = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "maxvalue") == 0) else if (strcmp(defel->defname, "maxvalue") == 0)
{ {
...@@ -1281,6 +1307,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1281,6 +1307,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
max_value = defel; max_value = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "minvalue") == 0) else if (strcmp(defel->defname, "minvalue") == 0)
{ {
...@@ -1290,6 +1317,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1290,6 +1317,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
min_value = defel; min_value = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "cache") == 0) else if (strcmp(defel->defname, "cache") == 0)
{ {
...@@ -1299,6 +1327,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1299,6 +1327,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
cache_value = defel; cache_value = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "cycle") == 0) else if (strcmp(defel->defname, "cycle") == 0)
{ {
...@@ -1308,6 +1337,7 @@ init_params(ParseState *pstate, List *options, bool for_identity, ...@@ -1308,6 +1337,7 @@ init_params(ParseState *pstate, List *options, bool for_identity,
errmsg("conflicting or redundant options"), errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location))); parser_errposition(pstate, defel->location)));
is_cycled = defel; is_cycled = defel;
*need_seq_rewrite = true;
} }
else if (strcmp(defel->defname, "owned_by") == 0) else if (strcmp(defel->defname, "owned_by") == 0)
{ {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment