Re: alter table set TABLE ACCESS METHOD

On Thu, Jul 15, 2021 at 10:49:23PM -0500, Justin Pryzby wrote:
> Also, there were two redundant checks for multiple SET ACCESS METHOD commands.
> But one of them wasn't hit if the ALTER was setting the current AM due to the
> no-op test.

Yep.

> I think it's better to fail in every case, and not just sometimes (especially
> if we were to use ERRCODE_SYNTAX_ERROR).

Looks rather fine.

- if (tab->newTableSpace)
+ if (OidIsValid(tab->newTableSpace))
This has no need to be part of this patch.

/*
- * If we have ALTER TABLE <sth> SET TABLESPACE provide a list of
- * tablespaces
+ * Complete with list of tablespaces (for SET TABLESPACE) or table AMs (for
+ * SET ACCESS METHOD).
*/
+ else if (Matches("ALTER", "TABLE", MatchAny, "SET", "ACCESS", "METHOD"))
+ COMPLETE_WITH_QUERY(Query_for_list_of_table_access_methods);
else if (Matches("ALTER", "TABLE", MatchAny, "SET", "TABLESPACE"))
COMPLETE_WITH_QUERY(Query_for_list_of_tablespaces);
Nit, there is no need to merge both block here. Let's keep them
separated.

+-- negative test
[...]
+-- negative test
Those descriptions could be better, and describe what they prevent
(aka no multiple subcommands SET ACCESS METHOD and not allowed on
partitioned tables).

> I included my 2ndary patch allowing to set the AM of partitioned table, same as
> for a tablespace.

I would suggest to not hide this topic within a thread unrelated to
it, as this is not going to ease the feedback around it. Let's start
a new thread if you feel this is necessary.

Jeff, you proposed to commit this patch upthread. Are you planning to
look at that and do so?
--
Michael