Re: pgbench's expression parsing & negative numbers

From: Andres Freund <andres(at)anarazel(dot)de>
To: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
Cc: Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com>, PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: pgbench's expression parsing & negative numbers
Date: 2018-09-28 05:02:49
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 2018-09-26 22:38:41 +0200, Fabien COELHO wrote:
> > > +"9223372036854775808" {
> > > + /* yuk: special handling for minint */
> > > + return MAXINT_PLUS_ONE_CONST;
> > > + }
> >
> > Yikes, do we really need this? If we dealt with - in the lexer, we
> > shouldn't need it, no?
> I'm not sure how to handle unary minus constant in the lexer, given that the
> same '-' character is also used as minus binary operator.

True. I think the nicer fix than what you've done here is to instead
simply always store the number, as coming from the lexer, as the
negative number. We do similarly in a number of places. I've gone with
yours for now.

> > > + /* this can't happen here, function called on int-looking strings. */
> >
> > This comment doesn't strike me as a good idea, it's almost guaranteed to
> > be outdated at some point.
> It is valid now, and it can be removed anyway.


> > > + if (unlikely(end == str || *end != '\0'))
> > Not sure I see much point in the unlikelys here, contrasting to the ones
> > in the backend (and the copy for the frontend) it's extremely unlikley
> > anything like this will ever be close to a bottleneck. In general, I'd
> > strongly suggest not placing unlikelys in non-critical codepaths -
> > they're way too often wrongly estimated.
> I put "unlikely" where I really thought it made sense. I do not know when
> they would have an actual performance impact, but I appreciate that they
> convey information to the reader of the code, telling that this path is
> expected not to be taken.

I removed them.

Pushed, thanks for the patch! I only did some very minor comment
changes, reset errno to 0 before strtod, removed a redundant
multiplication in PGBENCH_MUL.

FWIW, after this, and fixing the prerequisite general code paths, the
pgbench tests pass without -fsanitize=signed-integer-overflow errors.


Andres Freund

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2018-09-28 05:19:53 Re: file cloning in pg_upgrade and CREATE DATABASE
Previous Message Edmund Horner 2018-09-28 05:02:04 Re: Tid scan improvements