Re: pgbench's expression parsing & negative numbers

Hi,

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.

Removed

> > > + 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.

Greetings,

Andres Freund