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