BUG #17756: Invalid replica indentity set order in a dump

PG Bug reporting form <noreply@postgresql.org> writes:

Some database have a partitioned table with unique index used as REPLICA
IDENTITY. pg_dump places ALTER TABLE tbl REPLICA IDENTITY USING INDEX
some_idx after partial index creation. But Postgresql fails to restore such
dump because replica identity cannot be set on invalid index some_idx:
partition indices are not created.

Please provide a concrete example, preferably a SQL script to create
a database that triggers the problem. There are enough variables
here that nobody is likely to be excited about trying to reverse-
engineer a test case from only this amount of detail.

regards, tom lane

SQL:
create database testdb;
\c testdb
create table tbl (id integer not null primary key) partition by list (id);
create table tbl_1 partition of tbl for values in (1);
alter table tbl replica identity using index tbl_pkey;

Next do:
$ pg_dump testdb >testdb.sql
$ psql testdb -c "drop table tbl"
$ psql testdb <testdb.sql

result:
...
ALTER TABLE
ERROR: cannot use invalid index "tbl_pkey" as replica identity
...

Reproduced in Postgresql 15.1 too.

Sergey Belyashov

пт, 20 янв. 2023 г. в 18:42, Tom Lane <tgl@sss.pgh.pa.us>:

Sergey Belyashov <sergey.belyashov@gmail.com> writes:

SQL:
create database testdb;
\c testdb
create table tbl (id integer not null primary key) partition by list (id);
create table tbl_1 partition of tbl for values in (1);
alter table tbl replica identity using index tbl_pkey;

Next do:
$ pg_dump testdb >testdb.sql
$ psql testdb -c "drop table tbl"
$ psql testdb <testdb.sql

result:
...
ALTER TABLE
ERROR: cannot use invalid index "tbl_pkey" as replica identity

Thanks for the test case. So the problem occurs because pg_dump dumps
the commands in this order:

ALTER TABLE ONLY public.tbl
ADD CONSTRAINT tbl_pkey PRIMARY KEY (id);

ALTER TABLE ONLY public.tbl REPLICA IDENTITY USING INDEX tbl_pkey;

ALTER TABLE ONLY public.tbl_1
ADD CONSTRAINT tbl_1_pkey PRIMARY KEY (id);

ALTER INDEX public.tbl_pkey ATTACH PARTITION public.tbl_1_pkey;

but the backend won't take the ALTER REPLICA IDENTITY command
until after the ATTACH PARTITION.

We could probably make pg_dump emit things in the order that works,
but it'd be a significant amount of extra complication there
(the ALTER REPLICA IDENTITY command couldn't be treated as just
part of the index definition).

I wonder why it is that the backend rejects this sequence.
I see that you can do this:

regression=# create table tbl (id integer not null primary key) partition by list (id);
CREATE TABLE
regression=# alter table tbl replica identity using index tbl_pkey;
ALTER TABLE

and it doesn't seem like the partitioned index is notably more
valid in this state than in the one that pg_dump has created.
So I think it might be better to fix the backend to allow this
sequence of operations.

regards, tom lane

I wrote:

I wonder why it is that the backend rejects this sequence.
I see that you can do this:
regression=# create table tbl (id integer not null primary key) partition by list (id);
CREATE TABLE
regression=# alter table tbl replica identity using index tbl_pkey;
ALTER TABLE
and it doesn't seem like the partitioned index is notably more
valid in this state than in the one that pg_dump has created.
So I think it might be better to fix the backend to allow this
sequence of operations.

I looked into this, and I think we basically could just drop the
restriction in ATExecReplicaIdentity that rejects marking an invalid
index as replica identity. If it is invalid, then the relcache won't
treat it as being the live replica identity (cf RelationGetIndexList),
so nothing will happen until it becomes valid; but there's no reason
why we can't mark it as identity in advance of that. We support
not-yet-valid primary keys, so why not replica identity?

In checking this, I found a comment in index.c claiming

* ALTER TABLE assumes that indisreplident cannot be set for
* invalid indexes.

which was added by Michael's 9511fb37a. The back story for that
indicates that Michael was worried about multiple indexes of a
relation becoming marked with indisreplident. However, as far
as I can see the only actual issue there is that
relation_mark_replica_identity is too trusting, and that looks
like either sloppy coding or premature optimization. I think
we could just rip out its early-exit path and fix it to positively
guarantee that no more than one index is marked indisreplident.

In short, I propose the attached. This'd need to be back-patched,
which 9511fb37a wasn't, so maybe we should also back-patch 9511fb37a?
I don't think it's really necessary though.

regards, tom lane