MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 23 Dec 2020 19:30:35 +0300
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>, Alvaro Herrera
 <alvherre@alvh.no-ip.org>, Peter Eisentraut
 <peter.eisentraut@enterprisedb.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: <X+Lz6l/mz3cxA/Cp@paquier.xyz>
References: <7ec67c56-2377-cd05-51a0-691104404abe@enterprisedb.com>
 <20201216004517.GA18498@alvherre.pgsql> <X9lcV/nYVU5WYf7D@paquier.xyz>
 <X+GWnexHZdt/aaVl@paquier.xyz> <20201222083204.GL30237@telsasoft.com>
 <X+HDFVXTYbTJMSXZ@paquier.xyz> <20201222211537.GO30237@telsasoft.com>
 <X+Lz6l/mz3cxA/Cp@paquier.xyz>
User-Agent: Roundcube Webmail/1.4.0
Message-ID: <2f88c198e80e7352eb03de44017b929a@postgrespro.ru>
Precedence: bulk

On 2020-12-23 10:38, Michael Paquier wrote:
> On Tue, Dec 22, 2020 at 03:15:37PM -0600, Justin Pryzby wrote:
>> Now, I really think utility.c ought to pass in a pointer to a local
>> ReindexOptions variable to avoid all the memory context, which is 
>> unnecessary
>> and prone to error.
> 
> Yeah, it sounds right to me to just bite the bullet and do this
> refactoring, limiting the manipulations of the options as much as
> possible across contexts.  So +1 from me to merge 0001 and 0002
> together.
> 
> I have adjusted a couple of comments and simplified a bit more the
> code in utility.c.  I think that this is commitable, but let's wait
> more than a couple of days for Alvaro and Peter first.  This is a
> period of vacations for a lot of people, and there is no point to
> apply something that would need more work at the end.  Using hexas for
> the flags with bitmasks is the right conclusion IMO, but we are not
> alone.
> 

After eyeballing the patch I can add that we should alter this comment:

	int	options;	/* bitmask of VacuumOption */

as you are going to replace VacuumOption with VACOPT_* defs. So this 
should say:

/* bitmask of VACOPT_* */

Also I have found naming to be a bit inconsistent:

  * we have ReindexOptions, but VacuumParams
  * and ReindexOptions->flags, but VacuumParams->options

And the last one, you have used bits32 for Cluster/ReindexOptions, but 
left VacuumParams->options as int. Maybe we should also change it to 
bits32 for consistency?


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company