Re: alter table set TABLE ACCESS METHOD

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()