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

diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 8af49908c5a653e9d2da6175a5b4a0ff2a906dc9..31c3762cbeefe5c479fbc84ec8d9f40c9571c7c1 100644 (file)
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -895,8 +895,8 @@ WITH ( MODULUS numeric_literal, REM
      
       This clause creates the column as an identity
       column.  It will have an implicit sequence attached to it
-      and the column in new rows will automatically have values from the
-      sequence assigned to it.
+      and in newly-inserted rows the column will automatically have values
+      from the sequence assigned to it.
       Such a column is implicitly NOT NULL.
      
 
@@ -926,9 +926,16 @@ WITH ( MODULUS numeric_literal, REM
      
 
      
-      The optional sequence_options clause can be
-      used to override the options of the sequence.
-      See  for details.
+      The optional sequence_options clause can
+      be used to override the parameters of the sequence.  The available
+      options include those shown for ,
+      plus SEQUENCE NAME name,
+      LOGGED, and UNLOGGED, which
+      allow selection of the name and persistence level of the
+      sequence.  Without SEQUENCE NAME, the system
+      chooses an unused name for the sequence.
+      Without LOGGED or UNLOGGED,
+      the sequence will have the same persistence level as the table.
      
     
    

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 0a1019d6abff3a9600e6c80ac1ae7cdb0bb60033..fbcfcddb59eee0093ddf2898ac578321a943e2a3 100644 (file)
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1365,7 +1365,10 @@ init_params(ParseState *pstate, List *options, bool for_identity,
            /*
             * The parser allows this, but it is only for identity columns, in
             * which case it is filtered out in parse_utilcmd.c.  We only get
-            * here if someone puts it into a CREATE SEQUENCE.
+            * here if someone puts it into a CREATE SEQUENCE, where it'd be
+            * redundant.  (The same is true for the equally-nonstandard
+            * LOGGED and UNLOGGED options, but for those, the default error
+            * below seems sufficient.)
             */
            ereport(ERROR,
                    (errcode(ERRCODE_SYNTAX_ERROR),

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 2ece5d71d1a7261ef5392cde7869c2b320e32099..5433a264cff7942761524bab1b850c9632f6743f 100644 (file)
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -4738,6 +4738,10 @@ SeqOptElem: AS SimpleTypename
                {
                    $$ = makeDefElem("increment", (Node *) $3, @1);
                }
+           | LOGGED
+               {
+                   $$ = makeDefElem("logged", NULL, @1);
+               }
            | MAXVALUE NumericOnly
                {
                    $$ = makeDefElem("maxvalue", (Node *) $2, @1);
@@ -4760,7 +4764,6 @@ SeqOptElem: AS SimpleTypename
                }
            | SEQUENCE NAME_P any_name
                {
-                   /* not documented, only used by pg_dump */
                    $$ = makeDefElem("sequence_name", (Node *) $3, @1);
                }
            | START opt_with NumericOnly
@@ -4775,6 +4778,10 @@ SeqOptElem: AS SimpleTypename
                {
                    $$ = makeDefElem("restart", (Node *) $3, @1);
                }
+           | UNLOGGED
+               {
+                   $$ = makeDefElem("unlogged", NULL, @1);
+               }
        ;
 
 opt_by:        BY

diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index cd23eedddb80aff8d62e5a761d9e0ffb2584e1f3..4f416ed098fee20c4559667276207dac42370091 100644 (file)
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -365,30 +365,22 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
 {
    ListCell   *option;
    DefElem    *nameEl = NULL;
+   DefElem    *loggedEl = NULL;
    Oid         snamespaceid;
    char       *snamespace;
    char       *sname;
+   char        seqpersistence;
    CreateSeqStmt *seqstmt;
    AlterSeqStmt *altseqstmt;
    List       *attnamelist;
-   int         nameEl_idx = -1;
 
    /* Make a copy of this as we may end up modifying it in the code below */
    seqoptions = list_copy(seqoptions);
 
    /*
-    * Determine namespace and name to use for the sequence.
-    *
-    * First, check if a sequence name was passed in as an option.  This is
-    * used by pg_dump.  Else, generate a name.
-    *
-    * Although we use ChooseRelationName, it's not guaranteed that the
-    * selected sequence name won't conflict; given sufficiently long field
-    * names, two different serial columns in the same table could be assigned
-    * the same sequence name, and we'd not notice since we aren't creating
-    * the sequence quite yet.  In practice this seems quite unlikely to be a
-    * problem, especially since few people would need two serial columns in
-    * one table.
+    * Check for non-SQL-standard options (not supported within CREATE
+    * SEQUENCE, because they'd be redundant), and remove them from the
+    * seqoptions list if found.
     */
    foreach(option, seqoptions)
    {
@@ -399,12 +391,24 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
            if (nameEl)
                errorConflictingDefElem(defel, cxt->pstate);
            nameEl = defel;
-           nameEl_idx = foreach_current_index(option);
+           seqoptions = foreach_delete_current(seqoptions, option);
+       }
+       else if (strcmp(defel->defname, "logged") == 0 ||
+                strcmp(defel->defname, "unlogged") == 0)
+       {
+           if (loggedEl)
+               errorConflictingDefElem(defel, cxt->pstate);
+           loggedEl = defel;
+           seqoptions = foreach_delete_current(seqoptions, option);
        }
    }
 
+   /*
+    * Determine namespace and name to use for the sequence.
+    */
    if (nameEl)
    {
+       /* Use specified name */
        RangeVar   *rv = makeRangeVarFromNameList(castNode(List, nameEl->arg));
 
        snamespace = rv->schemaname;
@@ -418,11 +422,20 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
            snamespace = get_namespace_name(snamespaceid);
        }
        sname = rv->relname;
-       /* Remove the SEQUENCE NAME item from seqoptions */
-       seqoptions = list_delete_nth_cell(seqoptions, nameEl_idx);
    }
    else
    {
+       /*
+        * Generate a name.
+        *
+        * Although we use ChooseRelationName, it's not guaranteed that the
+        * selected sequence name won't conflict; given sufficiently long
+        * field names, two different serial columns in the same table could
+        * be assigned the same sequence name, and we'd not notice since we
+        * aren't creating the sequence quite yet.  In practice this seems
+        * quite unlikely to be a problem, especially since few people would
+        * need two serial columns in one table.
+        */
        if (cxt->rel)
            snamespaceid = RelationGetNamespace(cxt->rel);
        else
@@ -443,6 +456,30 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
                             cxt->stmtType, sname,
                             cxt->relation->relname, column->colname)));
 
+   /*
+    * Determine the persistence of the sequence.  By default we copy the
+    * persistence of the table, but if LOGGED or UNLOGGED was specified, use
+    * that (as long as the table isn't TEMP).
+    *
+    * For CREATE TABLE, we get the persistence from cxt->relation, which
+    * comes from the CreateStmt in progress.  For ALTER TABLE, the parser
+    * won't set cxt->relation->relpersistence, but we have cxt->rel as the
+    * existing table, so we copy the persistence from there.
+    */
+   seqpersistence = cxt->rel ? cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
+   if (loggedEl)
+   {
+       if (seqpersistence == RELPERSISTENCE_TEMP)
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+                    errmsg("cannot set logged status of a temporary sequence"),
+                    parser_errposition(cxt->pstate, loggedEl->location)));
+       else if (strcmp(loggedEl->defname, "logged") == 0)
+           seqpersistence = RELPERSISTENCE_PERMANENT;
+       else
+           seqpersistence = RELPERSISTENCE_UNLOGGED;
+   }
+
    /*
     * Build a CREATE SEQUENCE command to create the sequence object, and add
     * it to the list of things to be done before this CREATE/ALTER TABLE.
@@ -450,16 +487,7 @@ generateSerialExtraStmts(CreateStmtContext *cxt, ColumnDef *column,
    seqstmt = makeNode(CreateSeqStmt);
    seqstmt->for_identity = for_identity;
    seqstmt->sequence = makeRangeVar(snamespace, sname, -1);
-
-   /*
-    * Copy the persistence of the table.  For CREATE TABLE, we get the
-    * persistence from cxt->relation, which comes from the CreateStmt in
-    * progress.  For ALTER TABLE, the parser won't set
-    * cxt->relation->relpersistence, but we have cxt->rel as the existing
-    * table, so we copy the persistence from there.
-    */
-   seqstmt->sequence->relpersistence = cxt->rel ? cxt->rel->rd_rel->relpersistence : cxt->relation->relpersistence;
-
+   seqstmt->sequence->relpersistence = seqpersistence;
    seqstmt->options = seqoptions;
 
    /*

diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index ca86ae705fd04bf563dc4cb2a50efbf1d7822399..61d2345f384b0a3ccd9579d292719b605dc7a297 100644 (file)
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -16980,6 +16980,15 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
            appendPQExpBufferStr(query, "BY DEFAULT");
        appendPQExpBuffer(query, " AS IDENTITY (\n    SEQUENCE NAME %s\n",
                          fmtQualifiedDumpable(tbinfo));
+
+       /*
+        * Emit persistence option only if it's different from the owning
+        * table's.  This avoids using this new syntax unnecessarily.
+        */
+       if (tbinfo->relpersistence != owning_tab->relpersistence)
+           appendPQExpBuffer(query, "    %s\n",
+                             tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
+                             "UNLOGGED" : "LOGGED");
    }
    else
    {
@@ -17012,15 +17021,7 @@ dumpSequence(Archive *fout, const TableInfo *tbinfo)
                      cache, (cycled ? "\n    CYCLE" : ""));
 
    if (tbinfo->is_identity_sequence)
-   {
        appendPQExpBufferStr(query, "\n);\n");
-       if (tbinfo->relpersistence != owning_tab->relpersistence)
-           appendPQExpBuffer(query,
-                             "ALTER SEQUENCE %s SET %s;\n",
-                             fmtQualifiedDumpable(tbinfo),
-                             tbinfo->relpersistence == RELPERSISTENCE_UNLOGGED ?
-                             "UNLOGGED" : "LOGGED");
-   }
    else
        appendPQExpBufferStr(query, ";\n");
 

diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index cc7772349f20b6cf1dbabbe3a6bf4afd9129d8b0..1b74958de9388b48ba14d523d15795f0750bf6e3 100644 (file)
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -686,3 +686,19 @@ SELECT * FROM itest16;
 
 DROP TABLE itest15;
 DROP TABLE itest16;
+-- For testing of pg_dump and pg_upgrade, leave behind some identity
+-- sequences whose logged-ness doesn't match their owning table's.
+CREATE TABLE identity_dump_logged (a INT GENERATED ALWAYS AS IDENTITY);
+ALTER SEQUENCE identity_dump_logged_a_seq SET UNLOGGED;
+CREATE UNLOGGED TABLE identity_dump_unlogged (a INT GENERATED ALWAYS AS IDENTITY);
+ALTER SEQUENCE identity_dump_unlogged_a_seq SET LOGGED;
+SELECT relname, relpersistence FROM pg_class
+  WHERE relname ~ '^identity_dump_' ORDER BY 1;
+           relname            | relpersistence 
+------------------------------+----------------
+ identity_dump_logged         | p
+ identity_dump_logged_a_seq   | u
+ identity_dump_unlogged       | u
+ identity_dump_unlogged_a_seq | p
+(4 rows)
+

diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 91d2e443b4d3ecbc151456c9e423bed79b8ceb37..7537258a7547d9b8d413c42a8939c31feef0b2e0 100644 (file)
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -419,3 +419,12 @@ SELECT * FROM itest15;
 SELECT * FROM itest16;
 DROP TABLE itest15;
 DROP TABLE itest16;
+
+-- For testing of pg_dump and pg_upgrade, leave behind some identity
+-- sequences whose logged-ness doesn't match their owning table's.
+CREATE TABLE identity_dump_logged (a INT GENERATED ALWAYS AS IDENTITY);
+ALTER SEQUENCE identity_dump_logged_a_seq SET UNLOGGED;
+CREATE UNLOGGED TABLE identity_dump_unlogged (a INT GENERATED ALWAYS AS IDENTITY);
+ALTER SEQUENCE identity_dump_unlogged_a_seq SET LOGGED;
+SELECT relname, relpersistence FROM pg_class
+  WHERE relname ~ '^identity_dump_' ORDER BY 1;