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
