Re: Some RELKIND macro refactoring

On 19.11.21 08:31, Michael Paquier wrote:
> 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.

In that case, the existing check in guessConstraintInheritance() seems 
wrong, because it doesn't check for RELKIND_MATVIEW.  Should we fix 
that?  It's dead code either way, but if the code isn't exercises, then 
these kinds of inconsistency come about.

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

Maybe

     else
     {
         Assert(RELKIND_HAS_STORAGE(rel->rd_rel->relkind);
         RelationCreateStorage(rel->rd_node, relpersistence);
     }

create_storage is set earlier based on RELKIND_HAS_STORAGE(), so this 
would be consistent.