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