Re: tablecmds: fix bug where index rebuild loses replica identity on partitions

On Mon, Aug 3, 2026 at 1:17 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
> PFA v13: addressed Alberto’s comment on 0001; 0002 is the same as v12.

I started taking a look at this and found several other properties
that are dropped (or rather, not saved and restored) for leaf
partitions after an ALTER COLUMN TYPE (or ALTER COLUMN SET EXPRESSION)
rebuild. Comments, stats targets, and reloptions in addition to the
name, replica identity marker, and cluster-on marker (see repro at
bottom of email).

This made me think we should save all of these in a data structure on
the IndexStmt and then update the catalog tables after creating the
new index instead of doing the deferred sub-command execution (as your
v13-0002). I've attached a patch that implements my idea. It edits the
same locations as the custom name save-and-restore approach in
v13-0001 but does it for all the missing properties.

There is precedent for doing this -- index_concurrently_swap() does it
this way for indisreplident/indisclustered already.

It is possible to use the deferred sub-command method for RI, cluster,
and comment, but it won't work for name (bc name doesn't have an alter
sub-command for renaming an index) nor for stat-target and reloptions
(bc those lookup the index via oid and we need the old index oid which
is no longer around by the time we are executing a deferred
subcommand).

I don't love that the root partition properties are remembered as part
of ATExecAlterColumnType() and the child partitions as part of
ATPostAlterTypeParse(), but there didn't seem to be a good way of
moving either one. The IndexStmt doesn't exist yet in
ATExecAlterColumnType(). This is common to all the patches (v13 and my
attached patches).

The first patch is to preserve stats targets in general -- it wasn't
lost just for partition child indexes but also for regular indexes.
This is basically the same as a patch Zsolt proposed in [1]. Its test
might be able to be minimized or incorporated into an existing test,
but I haven't tried to do that yet.

The second patch handles preserving the properties for child
partitions. It also includes an idea for a regression test that
compares the catalog table rows before and after ALTER COLUMN TYPE to
make sure we are restoring everything we expect to be the same. I had
an LLM write it and it suggested using jsonb and a sql function so we
could subtract the columns we expect to change but select everything
else. The idea is to avoid regressions. If someone adds a new
property, they'll have to explicitly allow not transferring it after
ALTER COLUMN TYPE. The test is a little hard to read, so maybe there's
a way to simplify it. I'm not sure.

I'm not convinced this needs to be backpatched. I don't see anything
in the docs saying that after an ALTER COLUMN TYPE these various
properties would be preserved (and definitely nothing about them being
preserved on partition leaves). So, users most likely would have
scripts doing the follow-up alter tables themselves. And, for the
replica identity, you'll have to do the schema change on the replica
for it to work anyway, so it is already a multi-step process. I can be
convinced otherwise if, for example, not preserving these properties
poses a security risk like the one fixed in 6713a6e04cb.

It seems like there are some inaccuracies in the docs around what
operations recurse to leaves and which don't and potentially some
inconsistencies or even bugs in the behavior itself.
ALTER COLUMN ... SET (attribute_option) doesn't recurse to partitions
while SET STATISTICS does.
SET COMPRESSION doesn't recurse to partitions while SET STORAGE does.
ALTER TABLE parent RENAME CONSTRAINT on a UNIQUE/PK doesn't rename the
leaf's backing index (that one is debatable).
In the docs it says
"The actions for identity columns (ADD GENERATED, SET etc., DROP
IDENTITY), as well as the actions CLUSTER, OWNER, and TABLESPACE never
recurse to descendant tables; that is, they always act as though ONLY
were specified."
but identity does recurse to descendant tables.

And then there is Zsolt's other patch in [1] which keeps extended
statistics from losing their stats targets.

Anyway, I started to feel a bit defeated so I stopped looking.
Frankly, all of this made me wonder if we even know what behavior we
want in all these cases, and maybe I should just leave it the way it
is. I see you have started a thread where you try to define the
behavior [2] and mention that people like Robert Haas have been saying
for a long time that we should define consistent semantics for it all.
I don't think I'm up for trying to fix everything, but I do want to
make sure that I won't be making things worse by committing this
series of patches.

Repro for lost comment, stats target, and index reloption:

CREATE TABLE t (id int, val int) PARTITION BY RANGE (id);
CREATE TABLE t1 PARTITION OF t FOR VALUES FROM (0) TO (100);
CREATE INDEX t_expr ON t ((val + 1));
COMMENT ON INDEX t1_val_1_idx IS 'important note';
ALTER INDEX t1_val_1_idx ALTER COLUMN 1 SET STATISTICS 321;
ALTER INDEX t1_val_1_idx SET (fillfactor = 42);
SELECT obj_description('t1_val_1_idx'::regclass) AS comment,
(SELECT attstattarget FROM pg_attribute
WHERE attrelid = 't1_val_1_idx'::regclass AND attnum = 1) AS
stat_target,
(SELECT reloptions FROM pg_class WHERE relname =
't1_val_1_idx') AS reloptions;
ALTER TABLE t ALTER COLUMN val TYPE int;
SELECT obj_description('t1_val_1_idx'::regclass) AS comment,
(SELECT attstattarget FROM pg_attribute
WHERE attrelid = 't1_val_1_idx'::regclass AND attnum = 1) AS
stat_target,
(SELECT reloptions FROM pg_class WHERE relname =
't1_val_1_idx') AS reloptions;

- Melanie

[1] https://www.postgresql.org/message-id/flat/CAN4CZFNZwcCgi-igaD%3DLH1ubxMBqJJS%2Bp4ZnOKKdCi9duaMu_w%40mail.gmail.com
[2] https://www.postgresql.org/message-id/59FB38EF-FA62-41B7-A082-DDA251B04F9E@gmail.com