Re: BUG #18618: pg_upgrade from 14 to 15+ fails for unlogged table with identity column - Mailing list pgsql-bugs

PG Bug reporting form <noreply@postgresql.org> writes:
> If I create an unlogged table with an identity column as follows

> CREATE UNLOGGED TABLE test (a INT GENERATED BY DEFAULT AS IDENTITY);

> in PG14 and then run pg_upgrade to PG15+, it fails due to

> ```
> pg_restore: error: could not execute query: ERROR:  unexpected request for
> new relfilenode in binary upgrade mode
> Command was:
> -- For binary upgrade, must preserve pg_class oids and relfilenodes
> SELECT
> pg_catalog.binary_upgrade_set_next_heap_pg_class_oid('16384'::pg_catalog.oid);
> SELECT
> pg_catalog.binary_upgrade_set_next_heap_relfilenode('16384'::pg_catalog.oid);

> ALTER TABLE "public"."test" ALTER COLUMN "a" ADD GENERATED BY DEFAULT AS
> IDENTITY (
>     SEQUENCE NAME "public"."test_a_seq"
>     START WITH 1
>     INCREMENT BY 1
>     NO MINVALUE
>     NO MAXVALUE
>     CACHE 1
> );
> ALTER SEQUENCE "public"."test_a_seq" SET LOGGED;
> ```

Yeah.  pg_dump is trying to recreate the state of affairs in the
source database, where the table is unlogged but its owned sequence
is logged (because v14 didn't have unlogged sequences).  And
ALTER SEQUENCE SET LOGGED works by assigning the sequence a new
relfilenode and then writing its data into that.  That doesn't work
in binary-upgrade mode, and if it did work it'd be the wrong thing
because we have to preserve the sequence's relfilenode.

However, in binary-upgrade mode there seems little need to be
concerned about whether we produce a nice WAL trace of the SET LOGGED
operation.  I think we can just summarily change the sequence's
relpersistence field and be done with it, more or less as attached.
Anybody see a flaw in that reasoning?

The only alternative I can see is to extend the ALTER TABLE ADD
GENERATED syntax to allow correct specification of the sequence's
LOGGED/UNLOGGED status to begin with.  While cleaner, that would be
a lot more work and probably result in compatibility problems for
normal uses of pg_dump (where the output might get loaded into a
server version that lacks the syntax extension).

            regards, tom lane

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index b37fd688d3..73ab609153 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -545,6 +545,46 @@ SequenceChangePersistence(Oid relid, char newrelpersistence)
     Buffer        buf;
     HeapTupleData seqdatatuple;

+    /*
+     * In binary-upgrade mode, we cannot assign a new relfilenumber to the
+     * sequence, so the normal code path below doesn't work.  However, we
+     * don't really need to go through all the pushups of assigning a new
+     * relfilenumber in this mode, since there's no concern for concurrent
+     * operations, rollback-ability, or replication.  It seems sufficient to
+     * just summarily change the sequence's relpersistence value.
+     */
+    if (IsBinaryUpgrade)
+    {
+        Relation    pg_class;
+        HeapTuple    tuple;
+        Form_pg_class classform;
+
+        /* Get a writable copy of the pg_class tuple for the sequence. */
+        pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+        tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(relid));
+        if (!HeapTupleIsValid(tuple))
+            elog(ERROR, "could not find tuple for relation %u", relid);
+        classform = (Form_pg_class) GETSTRUCT(tuple);
+
+        /* Apply the update. */
+        classform->relpersistence = newrelpersistence;
+
+        CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+        heap_freetuple(tuple);
+
+        table_close(pg_class, RowExclusiveLock);
+
+        /*
+         * Make the pg_class row change visible.  This will cause the
+         * sequence's relcache entry to get updated, too.
+         */
+        CommandCounterIncrement();
+
+        return;
+    }
+
     /*
      * ALTER SEQUENCE acquires this lock earlier.  If we're processing an
      * owned sequence for ALTER TABLE, lock now.  Without the lock, we'd