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
