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

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

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

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

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

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

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

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.

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.

--
Sami Imseih
Amazon Web Services (AWS)