Re: BUG #18674: Partitioned table doesn't depend on access method it uses

On Mon, Oct 28, 2024 at 01:23:23AM +0500, Kirill Reshke wrote:
> Seems that simply changing RELKIND_HAS_TABLE_AM marco is too invasive.
> However, I will make another try. In v2 version, regression tests
> still fail, i will adjust them, if patching this way is the right
> approach to fix the problem..
>
> script provided by Alexander:
> CREATE ACCESS METHOD ham TYPE TABLE HANDLER heap_tableam_handler;
> CREATE TABLE pt (a int) PARTITION BY LIST (a) USING ham;
> DROP ACCESS METHOD ham;

#define RELKIND_HAS_TABLE_AM(relkind) \
((relkind) == RELKIND_RELATION || \
+ (relkind) == RELKIND_PARTITIONED_TABLE || \
(relkind) == RELKIND_TOASTVALUE || \
(relkind) == RELKIND_MATVIEW)

This is an incorrect approach to the problem. RELKIND_HAS_TABLE_AM()
should be true only for relkinds that have rd_tableam set in their
relcache entries. However, you are breaking the original promise of
this macro because it is not set (and should not be set) for
partitioned tables as these have no physical presence on disk.
pg_class.relam is just a reference to use when creating partitions on
it. Note the comment in pg_class.h explaining the case of partitioned
tables.

Using an ALTER TABLE .. SET ACCESS METHOD on the partitioned table and
attempting a DROP ACCESS METHOD would fail. We are missing a normal
dependency entry in pg_depend for your sequence of commands between
the new partitioned table with the USING clause and its access method
set.

The path of adding the dependency between an AM and a table/matview is
heap_create_with_catalog() when these are defined. We can just expand
that for partitioned tables and simply fix the problem. Attached is a
patch to do that, with tests around create_am.sql based on USING
checking the contents of pg_depend.
--
Michael