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

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