Re: Some RELKIND macro refactoring

On Mon, Nov 01, 2021 at 07:28:16AM +0100, Peter Eisentraut wrote:
>
> Small rebase of this patch set.

Regarding 0001, I find the existing code a bit more self-documenting
if we keep those checks flagInhAttrs() and guessConstraintInheritance().
So I would rather leave these.

I like 0002, which makes things a bit easier to go through across
various places where relkind-only checks are replaced by those new
macros. There is one thing I bumped into, though..

> if (create_storage)
> {
> - switch (rel->rd_rel->relkind)
> - {
> - case RELKIND_VIEW:
> - case RELKIND_COMPOSITE_TYPE:
> - case RELKIND_FOREIGN_TABLE:
> - case RELKIND_PARTITIONED_TABLE:
> - case RELKIND_PARTITIONED_INDEX:
> - Assert(false);
> - break;
> - [ .. deletions .. ]
> - }
> + if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind))
> + table_relation_set_new_filenode(rel, &rel->rd_node,
> + relpersistence,
> + relfrozenxid, relminmxid);
> + else
> + RelationCreateStorage(rel->rd_node, relpersistence);
> }

I think that you should keep this block as it is now. The part where
a relkind does not support table AMs and does not require storage
would get uncovered, and this new code would just attempt to create
storage, so it seems to me that the existing code is better when it
comes to prevent mistakes from developers? You could as well use an
else if (RELKIND_INDEX || RELKIND_SEQUENCE) for
RelationCreateStorage(), and fall back to Assert(false) in the else
branch, to get the same result as the original without impacting the
level of safety of the original code.
--
Michael