Re: extend pgbench expressions with functions - Mailing list pgsql-hackers

Hello Robert,

>> Attached is a rebase after recent changes in pgbench code & doc.
>
> +/* use short names in the evaluator */
> +#define INT(v) coerceToInt(&v)
> +#define DOUBLE(v) coerceToDouble(&v)
> +#define SET_INT(pv, ival) setIntValue(pv, ival)
> +#define SET_DOUBLE(pv, dval) setDoubleValue(pv, dval)
>
> I don't like this at all.  It seems to me that this really obscures
> the code.  The few extra characters are a small price to pay for not
> having to go look up the macro definition to understand what the code
> is doing.

Hmmm. Postgres indentation rules for "switch" are peculiar to say the 
least and make it hard to write code that stay under 80 columns. The 
coerceToInt function name looks pretty long (I would rather have 
toInt/toDbl/setInt/setDbl) but I was "told" to use that, so I'm trying to 
find a tradeoff with a macro. Obviously I can substitude and have rather 
long lines that I personally find much uglier.

> The third hunk in pgbench.c unnecessary deletes a blank line.

Yep, that is possible.

>        /*
>         * inner expresion in (cut, 1] (if parameter > 0), rand in [0, 1)
> +        * Assert((1.0 - cut) != 0.0);
>         */
> -       Assert((1.0 - cut) != 0.0);
>        rand = -log(cut + (1.0 - cut) * uniform) / parameter;
> +
>
> Moving the Assert() into the comment seems like a bad plan.  If the
> Assert is true, it shouldn't be commented out.  If it's not, it
> shouldn't be there at all.

I put this assertion when I initially wrote this code, but I think that it 
is proven so I moved it in comment just as a reminder for someone who 
might touch anything that this must hold.

> Commit e41beea0ddb74ef975f08b917a354ec33cb60830, which you wrote, went
> to some trouble to display good context for error messages.  What you
> have here seems like a huge step backwards:
>
> +                       fprintf(stderr, "double to int overflow for
> %f\n", dval);
> +                       exit(1);
>
> So, we're just going to give up on all of that error context reporting
> that we added back then?  That would be sad.

Well, I'm a lazy programmer, so I'm trying to measure the benefit. IMO 
there is no benefit to better manage this case, especially as the various 
solution I thought of where either ugly and/or had a significant impact on 
the code.

Note that in the best case the error would be detected and reported and 
the client is stopped, and other clients go on... But then, if you started 
a bench and some clients die while running probably your results are 
meaningless, so my opinion is that you are better off with an exit than
with some message that you may miss and performance results computed with 
much less clients than you asked for.

Pgbench is a bench tool, not a production tool.

-- 
Fabien.