Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

From: Michael Paquier <[email-masked]>
Date: Mon, 20 Mar 2023 09:30:50 +0900

On Thu, Jun 09, 2022 at 11:21:58AM -0700, Soumyadeep Chakraborty wrote:
> Thanks. Fixed and rebased.

I think that I am OK with the concept of this patch to use a
partitioned table's relam as a reference when creating a partition
rather than relying on the default GUC, in a way similar to
tablespaces.

One worry I see is that forcing a recursion on the leaves on ALTER
TABLE could silently break partitions where multiple table AMs are
used across separate partitions if ALTER TABLE SET ACCESS METHOD is
used on one of the parents, though it seems like this is not something
I would much worry about as now the command is an error.

A second worry is that we would just break existing creation flows
that rely on the GUC defining the default AM.  This is worth a close
lookup at pg_dump to make sure that we do things correctly with this
patch in place..  Did you check dump and restore flows with partition
trees and --no-table-access-method?  Perhaps there should be
some regression tests with partitioned tables?

+    /*
+     * For partitioned tables, when no access method is specified, we
+     * default to the parent table's AM.
+     */
+    Assert(list_length(inheritOids) == 1);
+    /* XXX: should implement get_rel_relam? */
+    relid = linitial_oid(inheritOids);
+    tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+    if (!HeapTupleIsValid(tup))
+            elog(ERROR, "cache lookup failed for relation %u", relid);

Having a wrapper for that could be useful, yes.  We don't have any
code paths that would directly need that now, from what I can see,
though.  This patch gives one reason to have one.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Tue, 28 Mar 2023 09:13:10 +0900

On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:
> Did you check dump and restore flows with partition

I was looking at the patch, and as I suspected the dumps generated
are forgetting to apply the AM to the partitioned tables.  For
example, assuming that the default AM is heap, the patch creates
child_0_10 with heap2 as AM, which is what we want:
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id) USING heap2;
CREATE TABLE child_0_10 PARTITION OF parent_tab
  FOR VALUES FROM (0) TO (10);

However a dump produces that (content cut except for its most relevant
parts):
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
SET default_tablespace = '';
CREATE TABLE public.parent_tab (
    id integer
)
PARTITION BY RANGE (id);
SET default_table_access_method = heap2;
CREATE TABLE public.child_0_10 (
    id integer
);

This would restore the previous contents incorrectly, where parent_tab
would use heap and child_0_10 would use heap2, causing any partitions
created after the restore to use silently heap.  This is going to
require a logic similar to tablespaces, where generate SET commands
on default_table_access_method so as --no-table-access-method in
pg_dump and pg_restore are able to work correctly.  Having tests to
cover all that is a must-have.
--
Michael