Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

On Thu, Mar 07, 2024 at 08:02:00PM -0600, Justin Pryzby wrote:
> As you wrote it, you pass the "defaultAccessMethod" bool to
> ATExecSetAccessMethodNoStorage(), which seems odd.  Why not just pass
> the target amoid as newAccessMethod ?

+    /*
+     * Check that the table access method exists.
+     * Use the access method, if specified, otherwise (when not specified) 0
+     * for partitioned tables or the configured default AM.
+     */
+    if (amname != NULL)
+        amoid = get_table_am_oid(amname, false);
+    else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+        amoid = 0;
+    else
+        amoid = get_table_am_oid(default_table_access_method, false);

..  While using this flow to choose the AM oid, that's neater than the
versions I could come up with, pretty cool.

> When I fooled with this on my side, I called it "chgAccessMethod" to
> follow "chgPersistence".  I think "is default" isn't the right data
> structure.
>
> Attached a relative patch with my version.

Thanks.  I've applied the patch to add the DEFAULT clause for now, to
ease the work on this patch.

> Also: I just realized that instead of adding a bool, we could test
> (tab->rewrite & AT_REWRITE_ACCESS_METHOD) != 0

Hmm.  I've considered that, but the boolean style is able to do the
work, while being more consistent, so I'm OK with what you are doing
in your 0002.

> +-- Default and AM set in in clause are the same, relam should be set.
>
> in in?

Oops, fixed.

I have spent more time reviewing the whole and the tests (I didn't see
much value in testing the DEFAULT clause twice for the partitioned
table case and there is a test in d61a6cad6418), tweaked a few
comments and the documentation, did an indentation and a commit
message draft.

How does that look to you?  The test coverage and the semantics do
what we want them to do, so that looks rather reasonable here.  A
second or even third pair of eyes would not hurt.
--
Michael