From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Transactional sequence stuff breaks pg_upgrade
Date: Sun, 11 Jun 2017 20:03:28 -0400
Message-ID: <9761.1497225808@sss.pgh.pa.us> (raw)
In-Reply-To: <19785.1497215827@sss.pgh.pa.us>
References: <19785.1497215827@sss.pgh.pa.us>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
I wrote:
> I believe I've identified the reason why skink and some other buildfarm
> members have been failing the pg_upgrade test recently.
> ...
> Not sure what we want to do about it. One idea is to make
> ALTER SEQUENCE not so transactional when in binary-upgrade mode.
On closer inspection, the only thing that pg_upgrade needs is to be
able to do ALTER SEQUENCE OWNED BY without a relfilenode bump. PFA
two versions of a patch that fixes this problem, at least to the
extent that it gets through check-world without triggering the Assert
I added to GetNewRelFileNode (which HEAD doesn't). The first one
is a minimally-invasive hack; the second one puts the responsibility
for deciding if a sequence rewrite is needed into init_params. That's
bulkier but might be useful if we ever grow additional ALTER SEQUENCE
options that don't need a rewrite. OTOH, I'm not very clear on what
such options might look like, so maybe the extra flexibility is useless.
Thoughts?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:download all (2 files, .tar.gz)fix-sequence-upgrade-v1.patch (text/x-diff, 2.4K) downloaddiff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.cindex 11ee536..43ef4cd 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
*************** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 ****
--- 391,403 ----
bool collides;
BackendId backend;
+ /*+ * If we ever get here during pg_upgrade, there's something wrong; all+ * relfilenode assignments during a binary-upgrade run should be+ * determined by commands in the dump script.+ */+ Assert(!IsBinaryUpgrade);+
switch (relpersistence)
{
case RELPERSISTENCE_TEMP:
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.cindex 7e85b69..188429c 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*************** AlterSequence(ParseState *pstate, AlterS
*** 473,490 ****
GetTopTransactionId();
/*
! * 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);
! /*
! * Insert the modified tuple into the new storage file.
! */
! fill_seq_with_data(seqrel, newdatatuple);
/* process OWNED BY if given */
if (owned_by)
--- 473,499 ----
GetTopTransactionId();
/*
! * If we are *only* doing OWNED BY, there is no need to rewrite the
! * sequence file nor the pg_sequence tuple; and we mustn't do so because
! * it breaks pg_upgrade by causing unwanted changes in the sequence's
! * relfilenode.
*/
! if (!(owned_by && list_length(stmt->options) == 1))
! {
! /*
! * 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);
! /*
! * Insert the modified tuple into the new storage file.
! */
! fill_seq_with_data(seqrel, newdatatuple);
! }
/* process OWNED BY if given */
if (owned_by)
fix-sequence-upgrade-v2.patch (text/x-diff, 9.1K) downloaddiff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.cindex 11ee536..43ef4cd 100644
*** a/src/backend/catalog/catalog.c
--- b/src/backend/catalog/catalog.c
*************** GetNewRelFileNode(Oid reltablespace, Rel
*** 391,396 ****
--- 391,403 ----
bool collides;
BackendId backend;
+ /*+ * If we ever get here during pg_upgrade, there's something wrong; all+ * relfilenode assignments during a binary-upgrade run should be+ * determined by commands in the dump script.+ */+ Assert(!IsBinaryUpgrade);+
switch (relpersistence)
{
case RELPERSISTENCE_TEMP:
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.cindex 7e85b69..34b8aa2 100644
*** a/src/backend/commands/sequence.c
--- b/src/backend/commands/sequence.c
*************** static Form_pg_sequence_data read_seq_tu
*** 101,107 ****
static void init_params(ParseState *pstate, List *options, bool for_identity,
bool isInit,
Form_pg_sequence 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);
--- 101,109 ----
static void init_params(ParseState *pstate, List *options, bool for_identity,
bool isInit,
Form_pg_sequence seqform,
! Form_pg_sequence_data seqdataform,
! bool *need_newrelfilenode,
! 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);
*************** DefineSequence(ParseState *pstate, Creat
*** 115,120 ****
--- 117,123 ----
{
FormData_pg_sequence seqform;
FormData_pg_sequence_data seqdataform;
+ bool need_newrelfilenode;
List *owned_by;
CreateStmt *stmt = makeNode(CreateStmt);
Oid seqoid;
*************** DefineSequence(ParseState *pstate, Creat
*** 153,159 ****
}
/* Check and set all option values */
! init_params(pstate, seq->options, seq->for_identity, true, &seqform, &seqdataform, &owned_by);
/*
* Create relation (and fill value[] and null[] for the tuple)
--- 156,164 ----
}
/* Check and set all option values */
! init_params(pstate, seq->options, seq->for_identity, true,
! &seqform, &seqdataform,
! &need_newrelfilenode, &owned_by);
/*
* Create relation (and fill value[] and null[] for the tuple)
*************** AlterSequence(ParseState *pstate, AlterS
*** 417,422 ****
--- 422,428 ----
HeapTupleData datatuple;
Form_pg_sequence seqform;
Form_pg_sequence_data newdataform;
+ bool need_newrelfilenode;
List *owned_by;
ObjectAddress address;
Relation rel;
*************** AlterSequence(ParseState *pstate, AlterS
*** 461,468 ****
UnlockReleaseBuffer(buf);
/* Check and set new values */
! 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 */
--- 467,475 ----
UnlockReleaseBuffer(buf);
/* Check and set new values */
! init_params(pstate, stmt->options, stmt->for_identity, false,
! seqform, newdataform,
! &need_newrelfilenode, &owned_by);
/* Clear local cache so that we don't think we have cached numbers */
/* Note that we do not change the currval() state */
*************** AlterSequence(ParseState *pstate, AlterS
*** 473,490 ****
GetTopTransactionId();
/*
! * 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);
! /*
! * Insert the modified tuple into the new storage file.
! */
! fill_seq_with_data(seqrel, newdatatuple);
/* process OWNED BY if given */
if (owned_by)
--- 480,500 ----
GetTopTransactionId();
/*
! * If needed, 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.
*/
! if (need_newrelfilenode)
! {
! RelationSetNewRelfilenode(seqrel, seqrel->rd_rel->relpersistence,
! InvalidTransactionId, InvalidMultiXactId);
! /*
! * Insert the modified tuple into the new storage file.
! */
! fill_seq_with_data(seqrel, newdatatuple);
! }
/* process OWNED BY if given */
if (owned_by)
*************** read_seq_tuple(Relation rel, Buffer *buf
*** 1205,1213 ****
* init_params: process the options list of CREATE or ALTER SEQUENCE, and
* store the values into appropriate fields of seqform, for changes that go
* into the pg_sequence catalog, and seqdataform for changes to the sequence
! * relation itself. Set *changed_seqform to true if seqform was changed
! * (interesting for 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;
* otherwise, do not change existing options that aren't explicitly overridden.
--- 1215,1224 ----
* init_params: process the options list of CREATE or ALTER SEQUENCE, and
* store the values into appropriate fields of seqform, for changes that go
* into the pg_sequence catalog, and seqdataform for changes to the sequence
! * relation itself. Set *need_newrelfilenode to true if we changed any
! * parameters that require assigning a new sequence relfilenode (interesting
! * for 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;
* otherwise, do not change existing options that aren't explicitly overridden.
*************** init_params(ParseState *pstate, List *op
*** 1217,1222 ****
--- 1228,1234 ----
bool isInit,
Form_pg_sequence seqform,
Form_pg_sequence_data seqdataform,
+ bool *need_newrelfilenode,
List **owned_by)
{
DefElem *as_type = NULL;
*************** init_params(ParseState *pstate, List *op
*** 1231,1236 ****
--- 1243,1249 ----
bool reset_max_value = false;
bool reset_min_value = false;
+ *need_newrelfilenode = false;
*owned_by = NIL;
foreach(option, options)
*************** init_params(ParseState *pstate, List *op
*** 1245,1250 ****
--- 1258,1264 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
as_type = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "increment") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1254,1259 ****
--- 1268,1274 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
increment_by = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "start") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1263,1268 ****
--- 1278,1284 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
start_value = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "restart") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1272,1277 ****
--- 1288,1294 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
restart_value = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "maxvalue") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1281,1286 ****
--- 1298,1304 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
max_value = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "minvalue") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1290,1295 ****
--- 1308,1314 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
min_value = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "cache") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1299,1304 ****
--- 1318,1324 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
cache_value = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "cycle") == 0)
{
*************** init_params(ParseState *pstate, List *op
*** 1308,1313 ****
--- 1328,1334 ----
errmsg("conflicting or redundant options"),
parser_errposition(pstate, defel->location)));
is_cycled = defel;
+ *need_newrelfilenode = true;
}
else if (strcmp(defel->defname, "owned_by") == 0)
{
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, peter.eisentraut@2ndquadrant.com
Subject: Re: Transactional sequence stuff breaks pg_upgrade
In-Reply-To: <9761.1497225808@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox