Horton

Thread

Commits

  1. Fix more strcmp() calls using boolean-like comparisons for result checks

  2. Fix backwards test in operator_precedence_warning logic.

  1. Suspicious strcmp() in src/backend/parser/parse_expr.c

    Rikard Falkeborn <rikard.falkeborn@gmail.com> — 2019-04-10T21:40:53Z

    In src/backend/parser/parse_expr.c the following snippet of code is found
(lines 3238-3242, rev 765525c8c2c6e55abe):

    if (strcmp(*nodename, "+") == 0 ||
            strcmp(*nodename, "-")) // <-- notice the lack of comparisson
here
            group = 0;
    else
            group = PREC_GROUP_PREFIX_OP;

Should the second part of the || be strcmp(*nodename, "-") == 0?

  2. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-04-10T22:33:35Z

    >>>>> "Rikard" == Rikard Falkeborn <rikard.falkeborn@gmail.com> writes:

 Rikard> In src/backend/parser/parse_expr.c the following snippet of code is found
 Rikard> (lines 3238-3242, rev 765525c8c2c6e55abe):

 Rikard>     if (strcmp(*nodename, "+") == 0 ||
 Rikard>             strcmp(*nodename, "-")) // <-- notice the lack of comparisson
 Rikard> here
 Rikard>             group = 0;
 Rikard>     else
 Rikard>             group = PREC_GROUP_PREFIX_OP;

 Rikard> Should the second part of the || be strcmp(*nodename, "-") ==
 Rikard> 0?

Yes it should.

The effect of this bug is to produce a false operator precedence warning
when those are enabled, like so:

postgres=# set operator_precedence_warning = on;
SET
postgres=# select -random() is null;
WARNING:  operator precedence change: IS is now lower precedence than -

when in fact "-random() is null" always did parse as "(-random()) is null"
making the warning spurious.

-- 
Andrew (irc:RhodiumToad)

  3. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-10T22:43:32Z

    Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Rikard" == Rikard Falkeborn <rikard.falkeborn@gmail.com> writes:
>  Rikard>     if (strcmp(*nodename, "+") == 0 ||
>  Rikard>             strcmp(*nodename, "-")) // <-- notice the lack of comparisson here
>  Rikard> Should the second part of the || be strcmp(*nodename, "-") == 0?

> Yes it should.

Indeed.  Considering how much I hate using strcmp's result as a boolean,
you'd think I'd have got that right.  Thanks for noticing!

			regards, tom lane

  4. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Michael Paquier <michael@paquier.xyz> — 2019-04-11T02:19:46Z

    On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
> Indeed.  Considering how much I hate using strcmp's result as a boolean,
> you'd think I'd have got that right.  Thanks for noticing!

Just a note about those strcmp() calls using a boolean as return
result in the tree:
src/backend/commands/lockcmds.c: (!strcmp(rte->eref->aliasname, "old")
|| !strcmp(rte->eref->aliasname, "new")))
src/test/modules/test_rls_hooks/test_rls_hooks.c:   if
(strcmp(RelationGetRelationName(relation), "rls_test_permissive")
src/test/modules/test_rls_hooks/test_rls_hooks.c:       &&
strcmp(RelationGetRelationName(relation), "rls_test_both"))
src/test/modules/test_rls_hooks/test_rls_hooks.c:   if
(strcmp(RelationGetRelationName(relation), "rls_test_restrictive")
src/test/modules/test_rls_hooks/test_rls_hooks.c:       &&
strcmp(RelationGetRelationName(relation), "rls_test_both"))

Would it be worth changing these as well?
--
Michael

  5. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Thomas Munro <thomas.munro@gmail.com> — 2019-04-11T03:26:31Z

    On Thu, Apr 11, 2019 at 2:20 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
> > Indeed.  Considering how much I hate using strcmp's result as a boolean,
> > you'd think I'd have got that right.  Thanks for noticing!
>
> Just a note about those strcmp() calls using a boolean as return
> result in the tree:
> src/backend/commands/lockcmds.c: (!strcmp(rte->eref->aliasname, "old")

Don't look at contrib/spi/refint.c if you value your sanity.

-- 
Thomas Munro
https://enterprisedb.com

  6. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-11T03:43:49Z

    On Thu, 11 Apr 2019 at 15:27, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Thu, Apr 11, 2019 at 2:20 PM Michael Paquier <michael@paquier.xyz> wrote:
> > On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
> > > Indeed.  Considering how much I hate using strcmp's result as a boolean,
> > > you'd think I'd have got that right.  Thanks for noticing!
> >
> > Just a note about those strcmp() calls using a boolean as return
> > result in the tree:
> > src/backend/commands/lockcmds.c: (!strcmp(rte->eref->aliasname, "old")
>
> Don't look at contrib/spi/refint.c if you value your sanity.

hmm, yeah. Take a non-bool expression, make it into a bool expression,
then compare that to 0 to make it look like a non-bool expression.
Weird.

If we're fixing some, we may as well do them all.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  7. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Michael Paquier <michael@paquier.xyz> — 2019-04-11T03:46:00Z

    On Thu, Apr 11, 2019 at 03:26:31PM +1200, Thomas Munro wrote:
> Don't look at contrib/spi/refint.c if you value your sanity.

Perhaps I don't value it much then.  At least it compares to 0 after
compiling a set of bool results.
--
Michael

  8. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-11T04:55:49Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Apr 10, 2019 at 06:43:32PM -0400, Tom Lane wrote:
>> Indeed.  Considering how much I hate using strcmp's result as a boolean,
>> you'd think I'd have got that right.  Thanks for noticing!

> Would it be worth changing these as well?

I'd be +1 for that, just on the grounds of having consistent coding
style.  But I'm not sufficiently excited about it to do the work
myself ...

			regards, tom lane

  9. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Michael Paquier <michael@paquier.xyz> — 2019-04-11T06:50:55Z

    On Thu, Apr 11, 2019 at 12:55:49AM -0400, Tom Lane wrote:
> I'd be +1 for that, just on the grounds of having consistent coding
> style.  But I'm not sufficiently excited about it to do the work
> myself ...

Well, here you go as per the attached as we are on it.  I am not
noticing any other spots.
--
Michael

  10. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    David Rowley <david.rowley@2ndquadrant.com> — 2019-04-11T14:06:11Z

    On Thu, 11 Apr 2019 at 18:51, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 11, 2019 at 12:55:49AM -0400, Tom Lane wrote:
> > I'd be +1 for that, just on the grounds of having consistent coding
> > style.  But I'm not sufficiently excited about it to do the work
> > myself ...
>
> Well, here you go as per the attached as we are on it.  I am not
> noticing any other spots.

Patch looks fine to me.  I also made a quick pass and noticed a few
more things out of place.

formatting.c in NUM_prepare_locale()

else if (strcmp(Np->decimal, ",") !=0)


spell.c in NISortDictionary()

if (i == 0
|| strcmp(Conf->Spell[i]->p.flag, Conf->Spell[i - 1]->p.flag))

if (i == 0
|| strcmp(Conf->Spell[i]->p.flag, Conf->AffixData[curaffix]))

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  11. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-04-11T14:12:06Z

    David Rowley <david.rowley@2ndquadrant.com> writes:
> formatting.c in NUM_prepare_locale()

> else if (strcmp(Np->decimal, ",") !=0)

I'll bet a nickel that that's pgindent's fault.  It probably thinks
that "decimal" is a typedef (greps typedefs.list ... yes), and that
tends to result in funny space-omissions later in the line.

			regards, tom lane

  12. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Michael Paquier <michael@paquier.xyz> — 2019-04-12T00:58:57Z

    On Thu, Apr 11, 2019 at 10:12:06AM -0400, Tom Lane wrote:
> I'll bet a nickel that that's pgindent's fault.  It probably thinks
> that "decimal" is a typedef (greps typedefs.list ... yes), and that
> tends to result in funny space-omissions later in the line.

Yes, I can see as well that pgindent complains here.  As that would
result in more noise on follow-up pgindent runs, I am not changing
this one.
--
Michael

  13. Re: Suspicious strcmp() in src/backend/parser/parse_expr.c

    Michael Paquier <michael@paquier.xyz> — 2019-04-12T01:29:54Z

    On Fri, Apr 12, 2019 at 02:06:11AM +1200, David Rowley wrote:
> spell.c in NISortDictionary()
> 
> if (i == 0
> || strcmp(Conf->Spell[i]->p.flag, Conf->Spell[i - 1]->p.flag))
> 
> if (i == 0
> || strcmp(Conf->Spell[i]->p.flag, Conf->AffixData[curaffix]))

Good catch on these two.  I have included these and fixed all the
spots found in d527fda.
--
Michael