Re: Some RELKIND macro refactoring - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: Some RELKIND macro refactoring
Date
Msg-id YZdS3b2lrrJL6PuD@paquier.xyz
Whole thread Raw
In response to Re: Some RELKIND macro refactoring  (Peter Eisentraut <peter.eisentraut@enterprisedb.com>)
Responses Re: Some RELKIND macro refactoring
Re: Some RELKIND macro refactoring
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Ken Kato
Date:
Subject: Re: CREATE tab completion
Next
From: Richard Guo
Date:
Subject: A spot of redundant initialization of brin memtuple