Re: Some RELKIND macro refactoring

On 16.08.21 16:22, Alvaro Herrera wrote:
> On 2021-Aug-16, Peter Eisentraut wrote:
>
>> + if (rel->rd_rel->relkind == RELKIND_INDEX ||
>> + rel->rd_rel->relkind == RELKIND_SEQUENCE)
>> + RelationCreateStorage(rel->rd_node, relpersistence);
>> + else if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind))
>> + table_relation_set_new_filenode(rel, &rel->rd_node,
>> + relpersistence,
>> + relfrozenxid, relminmxid);
>> + else
>> + Assert(false);
>
> I think you could turn this one (and the one in RelationSetNewRelfilenode) around:
>
> if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind))
> table_relation_set_new_filenode(...);
> else if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
> RelationCreateStorage(...);

done

>> +/*
>> + * Relation kinds with a table access method (rd_tableam). Although sequences
>> + * use the heap table AM, they are enough of a special case in most uses that
>> + * they are not included here.
>> + */
>> +#define RELKIND_HAS_TABLE_AM(relkind) \
>> + ((relkind) == RELKIND_RELATION || \
>> + (relkind) == RELKIND_TOASTVALUE || \
>> + (relkind) == RELKIND_MATVIEW)
>
> Partitioned tables are not listed here, but IIRC there's a patch to let
> partitioned tables have a table AM so that their partitions inherit it.
> I'm wondering if that patch is going to have to change this spot; and if
> it does, how will that affect the callers of this macro that this patch
> creates.

Interesting. It would be useful to consider both patches together then,
so that conceptual clarity is achieved. I will take a look.

After some consideration, I have removed RELKIND_HAS_INDEX_AM(), since
the way I had it and also considering that other patch effort, it didn't
make much sense.

> I think a few places assume that HAS_TABLE_AM means that the
> table has storage, but perhaps it would be better to spell that out
> explicitly:
>
> @@ -303,9 +303,7 @@ verify_heapam(PG_FUNCTION_ARGS)
> /*
> * Check that a relation's relkind and access method are both supported.
> */
> - if (ctx.rel->rd_rel->relkind != RELKIND_RELATION &&
> - ctx.rel->rd_rel->relkind != RELKIND_MATVIEW &&
> - ctx.rel->rd_rel->relkind != RELKIND_TOASTVALUE)
> + if (!(RELKIND_HAS_TABLE_AM(ctx.rel->rd_rel->relkind) && RELKIND_HAS_STOAGE(ctx.rel->rd_rel->relkind)))
> ereport(ERROR,
> (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> errmsg("cannot check relation \"%s\"",
>

I think, if something has a table AM, it implies that it has storage.

In the contrib modules, the right way to phrase the check is sometimes
not entirely clear, since part of the job of these modules is sometimes
to bypass the normal APIs and do extra checks etc.