Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:
> >> 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 ?
>
> Yes, that's what I would add with a few lines close to the beginning
> of ATExecSetTableSpaceNoStorage() to save from catalog updates if
> these are not needed.

I understand that it's possible for it to be conditional, but I don't
undertand why it's desirable/important ?

It still seems preferable to be unconditional.

On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:
>> Why is that desirable ? I'd prefer to see it written with fewer
>> conditionals, not more; then, it hits the same path every time.

It's not conditional in the tablespace code that this follows, nor
others like ATExecSetStatistics(), ATExecForceNoForceRowSecurity(),
ATExecSetCompression(), etc, some of which are recently-added. If it's
important to do here, isn't it also important to do everywhere else ?

--
Justin