Re: heapam_relation_toast_am() returns the wrong AM for a wrapped heap AM

On 2026-Aug-21, Andrew Dunstan wrote:

> heapam_relation_toast_am() returns rel->rd_rel->relam instead of the
> literal heap AM oid, on the assumption the two are always equal since
> it's only meant to run for relations that are themselves heap. That
> breaks for a table AM that copies heap's whole TableAmRoutine (via
> GetHeapamTableAmRoutine()) and overrides only a few callbacks -- a
> pattern heap_getnext()'s own identity check explicitly anticipates,
> per its comment about allowing "regression tests that create another
> AM reusing the heap handler." For such an AM, rel->rd_rel->relam is
> its own oid, so NewRelationCreateToastTable() creates the TOAST table
> with that AM too, and building its chunk_id/chunk_seq index then
> fails in heap_getnext(), which requires rd_tableam to be literally
> GetHeapamTableAmRoutine(): "only heap AM is supported" for any such
> AM as soon as a table needs a TOAST table.
>
> Fix by returning the literal HEAP_TABLE_AM_OID, which is what the
> function's own comment already claims it does ("TOAST tables for
> heap relations are just heap relations").

I'm not sure that this patch is correct. The heapam_relation_toast_am
function is the implementation for the relation_toast_am callback in the
heapam table AM. So this function is specific to heapam, and other
table AMs should have their own if they want to have different behavior.

The function is really small, so if a table AM that's not heapam can
very easily set up its own callback function that returns
HEAP_TABLE_AM_OID to get a regular heapam TOAST table, right?

So "a table AM that [copies the whole heap TableAmRoutine and overrides
only a handful of callbacks]" is Doing It Wrong by failing to also
override relation_toast_am.

(Whether it makes sense to return rd_rel->relam in a heapam-specific
function is something worth discussing, perhaps, but I don't think what
you propose is the right fix here. I think it may make more sense to
have a pg_class column that says what table AM the toast table uses.)

In any case, this comment:

> @@ -2048,12 +2049,22 @@ heapam_relation_needs_toast_table(Relation rel)
> }
>
> /*
> - * TOAST tables for heap relations are just heap relations.
> + * TOAST tables are always plain heap relations, regardless of the AM of
> + * the table they belong to. Return the literal heap AM oid rather than
> + * rel->rd_rel->relam: those are only the same value when rel is itself a
> + * genuine heap relation. A table AM that reuses this callback (e.g. by
> + * copying the whole heap TableAmRoutine and overriding only a handful of
> + * callbacks) is not itself heap, so returning rel->rd_rel->relam would
> + * create its TOAST table using that AM instead -- and storage-layer code
> + * that still calls heap_getnext() directly (see its comment) rejects any
> + * relation whose rd_tableam is not literally GetHeapamTableAmRoutine(),
> + * which fails as soon as anything scans that TOAST table, e.g. to build
> + * its chunk_id/chunk_seq index.
> */

would really need to be pared down to something a human would write.

Your commit message mentions review of a patch by Zsolt, but doesn't
provide a Discussion link to that discussion. I think it should.

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