Horton
Menu

Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

Justin Pryzby <pryzby@telsasoft.com>

View thread
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Soumyadeep Chakraborty <soumyadeep2007@gmail.com>, Zhihong Yu <zyu@yugabyte.com>, pgsql-hackers@postgresql.org, Ashwin Agrawal <ashwinstar@gmail.com>, vanjared@vmware.com
Date: 2023-05-31T23:35:34Z
Lists: pgsql-hackers

Attachments

On Thu, May 25, 2023 at 03:49:12PM +0900, Michael Paquier wrote:
> looking at the patch.  Here are a few comments.

...
>          * No need to add an explicit dependency for the toast table, as the
>          * main table depends on it.
>          */
> -       if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
> +       if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
> +               relkind == RELKIND_PARTITIONED_TABLE)
> 
> The comment at the top of this code block needs an update.

What do you think the comment ought to say ?  It already says:

src/backend/catalog/heap.c-              * Make a dependency link to force the relation to be deleted if its
src/backend/catalog/heap.c-              * access method is.

> Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
> update even if ALTER TABLE is defined to use the same table AM as what
> is currently set.  There is no need to update the relation's pg_class
> entry in this case.  Be careful that InvokeObjectPostAlterHook() still
> needs to be checked in this case.  Perhaps some tests should be added
> in test_oat_hooks.sql?  It would be tempted to add a new SQL file for
> that.

Are you suggesting to put this in a conditional: if oldrelam!=newAccessMethod ?

+       ((Form_pg_class) GETSTRUCT(tuple))->relam = newAccessMethod;
+       CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+       /* Update dependency on new AM */
+       changeDependencyFor(RelationRelationId, relid, AccessMethodRelationId,
+                                               oldrelam, newAccessMethod);

Why is that desirable ?  I'd prefer to see it written with fewer
conditionals, not more; then, it hits the same path every time.

This ought to address your other comments.

-- 
Justin

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