> On Aug 28, 2026, at 01:03, Sami Imseih <[email protected]> wrote: > > Hi Chao, > > All my comments are for v16-0002:
Hi Sami,
Thank you so much for the continuously review efforts.
>
> ==== 1.
>
> ```
> + if (stmt->idxconstraintcomment != NULL &&
> OidIsValid(createdConstraintId))
> + CreateComments(createdConstraintId, ConstraintRelationId, 0,
> + stmt->idxconstraintcomment);
> ```
>
> This looks a bit odd to me. I don't see other callers of
> `CreateComments()` checking `OidIsValid(...)`, and I am a bit surprised
> that `CreateComments()` itself does not check. I think hardening is a
> good idea, but a separate discussion.
>
> But for this case, I think `stmt->idxconstraintcomment != NULL`
> is all we need.
>
> We already populate `idxconstraintcomment` only after checking that
> the old descendant index has an associated constraint. So if
> `idxconstraintcomment` is non-NULL here, I would expect a valid
> `createdConstraintId` too. If not, that seems like an internal
> mismatch, not something we should silently skip over.
Yep, I was overly cautious.
>
> ```
> + if (OidIsValid(get_index_constraint(leafIndexOid)))
> + props->constraintcomment =
> +
> GetComment(get_index_constraint(leafIndexOid), ConstraintRelationId, 0);
> ```
>
> Also, instead of calling get_index_constraint(), we can probably just do this
> once and save the OID.
>
Ah, sorry, that was a result of copy-paste.
> ==== 2.
>
> By the way, these are not just "leaf" indexes, but descendants,
> so `RememberPartitionIndexProps()` should use that terminology
> throughout, including the variable names.
Good catch. I just went through all changes and replaced “leaf” with
“descendant”.
>
> ==== 3.
>
> ```
> + else if (classform->relkind != RELKIND_PARTITIONED_INDEX)
> + /* Avoid default_tablespace changing a
> storage-bearing index. */
> + props->tableSpace = pstrdup("pg_default");
> ```
>
> This seems unnecessarily complicated to me. `reset_default_tblspc` should
> just be propagated to descendant `IndexStmt`s, and then we can
> rely on the existing default-tablespace path in `DefineIndex()`.
>
I added pstrdup("pg_default”) because a regression test failed. But after a
later fix of propagating reset_default_tblspc, I wasn't aware that
props->tableSpace = pstrdup("pg_default”); became unnecessary.
> ==== 4.
>
> Zsolt's findings lead me to ask what else is missing, and I find one
> more: `DEPENDS ON EXTENSION` also needs to be handled.
>
> Here is a repro:
>
> ```
> postgres=# CREATE EXTENSION hstore;
> CREATE EXTENSION
> postgres=#
> postgres=# CREATE TABLE p (id int, a int) PARTITION BY LIST (id);
> CREATE TABLE
> postgres=# CREATE TABLE p1 PARTITION OF p FOR VALUES IN (1);
> CREATE TABLE
> postgres=#
> postgres=# CREATE INDEX p_idx ON ONLY p (a);
> CREATE INDEX
> postgres=# CREATE INDEX p1_idx ON p1 (a);
> CREATE INDEX
> postgres=# ALTER INDEX p_idx ATTACH PARTITION p1_idx;
> ALTER INDEX
> postgres=#
> postgres=# ALTER INDEX p1_idx DEPENDS ON EXTENSION hstore;
> ALTER INDEX
> postgres=#
> postgres=# SELECT d.deptype, e.extname
> postgres-# FROM pg_depend d
> postgres-# JOIN pg_extension e ON e.oid = d.refobjid
> postgres-# WHERE d.classid = 'pg_class'::regclass
> postgres-# AND d.objid = 'p1_idx'::regclass
> postgres-# AND d.refclassid = 'pg_extension'::regclass;
> deptype | extname
> ---------+---------
> x | hstore
> (1 row)
>
> postgres=#
> postgres=# ALTER TABLE p ALTER COLUMN a TYPE bigint;
> ALTER TABLE
> postgres=#
> postgres=# SELECT d.deptype, e.extname
> postgres-# FROM pg_depend d
> postgres-# JOIN pg_extension e ON e.oid = d.refobjid
> postgres-# WHERE d.classid = 'pg_class'::regclass
> postgres-# AND d.objid = 'p1_idx'::regclass
> postgres-# AND d.refclassid = 'pg_extension'::regclass;
> deptype | extname
> ---------+---------
> (0 rows)
> ```
Based on your repro, I found that if the dependency is on the parent index, it
will also be lost:
```
evantest=# CREATE TABLE p (id int, a int) PARTITION BY LIST (id);
CREATE TABLE
evantest=# CREATE TABLE p1 PARTITION OF p FOR VALUES IN (1);
CREATE TABLE
evantest=# CREATE INDEX p_idx ON ONLY p (a);
CREATE INDEX
evantest=# CREATE INDEX p1_idx ON p1 (a);
CREATE INDEX
evantest=# ALTER INDEX p_idx ATTACH PARTITION p1_idx;
ALTER INDEX
evantest=# ALTER INDEX p_idx DEPENDS ON EXTENSION hstore;
ALTER INDEX
evantest=# SELECT d.deptype, e.extname FROM pg_depend d JOIN pg_extension e ON
e.oid = d.refobjid WHERE d.classid = 'pg_class'::regclass AND d.objid =
'p_idx'::regclass AND d.refclassid = 'pg_extension'::regclass;
deptype | extname
---------+---------
x | hstore
(1 row)
evantest=# ALTER TABLE p ALTER COLUMN a TYPE bigint;
ALTER TABLE
evantest=# SELECT d.deptype, e.extname FROM pg_depend d JOIN pg_extension e ON
e.oid = d.refobjid WHERE d.classid = 'pg_class'::regclass AND d.objid =
'p_idx'::regclass AND d.refclassid = 'pg_extension'::regclass;
deptype | extname
---------+---------
(0 rows)
```
I fixed the both cases in v17.
PFA v17: addressed Sami’s comments in 0002, and 0001 is unchanged from v16.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v17-0001-Preserve-index-per-column-statistics-targets-acr.patch
Description: Binary data
v17-0002-Preserve-descendant-partition-index-properties-a.patch
Description: Binary data
