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

> On Aug 25, 2026, at 06:28, Melanie Plageman <melanieplageman(at)gmail(dot)com> wrote:
>
> 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.
>

I also have a documentation patch [3] that tries to clarify the inconsistent behaviors. I spent a lot of time verifying the behavior of every ALTER TABLE subcommand on partitions, and David G. Johnston also spent significant effort reviewing and editing the patch. But I then realized that the patch is too broad for committers to handle as part of this effort. Since these behaviors have existed for years without much feedback, I think it may be better to defer defining and changing their semantics until there is a separate, focused proposal.

Actually, I found the lost-RI bug while working on [2] and [3], so the initial scope of this patch was narrowed to RI only. Therefore, I agree that remembering those index properties in IndexStmt and restoring them in DefineIndex() is a better solution.

> 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
> <v14-0001-Preserve-index-per-column-statistics-targets-acr.patch><v14-0002-Preserve-leaf-partition-index-properties-across-.patch>

I reviewed v14 and found that a leaf index’s tablespace can also be lost and needs to be restored. I fixed that in v15-0002.

I also addressed Sami’s review comments in v15, except for the hash-table suggestion. I tried using an HTAB, but ran into several build failures. I didn’t want to spend more time on it, so I gave up on that approach.

Best regards.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/