Re: Some RELKIND macro refactoring

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(...);

> +/*
> + * 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. 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\"",

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/