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

> On Jul 29, 2026, at 04:05, Sami Imseih <samimseih(at)gmail(dot)com> wrote:
>
> Thanks for the updated patches!
>
>> PFA v10:
>>
>> 0001- preserve index name for partition indexes
>> 0002 - preserve replica identity and cluster on index
>
> Her are come comments I have

Thanks you very much or these valuable comments.

>
> v10-0001:
>
> 1/
>
> For this comment:
>
> + * If indexName isn't NULL, use it as the name of the cloned index; otherwise,
> + * let DefineIndex() choose a name. Usually the source index name cannot be
> + * used for a clone because it would conflict in the same schema. A caller
> + * rebuilding a previously existing index can provide the name that should be
> + * preserved.
>
> I think we should carry over some of the original commentary that was later
> down the line:
>
> - /*
> - * We don't try to preserve the name of the source index; instead, just
> - * let DefineIndex() choose a reasonable name. (If we tried to preserve
> - * the name, we'd get duplicate-relation-name failures unless the source
> - * table was in a different schema.)
> - */
>
> So I suggest:
>
> "
> If indexName isn't NULL, use it as the name of the cloned index; otherwise,
> let DefineIndex() choose a name. Most callers pass NULL because using the
> source index name would cause a duplicate-name failure when both indexes
> are in the same schema. Callers rebuilding a partition index (such as
> ALTER COLUMN TYPE) can pass the old name because the old index has already
> been dropped, freeing the name in the target namespace.
> "

Accepted.

>
> 2/
>
> I think we should add tests to prove that custom names are preserved for:
> Multi-level partitioning, custom named constraint indexes and SET EXPRESSION
> on generated columns
>

Added the tests.

>
> v10-0002:
>
> 1/
> - Combining the replica identity and cluster fix in the same patch is
> reasonable. The core of the change
> is the plumbing shared by both RI and cluster.
>
> 2/
> I'm on the fence about introducing lockmode in all the various
> Remeber* routines.. Why can't we just call
> find_inheritance_children() with NoLock and add a comment that the
> callers must have already acquired
> the proper lock? All current callers hold an AccessExclusiveLock by
> the time we are here.
>
> - RememberAllDependentForRebuilding(tab, AT_SetExpression, rel,
> attnum, colName);
> + RememberAllDependentForRebuilding(tab, AT_SetExpression, rel,
> attnum, colName, lockmode);
>

Actually, I was thinking about this too. Since this is an old bug and a candidate for back-patching to multiple branches, adding the lock mode to those Remember* functions may introduce unnecessary back-patching conflicts. Your comment makes me more confident about removing the lock mode from those functions.

> 3/
> We can simplify find_partition_index_properties()
>
> Instead of
>
> + partRelIds = find_inheritance_children(relId, lockmode);
>
> can we just use find_all_inheritors() and we can eliminate the need for
> recursion here?
>
> + if (partRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> + {
> + find_partition_index_properties(tab, partRelOid, partIndexId,
> + lockmode);
>

Good point. Changed to use find_all_inheritors.

> 4/
> This ERROR handling seems odd, even in the current upstream, and it's a
> bit more expensive to check now. The only way we can get here is if
> someone manually updates the catalog to set more than one replica
> identity. I am okay with keeping it, but I don't think we try very hard
> to detect catalog corruption in general, so it just seems not worth it
> to me, especially now that it's more expensive.
>
> - if (!get_index_isreplident(indoid))
> - return;
> -
> - if (tab->replicaIdentityIndex)
> - elog(ERROR, "relation %u has multiple indexes marked as
> replica identity", tab->relid);
> + if (get_index_isreplident(indoid))
> + {
> + foreach_oid(indexId, tab->replicaIdentityIndexOids)
> + {
> + if (IndexGetRelation(indexId, false) == tab->relid)
> + elog(ERROR, "relation %u has multiple indexes marked
> as replica identity", tab->relid);
> --
> +
> if (!get_index_isclustered(indoid))
> return;
>
> - if (tab->clusterOnIndex)
> - elog(ERROR, "relation %u has multiple clustered indexes", tab->relid);
> + relid = IndexGetRelation(indoid, false);
> + foreach_oid(indexId, tab->clusterOnIndexOids)
> + {
> + if (IndexGetRelation(indexId, false) == relid)
> + elog(ERROR, "relation %u has multiple clustered indexes", relid);
>

Agreed. Removed the check.

> 5/
> I think we should add tests for multi-level partitioning with CLUSTER
> and REPACK USING INDEX, using non-default index names, to prove the
> property is preserved for those cases. The current cluster test is
> single-level only.
> From this angle, both REPACK and CLUSTER do the same thing, but I still
> think separate tests make sense, since they are different commands.
>

Added the tests.

> I also found that there are several other gaps with ALTER TABLE ALTER
> COLUMN TYPE.
> See the attached. These are for a follow-up thread.
>

Sure, please go ahead with a new thread. Once I see the patch, I will review it.

PFA v11 - addressed Sami’s comments on v10.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/