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 |
Message-ID: | 20180928050249.j2uyrvgus2rt33ey@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
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 |