Thread: VACUUM fails to parse 0 and 1 as boolean value
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
Attachment
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
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
Attachment
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
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
Attachment
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
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
Attachment
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
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
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
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
Attachment
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
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
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
Attachment
> 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;
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.
> 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.
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
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
Attachment
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
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
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
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
Attachment
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
Attachment
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