On Tue, Mar 26, 2019 at 10:31 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for reviewing the patch.
I don't think the approach in v20-0001 is quite right.
if (strcmp(opt->defname, "verbose") == 0)
- params.options |= VACOPT_VERBOSE;
+ params.options |= defGetBoolean(opt) ? VACOPT_VERBOSE : 0;
It seems to me that it would be better to do declare a separate
boolean for each flag at the top; e.g. bool verbose. Then here do
verbose = defGetBoolean(opt). And then after the loop do
params.options = (verbose ? VACOPT_VERBOSE : 0) | ... similarly for
other options.
The thing I don't like about the way you have it here is that it's not
going to work well for options that are true by default but can
optionally be set to false. In that case, you would need to start
with the bit set and then clear it, but |= can only set bits, not
clear them. I went and looked at the VACUUM (INDEX_CLEANUP) patch on
the other thread and it doesn't have any special handling for that
case, which makes me suspect that if you use that patch, the reloption
works as expected but VACUUM (INDEX_CLEANUP false) doesn't actually
succeed in disabling index cleanup. The structure I suggested above
would fix that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company