Re: Allow table AMs to define their own reloptions

Hi,

Both of Zsolt's reports are real bugs, fixed in v5. Replying to both
threads together.

> [Zsolt] dummy_table_am seems to accept but ignore fillfactor
> options [...]
> heap_ff10 | 9091 | 71 MB
> dummy_ff10 | 885 | 7080 kB

Confirmed, and not just fillfactor. Four macros/call sites read
StdRdOptions fields straight out of rd_options behind a check,
RelationHasStdRdOptions(), that only asked "does this AM have a
custom amoptions at all" rather than "is this field actually there"
-- blocking exactly the case add_reloption_to_kind() exists for.
Same problem for toast_tuple_target, parallel_workers,
vacuum_index_cleanup, vacuum_truncate,
vacuum_max_eager_freeze_failure_rate, and autovacuum_enabled.

Fixed with TableAmRoutine.has_std_options_prefix: an AM sets it when
its amoptions struct embeds a full StdRdOptions as its first member,
and RelationHasStdRdOptions() trusts that. dummy_table_am now embeds
StdRdOptions and registers all seven other fields too, not just
fillfactor, so each gets its real default instead of zero. Re-ran
your repro:

    dummy_ff10 | 9091 | 71 MB
    heap_ff10  | 9091 | 71 MB

Matches now. Added a regression case that fails on v4, passes on v5.

> [Zsolt] doesn't seem to support text columns [...]
> ERROR: only heap AM is supported
> [Junwang] I think it's ok, it's just a test module

Also a real bug, not a limitation: heap's own relation_toast_am
callback, heapam_relation_toast_am(), returns rel->rd_rel->relam,
which is dummy_table_am's own oid rather than heap's since it
inherited that callback unchanged along with the rest of heap's
routine. That made its TOAST table a dummy_table_am relation too,
and building its index failed in heap_getnext(), which requires a
real heap relation. Fixed by having dummy_table_am override
relation_toast_am directly. Text columns work now.

> [Junwang] extract_autovac_opts... unconditionally interprets the
> result as StdRdOptions, is that intentional?

Yes, that call passes amoptions = NULL
explicitly, which forces heap_reloptions() regardless of the
relation's actual access method, so the result really is
StdRdOptions.

I think the behaviour is right, though: it means autovacuum's own
scheduling only ever sees the standard autovacuum_* names, never an
AM's own amoptions. Making it AM-aware would cost a catalog lookup
on every relation in autovacuum's periodic scan, for a capability
nothing needs. Documented that constraint in tableam.sgml rather
than leave it to be rediscovered.

cheers

andrew

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