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

Hi Chao,

All my comments are for v16-0002:

==== 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.

```
+ 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.

==== 2.

By the way, these are not just "leaf" indexes, but descendants,
so `RememberPartitionIndexProps()` should use that terminology
throughout, including the variable names.

==== 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()`.

```
if (stmt->reset_default_tblspc)
(void) set_config_option("default_tablespace", "",
PGC_USERSET, PGC_S_SESSION,
GUC_ACTION_SAVE, true, 0, false);

...

if (stmt->tableSpace)
tablespaceId = get_tablespace_oid(stmt->tableSpace, false);
else
tablespaceId =
GetDefaultTablespace(rel->rd_rel->relpersistence, partitioned);
```

==== 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)
```

--
Sami Imseih
Amazon Web Services (AWS)