Re: pgbench more operators & functions - Mailing list pgsql-hackers

On Sat, Jul 9, 2016 at 12:14 PM, Fabien COELHO <coelho@cri.ensmp.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.
>

Hi Fabien,

I did the review of your patch and here are my views on your patch.


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, !

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

I see there had been a discussion about timing for submission of patch, but not
much about what the patch is doing, so I believe the purpose of patch is quite
acceptable.
Currently there are limited number of operators available in pgbench. So, I
think these new operators definitely make a value addition towards custom
scripts.

Documentation:
=============
I could build the documentation without any errors.
New operators and functions are well categorized and added in proper sections
of existing pgbench 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.

Initial Run:
============
I was able to apply patch with 'patch -p1'.
The testcase file(functions.sql) given along the patch gives an expected output.

Further testing and review:
===========================
1. Postgres treats '^' as exponentiation rather than XOR, and '#' as XOR.
Personally, I think it can cause confusion, so it will be better if we can stick
to the behavior of Postgres mathematical operators.

2. I could not see any tests for bitwise operators in the functions.sql file
that you have attached.

3. Precedence:
a. Unary operators have more precedence than binary operators. So, UNOT and
UINV should have precedence next to UMINUS.
I tried couple of test expressions using C Vs your patch(pgbench)

expression result_in_C result_in_pgbench
(~14-14+2) -27 -3
(!14-14+2) -12 0

b. Similarly shift operators should take more precedence over other bitwise
operators:

expression result_in_C result_in_pgbench
(4|1<<1) 6 10
(4^5&3) 5 1

c. Also, comparison would take more precedence over bitwise operators(&,|,^)
but shift operators.

expression result_in_C result_in_pgbench
(2&1<3) 1 0

In backend/parser/gram.y, I see that unary operators are given higher precedence
than other operators, but it too does not have explicit precedence defined for
bitwise operators.
I tried to check Postgres documentation for operator precedence, but in
'Lexical Structure'[1] the documentation does not mention anything about bitwise
operators. Also, SQL standards 99 does not talk much about operator precedence.
I may be wrong while trying to understand the precedence you defined here and
you might have picked this per some standard, but do you have any reference
which you considered for this?

4. If we are going to stick to current precedence, I think it will be good idea
to document it.

5. Sorry, I was not able to understand the "should exist" comment in following
snippet.

+"xor" { return XOR_OP; } /* should exist */
+"^^" { return XOR_OP; } /* should exist */

7. You may want to reword following comment:

+ else /* cannot get there */

To

+ else /* cannot get here */

8.
+ case PGBENCH_IF:
+ /* should it do a lazy evaluation of the branch? */
+ Assert(nargs == 3);
+ *retval = coerceToBool(&vargs[0]) ? vargs[1] : vargs[2];

I believe ternary operator does the lazy evaluation internally, but to be sure
you may consider rewriting this as following:

if (coerceToBool(&vargs[0]))
*retval = vargs[1];
else
*retval = vargs[2];

Conclusion:
===========
I have tested the patch and each of the operator is implemented correctly.
The only concern I have is precedence, otherwise the patch seems to be doing
what it is supposed to do.

[1]https://www.postgresql.org/docs/9.6/static/sql-syntax-lexical.html

Regards,
Jeevan Ladhe.