Re: pgbench more operators & functions

Fabien, Jeevan,

* Jeevan Ladhe (jeevan(dot)ladhe(at)enterprisedb(dot)com) wrote:
> On Sat, Jul 9, 2016 at 12:14 PM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> wrote:
> >> Here is a simple patch which adds a bunch of operators (bitwise: & | ^ ~,
> >> comparisons: =/== <>/!= < <= > >=, logical: and/&& or/|| xor/^^ not/!) and
> >> functions (exp ln if) to pgbench. I've tried to be pg's SQL compatible where
> >> appropriate.
> >>
> >> Also attached is a simple test script.
> >
> > Here is a sightly updated version.
>
> I did the review of your patch and here are my views on your patch.

Thanks for the review, I agree with most of your comments and the patch
looks pretty good, in general, but I had a few specific questions.

Apologies if I missed where these were discussed already, I've just
been reading the thread linked from the CF app, so if there is prior
discussion that I should read, please provide a link or Message-ID and
I'll go read the thread(s).

> Purpose of the patch:
> =====================
>
> This patch introduces extensive list of new operators and functions that can be
> used in expressions in pgbench transaction scripts(given with -f option).
>
> Here is the list of operators and functions introduced by this patch:
>
> New operators:
> --------------
> bitwise: <<, >>, &, |, ^/#, ~
> comparisons: =/==, <>/!=, <, <=, >, >=
> logical: and/&&, or/||, xor/^^, not, !

I'm not sure that we want to introduce operators '&&', '||' as logical
'and' and 'or' when those have specific meaning in PG which is different
(array overlaps and concatenation, specifically). I can certainly see
how it could be useful to be able to perform string concatenation in a
\set command in pgbench, for example..

Also, are we sure that we want to have both '=' and '==' for comparison?

In general, my gut feeling is that we should be trying to make what's
available in PG available in pgbench, though I can see an argument for
making what is available in C available in pgbench, but this seems to be
more of a mix of the two and I think we'd be better off deciding which
we want and sticking to it.

> New functions:
> --------------
> exp, ln, if

I don't see any particular issue with the exp() and ln() functions. I
certainly understand how the if() function could be useful, but I'm not
entirely sure that the if(expression, true-result, false-result) is the
best approach. In particular, I recall cases with other languages where
that gets to be quite ugly when there are multiple levels of if/else
using that approach.

> Documentation:
> =============
> I have a small suggestion here: Earlier we had only few number of operators, so
> it was OK to have the operators embedded in the description of '\set' command
> itself, but now as we have much more number of operators will it be a good idea
> to have a table of operators similar to that of functions. We need not have
> several columns here like description, example etc., but a short table just
> categorizing the operators would be sufficient.

The table should really have a row per operator, which is what we do in
pretty much all of the core documention. In particular, it seems like
we should try to follow something like:

https://www.postgresql.org/docs/current/static/functions-math.html

Alternativly, if we decide that we really want to try and keep with how
PG works with these operators, we could simply make these operators work
like those and then refer to that page in the core docs.

The question which was brought up about having a line-continuation
(eg: '\') character would be useful, which really makes me wonder if we
shouldn't try to come up with a way to create functions in a pgbench
script that can later be used in a \set command. With such an approach,
we could have proper control structures for conditionals (if/else),
loops (while/for), etc, and not complicate the single-statement set of
operators with those constructs.

Lastly, we should really add some regression tests to pgbench. We
already have the start of a TAP test script which it looks like it
wouldn't be too hard to add on to.

Thanks!

Stephen