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