Horton
Menu

Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Peter Eisentraut <peter@eisentraut.org>, Soumyadeep Chakraborty <soumyadeep2007@gmail.com>, Zhihong Yu <zyu@yugabyte.com>, pgsql-hackers@postgresql.org, Ashwin Agrawal <ashwinstar@gmail.com>, vanjared@vmware.com
Date: 2024-03-08T04:32:10Z
Lists: pgsql-hackers

Attachments

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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add information about access method for partitioned relations in \dP+

  2. Fix dumps of partitioned tables with table AMs

  3. Don't use the pg_am system catalog in new test

  4. ALTER TABLE: rework determination of access method ID

  5. Allow specifying an access method for partitioned tables

  6. Review wording on tablespaces w.r.t. partitioned tables

  7. Add support for DEFAULT in ALTER TABLE .. SET ACCESS METHOD

  8. Update comment of AlterTableCmd->name in parsenodes.h