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.