VACUUM fails to parse 0 and 1 as boolean value

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

VACUUM fails to parse 0 and 1 as boolean value

Fujii Masao-2
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

vacuum_numeric_as_boolean.patch (540 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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 <[hidden email]>
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@...
    Discussion: http://postgr.es/m/CAD21AoBaFcKBAeL5_++j+Vzir2vBBcF4juW7qH8b3HsQY=Q6+w@...

Greetings,

Andres Freund


Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
In reply to this post by Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Masahiko Sawada
In reply to this post by Andres Freund
On Wed, May 15, 2019 at 2:52 AM Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-2
In reply to this post by Andres Freund
On Wed, May 15, 2019 at 2:52 AM Andres Freund <[hidden email]> 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

vacuum_numeric_as_boolean_v2.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
On Thu, May 16, 2019 at 2:56 PM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
We now have several syntax elements seemingly the same but behave
different way.

At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <[hidden email]> wrote in <[hidden email]>
> On Thu, May 16, 2019 at 2:56 PM Fujii Masao <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
Mmm. It has gone before complete.

At Fri, 17 May 2019 10:21:21 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <[hidden email]> wrote in <[hidden email]>

> We now have several syntax elements seemingly the same but behave
> different way.
>
> At Thu, 16 May 2019 15:29:36 -0400, Robert Haas <[hidden email]> wrote in <[hidden email]>
> > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
In reply to this post by Robert Haas
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-2
On Fri, May 17, 2019 at 10:35 AM Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
In reply to this post by Kyotaro HORIGUCHI-2
On Thu, May 16, 2019 at 9:21 PM Kyotaro HORIGUCHI
<[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Fujii Masao-2
In reply to this post by Kyotaro HORIGUCHI-2
On Fri, May 17, 2019 at 10:21 AM Kyotaro HORIGUCHI
<[hidden email]> 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 <[hidden email]> wrote in <[hidden email]>
> > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <[hidden email]> 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

analyze-option-doc.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On Thu, May 16, 2019 at 8:56 PM Fujii Masao <[hidden email]> 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;


Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
Hi,

On May 20, 2019 12:14:30 PM PDT, Dmitry Dolgov <[hidden email]> wrote:

>> On Thu, May 16, 2019 at 8:56 PM Fujii Masao <[hidden email]>
>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.


Reply | Threaded
Open this post in threaded view
|

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

Dmitry Dolgov
> On Mon, May 20, 2019 at 9:20 PM Andres Freund <[hidden email]> wrote:
>
> Hi,
>
> On May 20, 2019 12:14:30 PM PDT, Dmitry Dolgov <[hidden email]> wrote:
> >> On Thu, May 16, 2019 at 8:56 PM Fujii Masao <[hidden email]>
> >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.


Reply | Threaded
Open this post in threaded view
|

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

Masahiko Sawada
In reply to this post by Fujii Masao-2
On Tue, May 21, 2019 at 2:10 AM Fujii Masao <[hidden email]> wrote:

>
> On Fri, May 17, 2019 at 10:21 AM Kyotaro HORIGUCHI
> <[hidden email]> 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 <[hidden email]> wrote in <[hidden email]>
> > > On Thu, May 16, 2019 at 2:56 PM Fujii Masao <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
In reply to this post by Robert Haas
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Kyotaro HORIGUCHI-2
At Tue, 21 May 2019 14:31:32 +0900, Michael Paquier <[hidden email]> wrote in <[hidden email]>

> 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



12