On 2020-09-09 15:22, Michael Paquier wrote:
> On Tue, Sep 08, 2020 at 07:17:58PM -0500, Justin Pryzby wrote:
>> Initially I added List *params, and Michael suggested to retire
>> ReindexStmt->concurrent. I provided a patch to do so, initially by
>> leaving int
>> options and then, after this, removing it to "complete the thought",
>> and get
>> rid of the remnants of the "old way" of doing it. This is also how
>> vacuum and
>> explain are done.
>> https://www.postgresql.org/message-id/20200902022410.GA20149%40telsasoft.com
>
> Defining a set of DefElem when parsing and then using the int
> "options" with bitmasks where necessary at the beginning of the
> execution looks like a good balance to me. This way, you can extend
> the grammar to use things like (verbose = true), etc.
>
> By the way, skimming through the patch set, I was wondering if we
> could do the refactoring of patch 0005 as a first step
>
Yes, I did it with intention to put as a first patch, but wanted to get
some feedback. It's easier to refactor the last patch without rebasing
others.
>
> until I
> noticed this part:
> +common_option_name:
> NonReservedWord { $$ = $1; }
> | analyze_keyword { $$ = "analyze"; }
> This is not a good idea as you make ANALYZE an option available for
> all the commands involved in the refactoring. A portion of that could
> be considered though, like the use of common_option_arg.
>
From the grammar perspective ANY option is available for any command
that uses parenthesized option list. All the checks and validations are
performed at the corresponding command code.
This analyze_keyword is actually doing only an ANALYZE word
normalization if it's used as an option. Why it could be harmful?
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
