Horton

Thread

Commits

  1. Repair pg_upgrade for identity sequences with non-default persistence.

  2. Fix propagation of persistence to sequences in ALTER TABLE / ADD COLUMN

  1. BUG #18618: pg_upgrade from 14 to 15+ fails for unlogged table with identity column

    The Post Office <noreply@postgresql.org> — 2024-09-15T22:17:33Z

    The following bug has been logged on the website:

Bug reference:      18618
Logged by:          Anthony Hsu
Email address:      erwaman@gmail.com
PostgreSQL version: 15.7
Operating system:   Linux
Description:        

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;
```

If I use pg_upgrade from 15.6, it succeeds; if I use pg_upgrade from 15.7 or
15.8, it fails with the above. Upon further testing, I found that if I
revert
https://github.com/postgres/postgres/commit/d17a3a4c6a34f61a3d4d9faa7a70c14d8d0c0ffb,
then pg_upgrade succeeds. This bug also does not happen if I try upgrading
from 15 -> 15 (same version), as the "ALTER SEQUENCE "public"."test_a_seq"
SET LOGGED;" line is not generated by pg_dump in this case.

  2. Re: BUG #18618: pg_upgrade from 14 to 15+ fails for unlogged table with identity column

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-16T17:28:30Z

    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

  3. Re: BUG #18618: pg_upgrade from 14 to 15+ fails for unlogged table with identity column

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-16T21:11:09Z

    I wrote:
> 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.

Nope, after further testing that doesn't work at all.  In the first
place, the SET UNLOGGED case falls foul of some assertions in
the relcache code having to do with WAL-skipping; and that code looks
complex and fragile enough that I don't want to try to poke holes
in it.  In the second place, I'd forgotten the need to add or remove
an init fork.

> 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).

So we have to do it like that, and it seems not that bad, especially
if we follow the lead of the SEQUENCE NAME option and don't bother
to document this stuff.  (I don't think that's a great precedent,
but I didn't change it here.)

Looking at this draft patch a second time, one way that we could
reduce the surface area for compatibility problems is to emit these
new ADD GENERATED options only in binary-upgrade mode, and continue to
use ALTER SEQUENCE SET in standard dumps.  But I kind of think that
it's not worth the complication: sequences that don't match their
table's persistence must be quite rare, else we'd have heard about
this problem long ago.

			regards, tom lane

  4. Re: BUG #18618: pg_upgrade from 14 to 15+ fails for unlogged table with identity column

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-09-17T19:56:13Z

    I wrote:
>> 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).

> So we have to do it like that, and it seems not that bad, especially
> if we follow the lead of the SEQUENCE NAME option and don't bother
> to document this stuff.  (I don't think that's a great precedent,
> but I didn't change it here.)

Pushed with a bit of further polishing, including adding that
missing documentation.

			regards, tom lane