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