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
signature.asc
Description: PGP signature
