Horton

Thread

Commits

  1. Mention ANALYZE boolean options in documentation.

  2. Make VACUUM accept 1 and 0 as a boolean value.

  3. Allow existing VACUUM options to take a Boolean argument.

  1. VACUUM fails to parse 0 and 1 as boolean value

    Fujii Masao <masao.fujii@gmail.com> — 2019-05-14T17:45:21Z

    Hi,

VACUUM fails to parse 0 and 1 as boolean value

The document for VACUUM explains

    boolean
    Specifies whether the selected option should be turned on or off.
    You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF,
    or 0 to disable it.

But VACUUM fails to parse 0 and 1 as boolean value as follows.

    =# VACUUM (INDEX_CLEANUP 1);
    ERROR:  syntax error at or near "1" at character 23
    STATEMENT:  VACUUM (INDEX_CLEANUP 1);

This looks a bug. The cause of this is a lack of NumericOnly clause
for vac_analyze_option_arg in gram.y. The attached patch
adds such NumericOnly. The bug exists only in 12dev.

Barring any objection, I will commit the patch.

Regards,

-- 
Fujii Masao

  2. Re: VACUUM fails to parse 0 and 1 as boolean value

    Andres Freund <andres@anarazel.de> — 2019-05-14T17:52:23Z

    Hi,

On 2019-05-15 02:45:21 +0900, Fujii Masao wrote:
> VACUUM fails to parse 0 and 1 as boolean value
> 
> The document for VACUUM explains
> 
>     boolean
>     Specifies whether the selected option should be turned on or off.
>     You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF,
>     or 0 to disable it.
> 
> But VACUUM fails to parse 0 and 1 as boolean value as follows.
> 
>     =# VACUUM (INDEX_CLEANUP 1);
>     ERROR:  syntax error at or near "1" at character 23
>     STATEMENT:  VACUUM (INDEX_CLEANUP 1);
> 
> This looks a bug. The cause of this is a lack of NumericOnly clause
> for vac_analyze_option_arg in gram.y. The attached patch
> adds such NumericOnly. The bug exists only in 12dev.
> 
> Barring any objection, I will commit the patch.

Might be worth having a common rule for such options, so we don't
duplicate the knowledge between different places.

CCing Robert and Sawada-san, who committed / authored that code.

commit 41b54ba78e8c4d64679ba4daf82e4e2efefe1922
Author: Robert Haas <rhaas@postgresql.org>
Date:   2019-03-29 08:22:49 -0400

    Allow existing VACUUM options to take a Boolean argument.
    
    This makes VACUUM work more like EXPLAIN already does without changing
    the meaning of any commands that already work.  It is intended to
    facilitate the addition of future VACUUM options that may take
    non-Boolean parameters or that default to false.
    
    Masahiko Sawada, reviewed by me.
    
    Discussion: http://postgr.es/m/CA+TgmobpYrXr5sUaEe_T0boabV0DSm=utSOZzwCUNqfLEEm8Mw@mail.gmail.com
    Discussion: http://postgr.es/m/CAD21AoBaFcKBAeL5_++j+Vzir2vBBcF4juW7qH8b3HsQY=Q6+w@mail.gmail.com

Greetings,

Andres Freund

  3. Re: VACUUM fails to parse 0 and 1 as boolean value

    Michael Paquier <michael@paquier.xyz> — 2019-05-14T23:20:33Z

    On Tue, May 14, 2019 at 10:52:23AM -0700, Andres Freund wrote:
> Might be worth having a common rule for such options, so we don't
> duplicate the knowledge between different places.
> 
> CCing Robert and Sawada-san, who committed / authored that code.

Hmn.  I think that Robert's commit is right to rely on defGetBoolean()
for option parsing.  That's what we use for anything from CREATE
EXTENSION to CREATE SUBSCRIPTION, etc.
--
Michael

  4. Re: VACUUM fails to parse 0 and 1 as boolean value

    Andres Freund <andres@anarazel.de> — 2019-05-14T23:26:18Z

    Hi,

On 2019-05-15 08:20:33 +0900, Michael Paquier wrote:
> On Tue, May 14, 2019 at 10:52:23AM -0700, Andres Freund wrote:
> > Might be worth having a common rule for such options, so we don't
> > duplicate the knowledge between different places.
> > 
> > CCing Robert and Sawada-san, who committed / authored that code.
> 
> Hmn.  I think that Robert's commit is right to rely on defGetBoolean()
> for option parsing.  That's what we use for anything from CREATE
> EXTENSION to CREATE SUBSCRIPTION, etc.

That seems like a separate angle? What does that have to do with
accepting 0/1 in the grammar? I mean, EXPLAIN also uses defGetBoolean(),
while accepting NumericOnly for the option values?

Greetings,

Andres Freund

  5. Re: VACUUM fails to parse 0 and 1 as boolean value

    Michael Paquier <michael@paquier.xyz> — 2019-05-14T23:29:44Z

    On Wed, May 15, 2019 at 08:20:33AM +0900, Michael Paquier wrote:
> Hmn.  I think that Robert's commit is right to rely on defGetBoolean()
> for option parsing.  That's what we use for anything from CREATE
> EXTENSION to CREATE SUBSCRIPTION, etc.

And I need more coffee at this time of the day...  Because I have not
looked at the proposed patch.

The patch of Fujii-san does its job as far as it goes, but we have
more parsing nodes with the same logic:
- explain_option_arg, which is the same.
- copy_generic_opt_arg, which shares the same root.

So there is room for a common rule, still it does not impact that many
places.  I would have believed that more commands use that.
--
Michael

  6. Re: VACUUM fails to parse 0 and 1 as boolean value

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-05-15T01:46:03Z

    On Wed, May 15, 2019 at 2:52 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-05-15 02:45:21 +0900, Fujii Masao wrote:
> > VACUUM fails to parse 0 and 1 as boolean value
> >
> > The document for VACUUM explains
> >
> >     boolean
> >     Specifies whether the selected option should be turned on or off.
> >     You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF,
> >     or 0 to disable it.
> >
> > But VACUUM fails to parse 0 and 1 as boolean value as follows.
> >
> >     =# VACUUM (INDEX_CLEANUP 1);
> >     ERROR:  syntax error at or near "1" at character 23
> >     STATEMENT:  VACUUM (INDEX_CLEANUP 1);
> >
> > This looks a bug. The cause of this is a lack of NumericOnly clause
> > for vac_analyze_option_arg in gram.y. The attached patch
> > adds such NumericOnly. The bug exists only in 12dev.

Thank you for reporting and the patch.

> >
> > Barring any objection, I will commit the patch.
>
> Might be worth having a common rule for such options, so we don't
> duplicate the knowledge between different places.

+1 for committing this patch.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

  7. Re: VACUUM fails to parse 0 and 1 as boolean value

    Fujii Masao <masao.fujii@gmail.com> — 2019-05-16T18:56:17Z

    On Wed, May 15, 2019 at 2:52 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-05-15 02:45:21 +0900, Fujii Masao wrote:
> > VACUUM fails to parse 0 and 1 as boolean value
> >
> > The document for VACUUM explains
> >
> >     boolean
> >     Specifies whether the selected option should be turned on or off.
> >     You can write TRUE, ON, or 1 to enable the option, and FALSE, OFF,
> >     or 0 to disable it.
> >
> > But VACUUM fails to parse 0 and 1 as boolean value as follows.
> >
> >     =# VACUUM (INDEX_CLEANUP 1);
> >     ERROR:  syntax error at or near "1" at character 23
> >     STATEMENT:  VACUUM (INDEX_CLEANUP 1);
> >
> > This looks a bug. The cause of this is a lack of NumericOnly clause
> > for vac_analyze_option_arg in gram.y. The attached patch
> > adds such NumericOnly. The bug exists only in 12dev.
> >
> > Barring any objection, I will commit the patch.
>
> Might be worth having a common rule for such options, so we don't
> duplicate the knowledge between different places.

Yes. Thanks for the comment!
Attached is the updated version of the patch.
It adds such common rule.

Regards,

-- 
Fujii Masao

  8. Re: VACUUM fails to parse 0 and 1 as boolean value

    Robert Haas <robertmhaas@gmail.com> — 2019-05-16T19:29:36Z

    On Thu, May 16, 2019 at 2:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> Yes. Thanks for the comment!
> Attached is the updated version of the patch.
> It adds such common rule.

I'm not sure how much value it really has to define
opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
3 places, but costs 6 lines of code to have it.

Perhaps we could try to unify at a higher level.  Like can we merge
vac_analyze_option_list with explain_option_list?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: VACUUM fails to parse 0 and 1 as boolean value

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-05-17T01:21:21Z

    We now have several syntax elements seemingly the same but behave
different way.

At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobK1ngid9Pxs7g8RFQDH+O1X4yyL+vMQtaV7i6m-Xn0rw@mail.gmail.com>
> On Thu, May 16, 2019 at 2:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > Yes. Thanks for the comment!
> > Attached is the updated version of the patch.
> > It adds such common rule.
> 
> I'm not sure how much value it really has to define
> opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> 3 places, but costs 6 lines of code to have it.

ANALYZE (options) desn't accept 1/0 but only accepts true/false
or on/off. Why are we going to make VACUUM differently?

And the documentation for ANALYZE doesn't mention the change.

I think we don't need to support 1/0 as boolean here (it's
unnatural) and the documentation of VACUUM/ANALYZE should be
fixed.

> Perhaps we could try to unify at a higher level.  Like can we merge
> vac_analyze_option_list with explain_option_list?

Also REINDEX (VERBOSE) doesn't accept explict argument as of
now. (reindex_option_list)

I'm not sure about FDW/SERVER/CREATE USER MAPPING but perhaps
it's a different from this.

COPY .. WITH (options) doesn't accpet 1/0 as boolean.

copy_generic_opt_arg:
      opt_boolean_or_string      { $$ = (Node *) makeString($1); }
      | NumericOnly          { $$ = (Node *) $1; }
      | '*'              { $$ = (Node *) makeNode(A_Star); }
      | '(' copy_generic_opt_arg_list ')'    { $$ = (Node *) $2; }
      | /* EMPTY */          { $$ = NULL; }
    ;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  10. Re: VACUUM fails to parse 0 and 1 as boolean value

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-05-17T01:24:25Z

    Mmm. It has gone before complete.

At Fri, 17 May 2019 10:21:21 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190517.102121.72558057.horiguchi.kyotaro@lab.ntt.co.jp>
> We now have several syntax elements seemingly the same but behave
> different way.
> 
> At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobK1ngid9Pxs7g8RFQDH+O1X4yyL+vMQtaV7i6m-Xn0rw@mail.gmail.com>
> > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > > Yes. Thanks for the comment!
> > > Attached is the updated version of the patch.
> > > It adds such common rule.
> > 
> > I'm not sure how much value it really has to define
> > opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> > 3 places, but costs 6 lines of code to have it.
> 
> ANALYZE (options) desn't accept 1/0 but only accepts true/false
> or on/off. Why are we going to make VACUUM differently?

The patch changes the behvaior of ANALYZE together. Please ignore
this.

> And the documentation for ANALYZE doesn't mention the change.
> 
> I think we don't need to support 1/0 as boolean here (it's
> unnatural) and the documentation of VACUUM/ANALYZE should be
> fixed.
> 
> > Perhaps we could try to unify at a higher level.  Like can we merge
> > vac_analyze_option_list with explain_option_list?
> 
> Also REINDEX (VERBOSE) doesn't accept explict argument as of
> now. (reindex_option_list)
> 
> I'm not sure about FDW/SERVER/CREATE USER MAPPING but perhaps
> it's a different from this.
> 
> COPY .. WITH (options) doesn't accpet 1/0 as boolean.
> 
> copy_generic_opt_arg:
>       opt_boolean_or_string      { $$ = (Node *) makeString($1); }
>       | NumericOnly          { $$ = (Node *) $1; }
>       | '*'              { $$ = (Node *) makeNode(A_Star); }
>       | '(' copy_generic_opt_arg_list ')'    { $$ = (Node *) $2; }
>       | /* EMPTY */          { $$ = NULL; }
>     ;

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  11. Re: VACUUM fails to parse 0 and 1 as boolean value

    Michael Paquier <michael@paquier.xyz> — 2019-05-17T01:34:56Z

    On Thu, May 16, 2019 at 03:29:36PM -0400, Robert Haas wrote:
> I'm not sure how much value it really has to define
> opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> 3 places, but costs 6 lines of code to have it.
> 
> Perhaps we could try to unify at a higher level.  Like can we merge
> vac_analyze_option_list with explain_option_list?

var_value has also similar semantics, and it uses makeAConst().  At
this point of the game, I'd like to think that it would be just better
to leave all the refactoring behind us on HEAD, to apply the first
patch presented on this thread, and to work more on that for v13 once
it opens for business if there is a patch to discuss about.
--
Michael

  12. Re: VACUUM fails to parse 0 and 1 as boolean value

    Fujii Masao <masao.fujii@gmail.com> — 2019-05-20T02:47:08Z

    On Fri, May 17, 2019 at 10:35 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, May 16, 2019 at 03:29:36PM -0400, Robert Haas wrote:
> > I'm not sure how much value it really has to define
> > opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> > 3 places, but costs 6 lines of code to have it.
> >
> > Perhaps we could try to unify at a higher level.  Like can we merge
> > vac_analyze_option_list with explain_option_list?
>
> var_value has also similar semantics, and it uses makeAConst().  At
> this point of the game, I'd like to think that it would be just better
> to leave all the refactoring behind us on HEAD, to apply the first
> patch presented on this thread, and to work more on that for v13 once
> it opens for business if there is a patch to discuss about.

We can refactor the gram.y several ways and it's not good to
rush the partial refactoring code into v12 before beta.
I'm ok to apply the first patch and focus on the bugfix at this moment.

Regards,

-- 
Fujii Masao

  13. Re: VACUUM fails to parse 0 and 1 as boolean value

    Robert Haas <robertmhaas@gmail.com> — 2019-05-20T13:55:59Z

    On Thu, May 16, 2019 at 9:21 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> I think we don't need to support 1/0 as boolean here (it's
> unnatural) and the documentation of VACUUM/ANALYZE should be
> fixed.

Well, it's confusing that we're not consistent about which spellings
are accepted.  The GUC system accepts true/false, on/off, and 0/1, so
it seems reasonable to me to standardize on that treatment across the
board.  That's not necessarily something we have to do for v12, but
longer-term, consistency is of value.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  14. Re: VACUUM fails to parse 0 and 1 as boolean value

    Fujii Masao <masao.fujii@gmail.com> — 2019-05-20T17:10:24Z

    On Fri, May 17, 2019 at 10:21 AM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>
> We now have several syntax elements seemingly the same but behave
> different way.
>
> At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobK1ngid9Pxs7g8RFQDH+O1X4yyL+vMQtaV7i6m-Xn0rw@mail.gmail.com>
> > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > > Yes. Thanks for the comment!
> > > Attached is the updated version of the patch.
> > > It adds such common rule.
> >
> > I'm not sure how much value it really has to define
> > opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> > 3 places, but costs 6 lines of code to have it.
>
> ANALYZE (options) desn't accept 1/0 but only accepts true/false
> or on/off. Why are we going to make VACUUM differently?
>
> And the documentation for ANALYZE doesn't mention the change.

Commit 41b54ba78e seems to affect also ANALYZE syntax.
If it's intentional, IMO we should apply the attached patch.
Thought?

Regards,

-- 
Fujii Masao

  15. Re: VACUUM fails to parse 0 and 1 as boolean value

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-05-20T19:14:30Z

    > On Thu, May 16, 2019 at 8:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> Yes. Thanks for the comment!
> Attached is the updated version of the patch.
> It adds such common rule.

If I understand correctly, it resulted in the commit fc7c281f8. For some reason
it breaks vacuum tests for me, is it expected?

     ANALYZE (nonexistent-arg) does_not_exist;
    -ERROR:  syntax error at or near "-"
    +ERROR:  syntax error at or near "arg"
     LINE 1: ANALYZE (nonexistent-arg) does_not_exist;
    -                            ^
    +                             ^
     ANALYZE (nonexistentarg) does_not_exit;

  16. Re: VACUUM fails to parse 0 and 1 as boolean value

    Andres Freund <andres@anarazel.de> — 2019-05-20T19:20:35Z

    Hi,

On May 20, 2019 12:14:30 PM PDT, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>> On Thu, May 16, 2019 at 8:56 PM Fujii Masao <masao.fujii@gmail.com>
>wrote:
>>
>> Yes. Thanks for the comment!
>> Attached is the updated version of the patch.
>> It adds such common rule.
>
>If I understand correctly, it resulted in the commit fc7c281f8. For
>some reason
>it breaks vacuum tests for me, is it expected?
>
>     ANALYZE (nonexistent-arg) does_not_exist;
>    -ERROR:  syntax error at or near "-"
>    +ERROR:  syntax error at or near "arg"
>     LINE 1: ANALYZE (nonexistent-arg) does_not_exist;
>    -                            ^
>    +                             ^
>     ANALYZE (nonexistentarg) does_not_exit;

That has since been fixed, right?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  17. Re: VACUUM fails to parse 0 and 1 as boolean value

    Dmitry Dolgov <9erthalion6@gmail.com> — 2019-05-20T19:22:34Z

    > On Mon, May 20, 2019 at 9:20 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On May 20, 2019 12:14:30 PM PDT, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
> >> On Thu, May 16, 2019 at 8:56 PM Fujii Masao <masao.fujii@gmail.com>
> >wrote:
> >>
> >> Yes. Thanks for the comment!
> >> Attached is the updated version of the patch.
> >> It adds such common rule.
> >
> >If I understand correctly, it resulted in the commit fc7c281f8. For
> >some reason
> >it breaks vacuum tests for me, is it expected?
> >
> >     ANALYZE (nonexistent-arg) does_not_exist;
> >    -ERROR:  syntax error at or near "-"
> >    +ERROR:  syntax error at or near "arg"
> >     LINE 1: ANALYZE (nonexistent-arg) does_not_exist;
> >    -                            ^
> >    +                             ^
> >     ANALYZE (nonexistentarg) does_not_exit;
>
> That has since been fixed, right?

Yep, right, after I've checkout 47a14c99e471. Sorry for the noise.

  18. Re: VACUUM fails to parse 0 and 1 as boolean value

    Masahiko Sawada <sawada.mshk@gmail.com> — 2019-05-21T02:41:08Z

    On Tue, May 21, 2019 at 2:10 AM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Fri, May 17, 2019 at 10:21 AM Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> >
> > We now have several syntax elements seemingly the same but behave
> > different way.
> >
> > At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobK1ngid9Pxs7g8RFQDH+O1X4yyL+vMQtaV7i6m-Xn0rw@mail.gmail.com>
> > > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > > > Yes. Thanks for the comment!
> > > > Attached is the updated version of the patch.
> > > > It adds such common rule.
> > >
> > > I'm not sure how much value it really has to define
> > > opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> > > 3 places, but costs 6 lines of code to have it.
> >
> > ANALYZE (options) desn't accept 1/0 but only accepts true/false
> > or on/off. Why are we going to make VACUUM differently?
> >
> > And the documentation for ANALYZE doesn't mention the change.
>
> Commit 41b54ba78e seems to affect also ANALYZE syntax.
> If it's intentional, IMO we should apply the attached patch.
> Thought?
>

+1
Thank you for the patch!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

  19. Re: VACUUM fails to parse 0 and 1 as boolean value

    Michael Paquier <michael@paquier.xyz> — 2019-05-21T05:31:32Z

    On Mon, May 20, 2019 at 09:55:59AM -0400, Robert Haas wrote:
> Well, it's confusing that we're not consistent about which spellings
> are accepted.  The GUC system accepts true/false, on/off, and 0/1, so
> it seems reasonable to me to standardize on that treatment across the
> board.  That's not necessarily something we have to do for v12, but
> longer-term, consistency is of value.

+1.

Note: boolean GUCs accept a bit more: yes, no, tr, fa, and their upper
case flavors, etc.  These are everything parse_bool():bool.c accepts
as valid values.
--
Michael

  20. Re: VACUUM fails to parse 0 and 1 as boolean value

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2019-05-21T07:00:25Z

    At Tue, 21 May 2019 14:31:32 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190521053132.GG1921@paquier.xyz>
> On Mon, May 20, 2019 at 09:55:59AM -0400, Robert Haas wrote:
> > Well, it's confusing that we're not consistent about which spellings
> > are accepted.  The GUC system accepts true/false, on/off, and 0/1, so
> > it seems reasonable to me to standardize on that treatment across the
> > board.  That's not necessarily something we have to do for v12, but
> > longer-term, consistency is of value.
> 
> +1.
> 
> Note: boolean GUCs accept a bit more: yes, no, tr, fa, and their upper
> case flavors, etc.  These are everything parse_bool():bool.c accepts
> as valid values.

Yeah, I agree for longer-term. The opinion was short-term
consideration on v12. We would be able to achieve full
unification on sub-applications in v13 in that direction. (But I
don't think it's good that apps pass-through options then server
checkes them..)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  21. Re: VACUUM fails to parse 0 and 1 as boolean value

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-05-21T13:40:22Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, May 20, 2019 at 09:55:59AM -0400, Robert Haas wrote:
>> Well, it's confusing that we're not consistent about which spellings
>> are accepted.  The GUC system accepts true/false, on/off, and 0/1, so
>> it seems reasonable to me to standardize on that treatment across the
>> board.  That's not necessarily something we have to do for v12, but
>> longer-term, consistency is of value.

> +1.

> Note: boolean GUCs accept a bit more: yes, no, tr, fa, and their upper
> case flavors, etc.  These are everything parse_bool():bool.c accepts
> as valid values.

I'm not excited about allowing abbreviated keywords here, though.
Allowing true/false, on/off, and 0/1 seems reasonable but let's
not go overboard.

			regards, tom lane

  22. Re: VACUUM fails to parse 0 and 1 as boolean value

    Andres Freund <andres@anarazel.de> — 2019-05-21T15:19:59Z

    Hi,

On 2019-05-21 16:00:25 +0900, Kyotaro HORIGUCHI wrote:
> At Tue, 21 May 2019 14:31:32 +0900, Michael Paquier <michael@paquier.xyz> wrote in <20190521053132.GG1921@paquier.xyz>
> > On Mon, May 20, 2019 at 09:55:59AM -0400, Robert Haas wrote:
> > > Well, it's confusing that we're not consistent about which spellings
> > > are accepted.  The GUC system accepts true/false, on/off, and 0/1, so
> > > it seems reasonable to me to standardize on that treatment across the
> > > board.  That's not necessarily something we have to do for v12, but
> > > longer-term, consistency is of value.
> > 
> > +1.
> > 
> > Note: boolean GUCs accept a bit more: yes, no, tr, fa, and their upper
> > case flavors, etc.  These are everything parse_bool():bool.c accepts
> > as valid values.
> 
> Yeah, I agree for longer-term. The opinion was short-term
> consideration on v12. We would be able to achieve full
> unification on sub-applications in v13 in that direction. (But I
> don't think it's good that apps pass-through options then server
> checkes them..)

To me it is odd to introduce an option, just to revamp the accepted
style of arguments in the next release. I think we ought to just clean
this up now.

Greetings,

Andres Freund

  23. Re: VACUUM fails to parse 0 and 1 as boolean value

    Fujii Masao <masao.fujii@gmail.com> — 2019-05-21T19:32:38Z

    On Tue, May 21, 2019 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, May 21, 2019 at 2:10 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Fri, May 17, 2019 at 10:21 AM Kyotaro HORIGUCHI
> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > >
> > > We now have several syntax elements seemingly the same but behave
> > > different way.
> > >
> > > At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <robertmhaas@gmail.com> wrote in <CA+TgmobK1ngid9Pxs7g8RFQDH+O1X4yyL+vMQtaV7i6m-Xn0rw@mail.gmail.com>
> > > > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> > > > > Yes. Thanks for the comment!
> > > > > Attached is the updated version of the patch.
> > > > > It adds such common rule.
> > > >
> > > > I'm not sure how much value it really has to define
> > > > opt_boolean_or_string_or_numeric.  It saves 1 line of code in each of
> > > > 3 places, but costs 6 lines of code to have it.
> > >
> > > ANALYZE (options) desn't accept 1/0 but only accepts true/false
> > > or on/off. Why are we going to make VACUUM differently?
> > >
> > > And the documentation for ANALYZE doesn't mention the change.
> >
> > Commit 41b54ba78e seems to affect also ANALYZE syntax.
> > If it's intentional, IMO we should apply the attached patch.
> > Thought?
> >
>
> +1
> Thank you for the patch!

I found that tab-completion also needs to be updated for ANALYZE
boolean options. I added that change for tab-completion into
the patch and am thinking to apply the attached patch.

Regards,

-- 
Fujii Masao

  24. Re: VACUUM fails to parse 0 and 1 as boolean value

    Michael Paquier <michael@paquier.xyz> — 2019-05-22T07:46:42Z

    On Wed, May 22, 2019 at 04:32:38AM +0900, Fujii Masao wrote:
> I found that tab-completion also needs to be updated for ANALYZE
> boolean options. I added that change for tab-completion into
> the patch and am thinking to apply the attached patch.

Looks fine to me at quick glance.
--
Michael

  25. Re: VACUUM fails to parse 0 and 1 as boolean value

    Fujii Masao <masao.fujii@gmail.com> — 2019-05-22T16:20:53Z

    On Wed, May 22, 2019 at 4:47 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, May 22, 2019 at 04:32:38AM +0900, Fujii Masao wrote:
> > I found that tab-completion also needs to be updated for ANALYZE
> > boolean options. I added that change for tab-completion into
> > the patch and am thinking to apply the attached patch.
>
> Looks fine to me at quick glance.

Thanks! Committed.

Regards,

-- 
Fujii Masao