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

Hi!

On Sat, 26 Oct 2024 at 19:27, PG Bug reporting form
<noreply@postgresql.org> wrote:

The following bug has been logged on the website:

Bug reference: 18674
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 17.0
Operating system: Ubuntu 22.04
Description:

The following script:
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;

ends successfully, with the access method dropped, though then:
CREATE TABLE tp1 PARTITION OF pt FOR VALUES IN (1);

fails with
ERROR: cache lookup failed for access method 16385

Whilst
CREATE TABLE t (a int) USING ham;
DROP ACCESS METHOD ham;

fails with
ERROR: cannot drop access method ham because other objects depend on it
DETAIL: table t depends on access method ham
HINT: Use DROP ... CASCADE to drop the dependent objects too.

Reproduced starting from 374c7a229.

Nice catch!

Did you dig into reasons? I did a little...

After looking closely at the DefineRelation workflow, I discovered
that it is `RelationBuildLocalRelation` responsibility to add
dependency between relation and its TAM. The bug arises from the fact
that RELKIND_HAS_TABLE_AM doesn't respect partitioned relations.
So, a trivial fix is attached.

Does it look good for you?

--
Best regards,
Kirill Reshke

On Sun, 27 Oct 2024 at 12:53, Kirill Reshke <reshkekirill@gmail.com> wrote:

Hi!

On Sat, 26 Oct 2024 at 19:27, PG Bug reporting form
<noreply@postgresql.org> wrote:

The following bug has been logged on the website:

Bug reference: 18674
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 17.0
Operating system: Ubuntu 22.04
Description:

The following script:
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;

ends successfully, with the access method dropped, though then:
CREATE TABLE tp1 PARTITION OF pt FOR VALUES IN (1);

fails with
ERROR: cache lookup failed for access method 16385

Whilst
CREATE TABLE t (a int) USING ham;
DROP ACCESS METHOD ham;

fails with
ERROR: cannot drop access method ham because other objects depend on it
DETAIL: table t depends on access method ham
HINT: Use DROP ... CASCADE to drop the dependent objects too.

Reproduced starting from 374c7a229.

Nice catch!

Did you dig into reasons? I did a little...

After looking closely at the DefineRelation workflow, I discovered
that it is `RelationBuildLocalRelation` responsibility to add
dependency between relation and its TAM. The bug arises from the fact
that RELKIND_HAS_TABLE_AM doesn't respect partitioned relations.
So, a trivial fix is attached.

Does it look good for you?

--
Best regards,
Kirill Reshke

As mentioned by Alexander off-list, v1 patch actually fails in a lot
of regression tests... My bad.

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;

now gives the correct output.

--
Best regards,
Kirill Reshke

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

Hello Michael and Kirill,

28.10.2024 10:23, Michael Paquier wrote:

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.

0001-Fix-dependency....patch works for me, but while testing, I've also
observed another questionable behavior:
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
SET default_table_access_method = heap2;
CREATE TABLE pt (a int) PARTITION BY LIST (a);
\d+ pt

doesn't show heap2 as access method for the table.

Shouldn't default_table_access_method affect partitioned tables?

Best regards.
Alexander

On Mon, Oct 28, 2024 at 4:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:

Hello Michael and Kirill,

28.10.2024 10:23, Michael Paquier wrote:

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.

0001-Fix-dependency....patch works for me, but while testing, I've also
observed another questionable behavior:
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
SET default_table_access_method = heap2;
CREATE TABLE pt (a int) PARTITION BY LIST (a);
\d+ pt

doesn't show heap2 as access method for the table.

Shouldn't default_table_access_method affect partitioned tables?

I think `default_table_access_method` should not affect partitioned tables,
the partition's access_method will be default_table_access_method as
wanted.

But if it affects partitioned tables, then the partitions will always
inherit the access method of the partitioned table.

You can try the following if you want it to affect partitioned tables:

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1ccc80087c..434e82ab14 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -973,7 +973,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
                        accessMethodId =
get_rel_relam(linitial_oid(inheritOids));
                }

-               if (RELKIND_HAS_TABLE_AM(relkind) &&
!OidIsValid(accessMethodId))
+               if (!OidIsValid(accessMethodId))
                        accessMethodId =
get_table_am_oid(default_table_access_method, false);
        }

Best regards.
Alexander

--
Regards
Junwang Zhao

On Mon, Oct 28, 2024 at 07:49:54PM +0800, Junwang Zhao wrote:

I think `default_table_access_method` should not affect partitioned tables,
the partition's access_method will be default_table_access_method as
wanted.

But if it affects partitioned tables, then the partitions will always
inherit the access method of the partitioned table.

Yes. default_table_access_method not affecting partitioned tables is
something that has been discussed on the thread that dealt with this
feature.

The point is for pg_class.relam == 0, which is a backward-compatible
way of telling that the partitions attached to the partitioned table
should use as AM what default_table_access_method is set to when a
partition is created.

relam for a partitioned table would be set only if there is a USING
clause in the CREATE TABLE or for ALTER TABLE SET ACCESS METHOD (the new
DEFAULT keyword can be used to set relam back to 0). There is a
pretty good chunk of SQL in create_am.sql for all these interactions
between the GUC, multiple levels of partitionining and partitions
attached. See around "Partition hierarchies with access methods".
--
Michael

On Tue, Oct 29, 2024 at 07:48:30AM +0900, Michael Paquier wrote:

Yes. default_table_access_method not affecting partitioned tables is
something that has been discussed on the thread that dealt with this
feature.

Anyway, I've fixed this issue with 49a23441ca7f and backpatched it
down to 17. Thanks for the report, Alexander.
--
Michael