On Wed, May 15, 2019 at 7:51 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, May 15, 2019 at 03:19:29AM +0900, Fujii Masao wrote:
> > + if (strncasecmp(opt_str, "true", 4) != 0 &&
> > + strncasecmp(opt_str, "false", 5) != 0)
> >
> > Shouldn't we allow also "on" and "off", "1", "0" as a valid boolean value,
> > like VACUUM does?
>
> I am wondering, in order to keep this patch simple, if you shouldn't
> accept any value and just let the parsing logic on the backend side
> do all the work. That's what we do for other things like the
> connection parameter replication for example, and there is no need to
> mimic a boolean parsing equivalent on the frontend with something like
> check_bool_str() as presented in the patch. The main downside is that
> the error message gets linked to VACUUM and not vacuumdb.
I might be missing something but if the frontend code doesn't check
arguments and we let the backend parsing logic do all the work then it
allows user to execute an arbitrary SQL command via vacuumdb.
>
> Another thing which you may be worth looking at would be to make
> parse_bool() frontend aware, where pg_strncasecmp() is actually
> available.
Or how about add a function that parse a boolean string value, as a
common routine among frontend programs, maybe in common.c or fe_utils?
We results in having the duplicate code between frontend and backend
but it may be less side effects than making parse_bool available on
frontend code.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center