Date: Wed, 9 Jun 2021 21:40:14 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Zhihong Yu <zyu@yugabyte.com>, Michael Paquier <michael@paquier.xyz>
Cc: Jeff Davis <pgsql@j-davis.com>, pgsql-hackers@postgresql.org,
	Jacob Champion <pchampion@vmware.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Ashwin Agrawal <aagrawal@pivotal.io>,
	Ashutosh Sharma <ashu.coek88@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Robert Haas <robertmhaas@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: alter table set TABLE ACCESS METHOD
Message-ID: <20210610024014.GA16435@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALNJ-vQV32U7QpXWMbuHHxhB1eEBaPgZtvWGVbiwubRpO-AFKg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20210610024014.GA16435%40telsasoft.com>
Precedence: bulk

On Wed, Jun 09, 2021 at 01:45:52PM -0700, Zhihong Yu wrote:
> +           /* check if another access method change was already requested
> */
> +           if (tab->newAccessMethod)
> +               ereport(ERROR,
> +                       (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                        errmsg("cannot change access method setting twice")));
> 
> I think the error message can be refined - changing  access method twice is
> supported, as long as the two changes don't overlap.

That language is consistent wtih existing errors.

src/backend/commands/tablecmds.c:                                                errmsg("cannot change persistence setting twice")));
src/backend/commands/tablecmds.c:                                                errmsg("cannot change persistence setting twice")));
                                 errmsg("cannot alter type of column \"%s\" twice",

However, I think that SET TABLESPACE is a better template to follow:
                                 errmsg("cannot have multiple SET TABLESPACE subcommands")));

Michael pointed out that there's two, redundant checks:

+       if (rel->rd_rel->relam == amoid)
+               return;
+  
+       /* Save info for Phase 3 to do the real work */
+       if (OidIsValid(tab->newAccessMethod))
+               ereport(ERROR,
+                               (errcode(ERRCODE_SYNTAX_ERROR),
+                                errmsg("cannot have multiple SET ACCESS METHOD subcommands")));

I think that the "multiple subcommands" test should be before the "no-op" test.

@Jeff: In my original patch, I also proposed patches 2,3:

Subject: [PATCH v2 2/3] Allow specifying acccess method of partitioned tables..
Subject: [PATCH v2 3/3] Implement lsyscache get_rel_relam()