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

On 2026-08-21 Fr 2:21 PM, Andres Freund wrote:
> 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?
>
>

I had convinced myself that we should not require an AM that wanted to
override just one or two things in heap to have to override this as
well. I have seen the error of my ways.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com