Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

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