> On Aug 25, 2026, at 06:28, Melanie Plageman <[email protected]> wrote: > > On Mon, Aug 3, 2026 at 1:17 AM Chao Li <[email protected]> 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/[email protected]
> <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/
v15-0001-Preserve-index-per-column-statistics-targets-acr.patch
Description: Binary data
v15-0002-Preserve-leaf-partition-index-properties-across-.patch
Description: Binary data
