Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Justin Pryzby <pryzby(at)telsasoft(dot)com>
Cc: Peter Eisentraut <peter(at)eisentraut(dot)org>, Soumyadeep Chakraborty <soumyadeep2007(at)gmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, pgsql-hackers(at)postgresql(dot)org, Ashwin Agrawal <ashwinstar(at)gmail(dot)com>, vanjared(at)vmware(dot)com
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Date: 2024-03-08 04:32:10
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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.

Attachment Content-Type Size
v4-0001-Allow-specifying-access-method-for-partitioned-ta.patch text/x-diff 30.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Erik Wienhold 2024-03-08 04:39:34 Re: Patch: Add parse_type Function
Previous Message Erik Wienhold 2024-03-08 04:29:27 Re: CREATE TABLE creates a composite type corresponding to the table row, which is and is not there