pgbench more operators & functions

On 3 April 2016 at 06:54, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Here is a simple patch...

The patch deadline has passed and we are in the last CF of 9.6, as I'm sure
you know.

Another minor patch on pgbench probably isn't going to help stabilise this
release, so these changes won't be available in core until late 2017 now.

Given that, please save up all your desired changes to pgbench and submit
in one go nearer the next CF. Thanks.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Simon,

Here is a simple patch...

The patch deadline has passed and we are in the last CF of 9.6, as I'm
sure you know.

Yes I know, I'm ok with that, I was just putting stuff in the queue for
later, I was not asking for the patch to be considered right now.

Another minor patch on pgbench probably isn't going to help stabilise this
release, so these changes won't be available in core until late 2017 now.

Sure.

Given that, please save up all your desired changes to pgbench and submit
in one go nearer the next CF. Thanks.

Ok. Sorry, I did not realise that submitting stuff and recording it in a
CF should not be done now.

Maybe you should consider not opening the September CF if this is the
intent?

Also, what period "nearer to the next CF" is appropriate for sending
patches for this CF, which starts in five months?

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Apr 4, 2016 at 1:15 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Here is a simple patch...

The patch deadline has passed and we are in the last CF of 9.6, as I'm
sure you know.

Yes I know, I'm ok with that, I was just putting stuff in the queue for
later, I was not asking for the patch to be considered right now.

There is nothing bad in sending a patch now. Though it is true that at
this period of the 9.6 development attention should be to focus on the
remaining patches in the CF.

Given that, please save up all your desired changes to pgbench and submit
in one go nearer the next CF. Thanks.

Ok. Sorry, I did not realise that submitting stuff and recording it in a CF
should not be done now.

Personally I have no problem if someone wants to register a patch,
however reviews on such a patch are unfair for the other existing
ones. Perhaps you got an idea and wanted to code it and thought that
it would be a good idea to send it now instead of three month later.
I'd say why not.

Maybe you should consider not opening the September CF if this is the
intent?
Also, what period "nearer to the next CF" is appropriate for sending patches
for this CF, which starts in five months?

The CF can remain open as far as it goes in my view to allow people to
add patches whenever they want, I see little point to close it and
prevent people from registering patches if they'd want to. They are
just not going to be considered for review and integration until the
next CF begins if those are new features, note that some of the
patches registered there are aimed at being bug fixes.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 4 April 2016 at 01:14, Michael Paquier <michael.paquier@gmail.com> wrote:

I'd say why not.

I'd say "why not wait?". Minor, non-urgent patches will definitely go
nowhere for a long time, so it gains nobody to submit now.

Submitting patches during freeze has been discouraged for many years, so
asking a long term contributor to avoid sending multiple minor patches is
in line with that.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sun, Apr 3, 2016 at 10:18 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 4 April 2016 at 01:14, Michael Paquier <michael.paquier@gmail.com>
wrote:

I'd say why not.

I'd say "why not wait?". Minor, non-urgent patches will definitely go
nowhere for a long time, so it gains nobody to submit now.

Submitting patches during freeze has been discouraged for many years, so
asking a long term contributor to avoid sending multiple minor patches is
in line with that.

The main downside I see is on the CF manager having more items to manage.
The committers should be able to prioritize and so seeing the other items,
while maybe not ideal (though they should be in a future CF period so they
shouldn't be too visible), doesn't seem that bad. What it does allow is
for lurkers or potential reviewers and developers to see what is being (has
been) worked on by others in the community. That kind of visibility seems
like it should be desired - since proving that nobody benefits from it
being published seem a bit of stretch of reason. But maybe I'm just not
close enough to the problems it causes - which ideally could be mitigated
in some form other than asking people to hold off making work public.

The main downside would be the human tendency to want to look at, comment
and/or work on these more minor items when they should be working on more
important things. That, though, seem like the opposite of saying
"non-urgent patches will definitely go nowhere for a long time" and
probably installs a level of parental involvement that is not necessarily
the community's role.

David J.

On 2016-04-04 06:18:47 +0100, Simon Riggs wrote:

I'd say "why not wait?". Minor, non-urgent patches will definitely go
nowhere for a long time, so it gains nobody to submit now.

Submitting patches during freeze has been discouraged for many years, so
asking a long term contributor to avoid sending multiple minor patches is
in line with that.

I don't see much point in asking people to postpone. I do think however
it can make sense to respond with something like:
Fabien, you've been submitting a lot of patches over the last
year. Thanks for the that! To keep up with the amount of incoming work
the prject relies on contributors also shouldering some review
responsibility. Please consider focusing on that, while we're working on
getting 9.6 ready.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Apr 4, 2016 at 11:51 AM, Andres Freund <andres@anarazel.de> wrote:

On 2016-04-04 06:18:47 +0100, Simon Riggs wrote:

I'd say "why not wait?". Minor, non-urgent patches will definitely go
nowhere for a long time, so it gains nobody to submit now.

Submitting patches during freeze has been discouraged for many years, so
asking a long term contributor to avoid sending multiple minor patches

is

in line with that.

I don't see much point in asking people to postpone. I do think however
it can make sense to respond with something like:
Fabien, you've been submitting a lot of patches over the last
year. Thanks for the that! To keep up with the amount of incoming work
the prject relies on contributors also shouldering some review
responsibility. Please consider focusing on that, while we're working on
getting 9.6 ready.

+1. Extremely positive and encouraging way of involving other people.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hello Andres,

I don't see much point in asking people to postpone. I do think however
it can make sense to respond with something like: Fabien, you've been
submitting a lot of patches over the last year. Thanks for the that! To
keep up with the amount of incoming work the prject relies on
contributors also shouldering some review responsibility. Please
consider focusing on that, while we're working on getting 9.6 ready.

Sure, I definitely agree about that.

I try to review all patches in my (small) area of (limited) expertise,
which is currently pgbench & some part of the checkpointer. I did also
minor bug fixes (eg isbn). AFAICS none of the patches lacking a reviewer
in 9.6 CF fall in these area.

Also note that while I submitted patches for the checkpointer, I ended up
reviewing your version of the patches, so somehow I was first author, then
a driving force to provoke you to do it your way, and finally a reviewer,
esp in performance testing which is a time consumming task.

I can also learn other things, but that means more time to do a useful
review. This "more" time is available for me mostly over the Summer, so
I'll try to be more useful to the community, and also learn new stuff,
then. Probably not ideal for 9.6, but it cannot be helped.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien COELHO wrote:

I try to review all patches in my (small) area of (limited) expertise, which
is currently pgbench & some part of the checkpointer. I did also minor bug
fixes (eg isbn). AFAICS none of the patches lacking a reviewer in 9.6 CF
fall in these area.

Also note that while I submitted patches for the checkpointer, I ended up
reviewing your version of the patches, so somehow I was first author, then a
driving force to provoke you to do it your way, and finally a reviewer,
esp in performance testing which is a time consumming task.

Please note that the checkpointer patch has two open items that perhaps
you can help with --- see https://wiki.postgresql.org/wiki/Open_Items

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Please note that the checkpointer patch has two open items that perhaps
you can help with --- see https://wiki.postgresql.org/wiki/Open_Items

Indeed, I just looked at the commitfest, and I did not notice the other
threads.

I do not have an OSX available, but I'll have a look at the other one.

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Fabien.

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]https://www.postgresql.org/docs/9.6/static/sql-syntax-lexical.html 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]https://www.postgresql.org/docs/9.6/static/sql-syntax-lexical.html;
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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hello Jeevan,

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

Thanks for this detailed review and debugging!

Documentation: [...] 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.

Ok, done.

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.

Ok. I agree to avoid '^'.

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

Indeed. Included in attached version.

3. Precedence: [...]

Hmm. I got them all wrong, shame on me! I've followed C rules in the
updated version.

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 */

There is no "logical exclusive or" operator in C nor in SQL. I do not see
why not, so I put one...

7. You may want to reword following comment: [...] there -> here

Ok, fixed twice.

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

Ok.

Attached is an updated patch & test script.

--
Fabien.

Hi,

The patch has correct precedence now.

Further minor comments:

1. About documentation, I think it will be good idea to arrange the
operators
table with the precedence and add a line at top: "In decreasing order of
precedence".

2. You may want to remove the comment:
+ /* should it do a lazy evaluation of the branch? */

Regards,
Jeevan Ladhe

Hello Jeevan.

1. About documentation, I think it will be good idea to arrange the
operators table with the precedence and add a line at top: "In
decreasing order of precedence".

Done, see attached.

2. You may want to remove the comment:
+ /* should it do a lazy evaluation of the branch? */

Ok.

--
Fabien.

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

The patch looks good to me now.
Passing this to committer.

The new status of this patch is: Ready for Committer

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien, Jeevan,

* Jeevan Ladhe (jeevan.ladhe@enterprisedb.com) wrote:

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.

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

Hello Stephen,

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?

The reason I added C operators is that Pg SQL has '!=' and a few others,
so once some are there I do not see why others should not be there as well
for consistency.

Now I agree that having operators which behave differently from psql to
pgbench is a bad idea.

In the attached patched I only included pg operators, plus "xor" which I
feel is missing and does not seem to harm.

[...] I certainly understand how the if() function could be useful

Indeed, some kind of "if" is needed, for instance to implement "tpc-b"
correctly.

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

I think that pgbench is not a programming language, but an expression
language, which means that you have to provide a result, so you can only
have balanced if/then/else, which simplifies things a bit compared to
"full" language.

The SQL syntax for CASE is on the very heavy side and would be quite
complicated to implement in pgbench, so I rejected that and selected the
simplest possible function for the job.

Maybe the "if" function could be named something fancier to avoid possible
future clash on an eventual "if" keyword? Note that excel has an IF
function. Maybe "conditional"... However, as I do not envision pgbench
growing to a full language, I would be fine keeping "if" as it is.

The table should really have a row per operator, which is what we do in
pretty much all of the core documention. [...]

Ok for one line per operator.

The question which was brought up about having a line-continuation
(eg: '\') character would be useful,

:-) I submitted a patch for that, which got rejected and resulted in Tom
making pgbench share psql lexer. This is a good thing, although the
continuation extension was lost in the process. Also this means that now
such logic would probably be shared with psql, not necessarily a bad thing
either, but clearly material for another patch.

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.

I do not think that pgbench script is a good target to define functions,
because the script is implicitely in a very large loop which is executing
it over and over again. I do not think that it would make much sense to
redefine functions on each transaction... So my opinion is that without a
compeling use case, I would avoid such a feature, which would need some
careful thinking on the design side, and the implementation of which is
non trivial.

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.

If you want control, you can already use a DO & PL/pgsql script, although
it is interpreted server-side. Yet again, I'm not convince that pgbench is
material for such features, and it would take a compeling use case for me
to add such a thing. Moreover, the currently simple internal data
structures and executor would have to be profoundly reworked and extended
to handle a full language and functions.

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.

I agree that pgbench should be tested systematically. I think that this is
not limited to these functions and operators but a more general and
desirable feature, thus is really material for another patch.

Attached version changes:
- removes C operators not present in psql
- document operators one per line

--
Fabien.

On Sat, Oct 1, 2016 at 11:35 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Attached version changes:
- removes C operators not present in psql
- document operators one per line

Moved to next CF with same status: "Ready for committer".
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers