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

Hi Melanie,

Thanks for the patches. I have a few comments.

> 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 are
> affected in addition to the name, replica identity marker, and
> cluster-on marker. See the repro at the bottom of the email.

Yes, if we can fix all these cases, that is better.

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

For these child partition cases, I am not quite sure that restoring the
properties via direct catalog updates is the behavior we want. In
particular, would that bypass the usual post-alter-hook behavior for
these changes, and if so, is that OK? Today these properties are already
being lost due to the bug, so there is no existing behavior change; but if
we fix that, I wonder whether we should preserve the normal alter-time
hook behavior as well, rather than restoring the catalog state silently
inside
the rebuild path.

Some comments on the attached patches:

v14-0001:

1/ In SetIndexStatTargets() the patch currently does

```
attup = SearchSysCacheCopy2(ATTNUM,
ObjectIdGetDatum(indexRelationId),
Int16GetDatum(st->attnum));
if (!HeapTupleIsValid(attup))
continue;
```

and GetIndexStatTargets() does the same with SearchSysCache2().

It should not be expected to have a failed cached lookup for the attribute
in either case, right?

Maybe fail with an ERROR will be better. Right?

```
elog(ERROR, "cache lookup failed for attribute %d of relation %u",
...);
```

2/

```
+ int stattarget; /* attstattarget value to restore */
```

Shouldn't statstarget be an int16 instead?

For v14-0002,

1/ I think the test coverage should go a bit further.

- sub-partitions
- Partitions with values different from the parent or from other partitions.
- ALTER TABLE ... ALTER COLUMN ... SET EXPRESSION test

2/

TransferPartitionIndexProps() currently linearly scans
stmt->oldPartIndexProps for each recursive child index build in
DefineIndex(). Maybe we should track these in a backend-local hash instead?
That would make the lookup much cheaper for partition hierarchies with
hundreds or thousands of partitions. WDYT?

3/

I think the DDL / non-DDL terminology is a bit fuzzy in a few comments.
What we really mean here is "properties not preserved by the cloned rebuild
definition", not necessarily properties that are not manageable by DDL.

For example, this comment says

```
/*
* Copy this partition's non-DDL properties ...
*/
``

but things like comments or index renames are still DDL-manageable
in general. The real distinction is that they are not reproduced
by the generateClonedIndexStmt() / CREATE INDEX-style rebuild path.

--
Sami Imseih
Amazon Web Services (AWS)