Re: Fallback table AM for relkinds without storage

Hi,

On 2021-02-15 16:21:38 +0900, Michael Paquier wrote:
> I have been playing with this idea, and finished with the attached,
> which is not the sexiest patch around. The table AM used as fallback
> for tables without storage is called no_storage (this could be called
> virtual_am?).

> One thing to note is that this simplifies a bit slot_callbacks as
> views, foreign tables and partitioned tables can grab their slot type
> directly from this new table AM.

This doesn't seem like an advantage to me. Isn't this just pushing logic
away from a fairly obvious point into an AM that one would expect to
never actually get called?

> +static const TupleTableSlotOps *
> +no_storage_slot_callbacks(Relation relation)
> +{
> + if (relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
> + {
> + /*
> + * Historically FDWs expect to store heap tuples in slots. Continue
> + * handing them one, to make it less painful to adapt FDWs to new
> + * versions. The cost of a heap slot over a virtual slot is pretty
> + * small.
> + */
> + return &TTSOpsHeapTuple;
> + }
> +
> + /*
> + * These need to be supported, as some parts of the code (like COPY) need
> + * to create slots for such relations too. It seems better to centralize
> + * the knowledge that a heap slot is the right thing in that case here.
> + */
> + if (relation->rd_rel->relkind != RELKIND_VIEW &&
> + relation->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
> + elog(ERROR, "no_storage_slot_callbacks() failed on relation \"%s\"",
> + RelationGetRelationName(relation));
> + return &TTSOpsVirtual;
> +}

If we want to go down this path what's the justification for have
relkind awareness inside the AM? If we want it, we should give FDWs
their own tableam. And we should do the same for sequences (that'd imo
be a much nicer improvement than this change in itself).

If we were to go for separate AMs I think we could consider implementing
most of their functionality in one file, to avoid needing to duplicate
the functions that error out.

And I'd vote for not naming it no_storage - to me that sounds like a
name you'd give "blackhole_am". This concept kinda reminds me of
pseudotypes - so maybe we should just name it pseudo_am.c or such?

Greetings,

Andres Freund