Skip to content

Commit 918e21d

Browse files
tglsfdctglsfdc
committed
Repair pg_upgrade for identity sequences with non-default persistence.
Since we introduced unlogged sequences in v15, identity sequences have defaulted to having the same persistence as their owning table. However, it is possible to change that with ALTER SEQUENCE, and pg_dump tries to preserve the logged-ness of sequences when it doesn't match (as indeed it wouldn't for an unlogged table from before v15). The fly in the ointment is that ALTER SEQUENCE SET [UN]LOGGED fails in binary-upgrade mode, because it needs to assign a new relfilenode which we cannot permit in that mode. Thus, trying to pg_upgrade a database containing a mismatching identity sequence failed. To fix, add syntax to ADD/ALTER COLUMN GENERATED AS IDENTITY to allow the sequence's persistence to be set correctly at creation, and use that instead of ALTER SEQUENCE SET [UN]LOGGED in pg_dump. (I tried to make SET [UN]LOGGED work without any pg_dump modifications, but that seems too fragile to be a desirable answer. This way should be markedly faster anyhow.) In passing, document the previously-undocumented SEQUENCE NAME option that pg_dump also relies on for identity sequences; I see no value in trying to pretend it doesn't exist. Per bug #18618 from Anthony Hsu. Back-patch to v15 where we invented this stuff. Discussion: https://postgr.es/m/18618-d4eb26d669ed110a@postgresql.org
1 parent 2520226 commit 918e21d

File tree

7 files changed

+112
-41
lines changed

7 files changed

+112
-41
lines changed

‎doc/src/sgml/ref/create_table.sgml

+12-5
Original file line numberDiff line numberDiff line change
@@ -924,8 +924,8 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
924924
<para>
925925
This clause creates the column as an <firstterm>identity
926926
column</firstterm>. It will have an implicit sequence attached to it
927-
and the column in new rows will automatically have values from the
928-
sequence assigned to it.
927+
and in newly-inserted rows the column will automatically have values
928+
from the sequence assigned to it.
929929
Such a column is implicitly <literal>NOT NULL</literal>.
930930
</para>
931931

@@ -955,9 +955,16 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
955955
</para>
956956

957957
<para>
958-
The optional <replaceable>sequence_options</replaceable> clause can be
959-
used to override the options of the sequence.
960-
See <xref linkend="sql-createsequence"/> for details.
958+
The optional <replaceable>sequence_options</replaceable> clause can
959+
be used to override the parameters of the sequence. The available
960+
options include those shown for <xref linkend="sql-createsequence"/>,
961+
plus <literal>SEQUENCE NAME <replaceable>name</replaceable></literal>,
962+
<literal>LOGGED</literal>, and <literal>UNLOGGED</literal>, which
963+
allow selection of the name and persistence level of the
964+
sequence. Without <literal>SEQUENCE NAME</literal>, the system
965+
chooses an unused name for the sequence.
966+
Without <literal>LOGGED</literal> or <literal>UNLOGGED</literal>,
967+
the sequence will have the same persistence level as the table.
961968
</para>
962969
</listitem>
963970
</varlistentry>

‎src/backend/commands/sequence.c

+4-1
Original file line numberDiff line numberDiff line change
@@ -1347,7 +1347,10 @@ init_params(ParseState *pstate, List *options, bool for_identity,
13471347
/*
13481348
* The parser allows this, but it is only for identity columns, in
13491349
* which case it is filtered out in parse_utilcmd.c. We only get
1350-
* here if someone puts it into a CREATE SEQUENCE.
1350+
* here if someone puts it into a CREATE SEQUENCE, where it'd be
1351+
* redundant. (The same is true for the equally-nonstandard
1352+
* LOGGED and UNLOGGED options, but for those, the default error
1353+
* below seems sufficient.)
13511354
*/
13521355
ereport(ERROR,
13531356
(errcode(ERRCODE_SYNTAX_ERROR),

‎src/backend/parser/gram.y

+8-1
Original file line numberDiff line numberDiff line change
@@ -4936,6 +4936,10 @@ SeqOptElem: AS SimpleTypename
49364936
{
49374937
$$ = makeDefElem("increment", (Node *) $3, @1);
49384938
}
4939+
| LOGGED
4940+
{
4941+
$$ = makeDefElem("logged", NULL, @1);
4942+
}
49394943
| MAXVALUE NumericOnly
49404944
{
49414945
$$ = makeDefElem("maxvalue", (Node *) $2, @1);
@@ -4958,7 +4962,6 @@ SeqOptElem: AS SimpleTypename
49584962
}
49594963
| SEQUENCE NAME_P any_name
49604964
{
4961-
/* not documented, only used by pg_dump */
49624965
$$ = makeDefElem("sequence_name", (Node *) $3, @1);
49634966
}
49644967
| START opt_with NumericOnly
@@ -4973,6 +4976,10 @@ SeqOptElem: AS SimpleTypename
49734976
{
49744977
$$ = makeDefElem("restart", (Node *) $3, @1);
49754978
}
4979+
| UNLOGGED
4980+
{
4981+
$$ = makeDefElem("unlogged", NULL, @1);
4982+
}
49764983
;
49774984

49784985
opt_by: BY

‎src/backend/parser/parse_utilcmd.c

+54-26
Original file line numberDiff line numberDiff line change
@@ -365,30 +365,22 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
365365
{
366366
ListCell *option;
367367
DefElem *nameEl = NULL;
368+
DefElem *loggedEl = NULL;
368369
Oid snamespaceid;
369370
char *snamespace;
370371
char *sname;
372+
char seqpersistence;
371373
CreateSeqStmt *seqstmt;
372374
AlterSeqStmt *altseqstmt;
373375
List *attnamelist;
374-
int nameEl_idx = -1;
375376

376377
/* Make a copy of this as we may end up modifying it in the code below */
377378
seqoptions = list_copy(seqoptions);
378379

379380
/*
380-
* Determine namespace and name to use for the sequence.
381-
*
382-
* First, check if a sequence name was passed in as an option. This is
383-
* used by pg_dump. Else, generate a name.
384-
*
385-
* Although we use ChooseRelationName, it's not guaranteed that the
386-
* selected sequence name won't conflict; given sufficiently long field
387-
* names, two different serial columns in the same table could be assigned
388-
* the same sequence name, and we'd not notice since we aren't creating
389-
* the sequence quite yet. In practice this seems quite unlikely to be a
390-
* problem, especially since few people would need two serial columns in
391-
* one table.
381+
* Check for non-SQL-standard options (not supported within CREATE
382+
* SEQUENCE, because they'd be redundant), and remove them from the
383+
* seqoptions list if found.
392384
*/
393385
foreach(option, seqoptions)
394386
{
@@ -399,12 +391,24 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
399391
if (nameEl)
400392
errorConflictingDefElem(defel, cxt->pstate);
401393
nameEl = defel;
402-
nameEl_idx = foreach_current_index(option);
394+
seqoptions = foreach_delete_current(seqoptions, option);
395+
}
396+
else if (strcmp(defel->defname, "logged") == 0 ||
397+
strcmp(defel->defname, "unlogged") == 0)
398+
{
399+
if (loggedEl)
400+
errorConflictingDefElem(defel, cxt->pstate);
401+
loggedEl = defel;
402+
seqoptions = foreach_delete_current(seqoptions, option);
403403
}
404404
}
405405

406+
/*
407+
* Determine namespace and name to use for the sequence.
408+
*/
406409
if (nameEl)
407410
{
411+
/* Use specified name */
408412
RangeVar *rv = makeRangeVarFromNameList(castNode(List, nameEl->arg));
409413

410414
snamespace = rv->schemaname;
@@ -418,11 +422,20 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
418422
snamespace = get_namespace_name(snamespaceid);
419423
}
420424
sname = rv->relname;
421-
/* Remove the SEQUENCE NAME item from seqoptions */
422-
seqoptions = list_delete_nth_cell(seqoptions, nameEl_idx);
423425
}
424426
else
425427
{
428+
/*
429+
* Generate a name.
430+
*
431+
* Although we use ChooseRelationName, it's not guaranteed that the
432+
* selected sequence name won't conflict; given sufficiently long
433+
* field names, two different serial columns in the same table could
434+
* be assigned the same sequence name, and we'd not notice since we
435+
* aren't creating the sequence quite yet. In practice this seems
436+
* quite unlikely to be a problem, especially since few people would
437+
* need two serial columns in one table.
438+
*/
426439
if (cxt->rel)
427440
snamespaceid = RelationGetNamespace(cxt->rel);
428441
else
@@ -443,23 +456,38 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
443456
cxt->stmtType, sname,
444457
cxt->relation->relname, column->colname)));
445458

459+
/*
460+
* Determine the persistence of the sequence. By default we copy the
461+
* persistence of the table, but if LOGGED or UNLOGGED was specified, use
462+
* that (as long as the table isn't TEMP).
463+
*
464+
* For CREATE TABLE, we get the persistence from cxt->relation, which
465+
* comes from the CreateStmt in progress. For ALTER TABLE, the parser
466+
* won't set cxt->relation->relpersistence, but we have cxt->rel as the
467+
* existing table, so we copy the persistence from there.
468+
*/
469+
seqpersistence = cxt->rel ? cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
470+
if (loggedEl)
471+
{
472+
if (seqpersistence == RELPERSISTENCE_TEMP)
473+
ereport(ERROR,
474+
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
475+
errmsg("cannot set logged status of a temporary sequence"),
476+
parser_errposition(cxt->pstate, loggedEl->location)));
477+
else if (strcmp(loggedEl->defname, "logged") == 0)
478+
seqpersistence = RELPERSISTENCE_PERMANENT;
479+
else
480+
seqpersistence = RELPERSISTENCE_UNLOGGED;
481+
}
482+
446483
/*
447484
* Build a CREATE SEQUENCE command to create the sequence object, and add
448485
* it to the list of things to be done before this CREATE/ALTER TABLE.
449486
*/
450487
seqstmt = makeNode(CreateSeqStmt);
451488
seqstmt->for_identity = for_identity;
452489
seqstmt->sequence = makeRangeVar(snamespace, sname, -1);
453-
454-
/*
455-
* Copy the persistence of the table. For CREATE TABLE, we get the
456-
* persistence from cxt->relation, which comes from the CreateStmt in
457-
* progress. For ALTER TABLE, the parser won't set
458-
* cxt->relation->relpersistence, but we have cxt->rel as the existing
459-
* table, so we copy the persistence from there.
460-
*/
461-
seqstmt->sequence->relpersistence = cxt->rel ? cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
462-
490+
seqstmt->sequence->relpersistence = seqpersistence;
463491
seqstmt->options = seqoptions;
464492

465493
/*

‎src/bin/pg_dump/pg_dump.c

+9-8
Original file line numberDiff line numberDiff line change
@@ -17568,6 +17568,15 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
1756817568
appendPQExpBufferStr(query, "BY DEFAULT");
1756917569
appendPQExpBuffer(query, " AS IDENTITY (\n SEQUENCE NAME %s\n",
1757017570
fmtQualifiedDumpable(tbinfo));
17571+
17572+
/*
17573+
* Emit persistence option only if it's different from the owning
17574+
* table's. This avoids using this new syntax unnecessarily.
17575+
*/
17576+
if (tbinfo->relpersistence != owning_tab->relpersistence)
17577+
appendPQExpBuffer(query, " %s\n",
17578+
tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
17579+
"UNLOGGED" : "LOGGED");
1757117580
}
1757217581
else
1757317582
{
@@ -17600,15 +17609,7 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
1760017609
seq->cache, (seq->cycled ? "\n CYCLE" : ""));
1760117610

1760217611
if (tbinfo->is_identity_sequence)
17603-
{
1760417612
appendPQExpBufferStr(query, "\n);\n");
17605-
if (tbinfo->relpersistence != owning_tab->relpersistence)
17606-
appendPQExpBuffer(query,
17607-
"ALTER SEQUENCE %s SET %s;\n",
17608-
fmtQualifiedDumpable(tbinfo),
17609-
tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
17610-
"UNLOGGED" : "LOGGED");
17611-
}
1761217613
else
1761317614
appendPQExpBufferStr(query, ";\n");
1761417615

‎src/test/regress/expected/identity.out

+16
Original file line numberDiff line numberDiff line change
@@ -905,3 +905,19 @@ SELECT * FROM itest16;
905905

906906
DROP TABLE itest15;
907907
DROP TABLE itest16;
908+
-- For testing of pg_dump and pg_upgrade, leave behind some identity
909+
-- sequences whose logged-ness doesn't match their owning table's.
910+
CREATE TABLE identity_dump_logged (a INT GENERATED ALWAYS AS IDENTITY);
911+
ALTER SEQUENCE identity_dump_logged_a_seq SET UNLOGGED;
912+
CREATE UNLOGGED TABLE identity_dump_unlogged (a INT GENERATED ALWAYS AS IDENTITY);
913+
ALTER SEQUENCE identity_dump_unlogged_a_seq SET LOGGED;
914+
SELECT relname, relpersistence FROM pg_class
915+
WHERE relname ~ '^identity_dump_' ORDER BY 1;
916+
relname | relpersistence
917+
------------------------------+----------------
918+
identity_dump_logged | p
919+
identity_dump_logged_a_seq | u
920+
identity_dump_unlogged | u
921+
identity_dump_unlogged_a_seq | p
922+
(4 rows)
923+

‎src/test/regress/sql/identity.sql

+9
Original file line numberDiff line numberDiff line change
@@ -528,3 +528,12 @@ SELECT * FROM itest15;
528528
SELECT * FROM itest16;
529529
DROP TABLE itest15;
530530
DROP TABLE itest16;
531+
532+
-- For testing of pg_dump and pg_upgrade, leave behind some identity
533+
-- sequences whose logged-ness doesn't match their owning table's.
534+
CREATE TABLE identity_dump_logged (a INT GENERATED ALWAYS AS IDENTITY);
535+
ALTER SEQUENCE identity_dump_logged_a_seq SET UNLOGGED;
536+
CREATE UNLOGGED TABLE identity_dump_unlogged (a INT GENERATED ALWAYS AS IDENTITY);
537+
ALTER SEQUENCE identity_dump_unlogged_a_seq SET LOGGED;
538+
SELECT relname, relpersistence FROM pg_class
539+
WHERE relname ~ '^identity_dump_' ORDER BY 1;

0 commit comments

Comments
 (0)