> On Aug 26, 2026, at 06:48, Sami Imseih <[email protected]> wrote:
>
> 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)
Hi Sami,
Thanks for your comments. I have addressed them in v15 expect 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/
