MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 09 Sep 2020 16:03:45 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Alvaro Herrera
 <alvherre@2ndquadrant.com>, Masahiko Sawada
 <masahiko.sawada@2ndquadrant.com>, Steve Singer <steve@ssinger.info>,
 pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>, Masahiko Sawada
 <sawada.mshk@gmail.com>, Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
In-Reply-To: <20200909122200.GA2743@paquier.xyz>
References: <20200908233951.GC18552@telsasoft.com>
 <20200909000238.GA1563@alvherre.pgsql>
 <20200909001757.GD18552@telsasoft.com> <20200909122200.GA2743@paquier.xyz>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <ff51e40c73df2c6ae5fc58f014f37cca@postgrespro.ru>
Precedence: bulk

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