Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

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