Date: Sun, 16 Jul 2023 21:37:28 -0500
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
Subject: Re: ALTER TABLE SET ACCESS METHOD on partitioned tables
Message-ID: <ZLSpaFcIFYvfhQpr@telsasoft.com>
References: <ZCIxFkgOGv64nYE7@paquier.xyz>
 <ZCJuW3GOfE/0eBjh@telsasoft.com>
 <ZCUZLiCeXq4Im7OG@telsasoft.com>
 <ZEccbkBYCMy42Rxt@telsasoft.com>
 <ZG8E6DggGkPyEPPx@paquier.xyz>
 <ZHfZxjcR0J9Ws7ze@telsasoft.com>
 <ZHiUKhhT71X4CjJl@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZHiUKhhT71X4CjJl@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZLSpaFcIFYvfhQpr%40telsasoft.com>
Precedence: bulk

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