Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dKCpW-0001jv-V0 for pgsql-hackers@arkaria.postgresql.org; Mon, 12 Jun 2017 00:03:39 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1dKCpV-0008CI-Tv for pgsql-hackers@arkaria.postgresql.org; Mon, 12 Jun 2017 00:03:37 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dKCpU-0008C6-5b for pgsql-hackers@postgresql.org; Mon, 12 Jun 2017 00:03:36 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dKCpQ-0000es-Jl for pgsql-hackers@postgresql.org; Mon, 12 Jun 2017 00:03:35 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id v5C03Sch009762; Sun, 11 Jun 2017 20:03:28 -0400 From: Tom Lane To: Andres Freund cc: Peter Eisentraut , pgsql-hackers@postgresql.org Subject: Re: Transactional sequence stuff breaks pg_upgrade In-reply-to: <19785.1497215827@sss.pgh.pa.us> References: <19785.1497215827@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Sun, 11 Jun 2017 17:17:07 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <9707.1497225710.0@sss.pgh.pa.us> Date: Sun, 11 Jun 2017 20:03:28 -0400 Message-ID: <9761.1497225808@sss.pgh.pa.us> List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgsql-hackers Precedence: bulk Sender: pgsql-hackers-owner@postgresql.org ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <9707.1497225710.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-sequence-upgrade-v1.patch"; charset="us-ascii" Content-ID: <9707.1497225710.2@sss.pgh.pa.us> Content-Description: fix-sequence-upgrade-v1.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 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; =20=20 + /* + * 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); +=20 switch (relpersistence) { case RELPERSISTENCE_TEMP: diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequenc= e.c index 7e85b69..188429c 100644 *** a/src/backend/commands/sequence.c --- b/src/backend/commands/sequence.c *************** AlterSequence(ParseState *pstate, AlterS *** 473,490 **** GetTopTransactionId(); =20=20 /* ! * Create a new storage file for the sequence, making the state changes ! * transactional. We want to keep the sequence's relfrozenxid at 0, sin= ce ! * 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); =20=20 ! /* ! * Insert the modified tuple into the new storage file. ! */ ! fill_seq_with_data(seqrel, newdatatuple); =20=20 /* process OWNED BY if given */ if (owned_by) --- 473,499 ---- GetTopTransactionId(); =20=20 /* ! * 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) =3D=3D 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); =20=20 ! /* ! * Insert the modified tuple into the new storage file. ! */ ! fill_seq_with_data(seqrel, newdatatuple); ! } =20=20 /* process OWNED BY if given */ if (owned_by) ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-sequence-upgrade-v2.patch"; charset="us-ascii" Content-ID: <9707.1497225710.3@sss.pgh.pa.us> Content-Description: fix-sequence-upgrade-v2.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c index 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; =20=20 + /* + * 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); +=20 switch (relpersistence) { case RELPERSISTENCE_TEMP: diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequenc= e.c index 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_ident= ity, 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_id= entity); =20=20 --- 101,109 ---- static void init_params(ParseState *pstate, List *options, bool for_ident= ity, 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_id= entity); =20=20 *************** 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 =3D makeNode(CreateStmt); Oid seqoid; *************** DefineSequence(ParseState *pstate, Creat *** 153,159 **** } =20=20 /* Check and set all option values */ ! init_params(pstate, seq->options, seq->for_identity, true, &seqform, &se= qdataform, &owned_by); =20=20 /* * Create relation (and fill value[] and null[] for the tuple) --- 156,164 ---- } =20=20 /* Check and set all option values */ ! init_params(pstate, seq->options, seq->for_identity, true, ! &seqform, &seqdataform, ! &need_newrelfilenode, &owned_by); =20=20 /* * 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); =20=20 /* Check and set new values */ ! init_params(pstate, stmt->options, stmt->for_identity, false, seqform, ! newdataform, &owned_by); =20=20 /* 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); =20=20 /* Check and set new values */ ! init_params(pstate, stmt->options, stmt->for_identity, false, ! seqform, newdataform, ! &need_newrelfilenode, &owned_by); =20=20 /* 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(); =20=20 /* ! * Create a new storage file for the sequence, making the state changes ! * transactional. We want to keep the sequence's relfrozenxid at 0, sin= ce ! * 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); =20=20 ! /* ! * Insert the modified tuple into the new storage file. ! */ ! fill_seq_with_data(seqrel, newdatatuple); =20=20 /* process OWNED BY if given */ if (owned_by) --- 480,500 ---- GetTopTransactionId(); =20=20 /* ! * If needed, create a new storage file for the sequence, making the sta= te ! * 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); =20=20 ! /* ! * Insert the modified tuple into the new storage file. ! */ ! fill_seq_with_data(seqrel, newdatatuple); ! } =20=20 /* 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 seque= nce ! * 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 overr= idden. --- 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 seque= nce ! * relation itself. Set *need_newrelfilenode to true if we changed any ! * parameters that require assigning a new sequence relfilenode (interest= ing ! * 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 overr= idden. *************** 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 =3D NULL; *************** init_params(ParseState *pstate, List *op *** 1231,1236 **** --- 1243,1249 ---- bool reset_max_value =3D false; bool reset_min_value =3D false; =20=20 + *need_newrelfilenode =3D false; *owned_by =3D NIL; =20=20 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 =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "increment") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1254,1259 **** --- 1268,1274 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); increment_by =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "start") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1263,1268 **** --- 1278,1284 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); start_value =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "restart") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1272,1277 **** --- 1288,1294 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); restart_value =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "maxvalue") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1281,1286 **** --- 1298,1304 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); max_value =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "minvalue") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1290,1295 **** --- 1308,1314 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); min_value =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "cache") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1299,1304 **** --- 1318,1324 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); cache_value =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "cycle") =3D=3D 0) { *************** init_params(ParseState *pstate, List *op *** 1308,1313 **** --- 1328,1334 ---- errmsg("conflicting or redundant options"), parser_errposition(pstate, defel->location))); is_cycled =3D defel; + *need_newrelfilenode =3D true; } else if (strcmp(defel->defname, "owned_by") =3D=3D 0) { ------- =_aaaaaaaaaa0 Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ------- =_aaaaaaaaaa0--