Hi,
On 2026-08-21 09:20:07 -0400, 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 following would that would be a good idea? Right now we can have a
separately registered table AM to write tests for non-default AMs without a
separately maintained AM. But with this that won't test the toast paths
anymore, because you make those to be a plain heapam, rather than the
simulacrum of a separate AM?
I certainly don't see how that would be a sane thing to backpatch. It's quite
possible that would break currently working code, no?
Greetings,
Andres Freund
