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/

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

On 2026-08-21 Fr 12:50 PM, Álvaro Herrera wrote:

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.

OK, fair enough, I withdraw the proposal. I guess the dummy AM I set up
for testing the AM options patch Did It Wrong ;-)

Maybe this hazard is worth documenting somewhere. I'll look for a good
place.

cheers

andrew

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

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