Re: tablecmds: fix bug where index rebuild loses replica identity on partitions - Mailing list pgsql-hackers

> On Aug 28, 2026, at 01:03, Sami Imseih <samimseih.pg@gmail.com> 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'::regclassAND 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'::regclassAND 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/