Thread: pgbench more operators & functions

Hello,

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.

Some kind of continuations in \ commands would be a good thing.

-- 
Fabien.

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.

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

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.

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.

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

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

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

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.

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

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

Fabien,

* Fabien COELHO (coelho@cri.ensmp.fr) wrote:
> >>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'm pretty sure we should hold off on adding 'xor' until it's actually
in PG proper, otherwise we run a very serious risk that whatever we do
to add it in PG will be different from what you're suggesting we do here
for pgbench.

> >[...] I certainly understand how the if() function could be useful
>
> Indeed, some kind of "if" is needed, for instance to implement
> "tpc-b" correctly.

That's an interesting point..  Have you thought about ripping out the
built-in TPC-B-like functionality of pgbench and making that into a
script instead?  Doing so would likely point out places where we need to
add functionality or cases which aren't handled very well.  We'd also be
able to rip out all that code from pgbench and make it into a general
utility instead of "general utility + other stuff."

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

I'm not quite sure that I follow why you feel that CASE would be too
difficult to implement here..?

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

Excel would be one of the ugly if-nesting cases that I was thinking
of, actually.  I'm certainly not thrilled with the idea of modelling
anything off of it.

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

Thanks!

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

Sure, that makes sense to do independently.

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

If we're going to replace the built-in TPC-B functionality then we're
going to need a way to pass in an 'init' script of some kind which only
gets run once, that could certainly be where functions could be
defined.  As for the use-case for functions, well, we need an if()
construct, as discussed, and having a need for loops doesn't seem too
far off from needing conditional control structures.

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

Doing it server-side would certainly not be the same as having it
client-side.  Perhaps it'd be difficult to rework pgbench to support it,
but I do think it's something we should at least be considering and
making sure we don't wall ourselves off from implementing in the future.
Most scripting languages do support functions of one form or another.

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

I don't agree with this- at a minimum, this patch should add regression
tests for the features that it's adding.  Other tests should be added
but we do not need to wait for some full test suite to be dropped into
pgbench before adding any regression tests.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Fabien COELHO (coelho@cri.ensmp.fr) wrote:
>> In the attached patched I only included pg operators, plus "xor"
>> which I feel is missing and does not seem to harm.

> I'm pretty sure we should hold off on adding 'xor' until it's actually
> in PG proper, otherwise we run a very serious risk that whatever we do
> to add it in PG will be different from what you're suggesting we do here
> for pgbench.

Agreed --- we don't really want stuff in pgbench's language that's not
in regular SQL, IMO.

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

> That's an interesting point..  Have you thought about ripping out the
> built-in TPC-B-like functionality of pgbench and making that into a
> script instead?

It already is a script, it's just hardwired as a string constant in
pgbench.c rather than being a separate file.  I think Fabien is
suggesting that it could be changed to more nearly approximate the
actual TPC-B spec, but IMO that would be a seriously bad idea because
it would invalidate all cross-version performance comparisons.  We
decided years ago that the default script is what it is and we aren't
going to change it to try to match TPC-B more exactly.

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

> I'm not quite sure that I follow why you feel that CASE would be too
> difficult to implement here..?

If you want simple, you could provide just a subset of CASE (ie, only
the CASE WHEN boolexpr variant).  I think inventing some random new syntax
is a bad idea.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Fabien COELHO (coelho@cri.ensmp.fr) wrote:
> >> Indeed, some kind of "if" is needed, for instance to implement
> >> "tpc-b" correctly.
>
> > That's an interesting point..  Have you thought about ripping out the
> > built-in TPC-B-like functionality of pgbench and making that into a
> > script instead?
>
> It already is a script, it's just hardwired as a string constant in
> pgbench.c rather than being a separate file.  I think Fabien is
> suggesting that it could be changed to more nearly approximate the
> actual TPC-B spec, but IMO that would be a seriously bad idea because
> it would invalidate all cross-version performance comparisons.  We
> decided years ago that the default script is what it is and we aren't
> going to change it to try to match TPC-B more exactly.

If we could replicate what the hardwired script does in an external
script, keeping that as the default, and then provide a 'Closer to
TPC-B' script, then I'm all for that.  If the existing "hardwired
script" is really just a string constant, then we shouldn't need to
worry about that invalidating prior runs.

> >> 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.
>
> > I'm not quite sure that I follow why you feel that CASE would be too
> > difficult to implement here..?
>
> If you want simple, you could provide just a subset of CASE (ie, only
> the CASE WHEN boolexpr variant).  I think inventing some random new syntax
> is a bad idea.

Agreed.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> It already is a script, it's just hardwired as a string constant in
>> pgbench.c rather than being a separate file.  I think Fabien is
>> suggesting that it could be changed to more nearly approximate the
>> actual TPC-B spec, but IMO that would be a seriously bad idea because
>> it would invalidate all cross-version performance comparisons.  We
>> decided years ago that the default script is what it is and we aren't
>> going to change it to try to match TPC-B more exactly.

> If we could replicate what the hardwired script does in an external
> script, keeping that as the default, and then provide a 'Closer to
> TPC-B' script, then I'm all for that.

I've got no objection to a more-nearly-TPC-B script as an option.
But why do you feel the need to pull the default script out into
a separate file?  Seems to me that just adds maintenance complexity,
and the need for pgbench to have a notion of a library directory,
for little gain.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> It already is a script, it's just hardwired as a string constant in
> >> pgbench.c rather than being a separate file.  I think Fabien is
> >> suggesting that it could be changed to more nearly approximate the
> >> actual TPC-B spec, but IMO that would be a seriously bad idea because
> >> it would invalidate all cross-version performance comparisons.  We
> >> decided years ago that the default script is what it is and we aren't
> >> going to change it to try to match TPC-B more exactly.
>
> > If we could replicate what the hardwired script does in an external
> > script, keeping that as the default, and then provide a 'Closer to
> > TPC-B' script, then I'm all for that.
>
> I've got no objection to a more-nearly-TPC-B script as an option.
> But why do you feel the need to pull the default script out into
> a separate file?  Seems to me that just adds maintenance complexity,
> and the need for pgbench to have a notion of a library directory,
> for little gain.

Part of it is a feeling that we should really be 'eating our own
dogfood' when it comes to pgbench, but also that it seems to add
unnecessary C-level code to an otherwise general-purpose utility
for no particular reason except "that's how it was first written."

Perhaps that's overkill for this case and you have an interesting point
that it might require additional code in pgbench (though I'm not
completely convinced of that...), so I won't push too hard on it, but I
still think it'd be "better" to have pgbench just be the general purpose
utility and not also have some built-in thing, even if it's obvious that
it could just be a script.

Thanks!

Stephen

On Sun, Oct 2, 2016 at 9:41 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> 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".

I think it's pretty clear that this patch is not Ready for Committer,
because there's no consensus that we want this, and like Tom and
Stephen, I'd argue that there are large parts of it we don't want.
The documentation in the latest patch version mentions XOR and IF
which we definitely don't want because there is no similar thing in
SQL, but in addition to that, I don't think much of an argument has
been made that any of this is actually useful.  I'm skeptical about
the notion that giving pgbench a vast repertoire of mathematical
functions is a good idea.  What does that actually let us do that is
useful and not possible today?

I'm happy to see pgbench made better in a variety of ways, but I don't
really see why that particular thing is useful.  Perhaps I'm just
missing something.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello Stephen,

> I'm pretty sure we should hold off on adding 'xor' [...]

So I removed "xor" in the attached version.

>>> [...] I certainly understand how the if() function could be useful
>>
>> Indeed, some kind of "if" is needed, for instance to implement
>> "tpc-b" correctly.
>
> That's an interesting point..  Have you thought about ripping out the
> built-in TPC-B-like functionality of pgbench and making that into a
> script instead? Doing so would likely point out places where we need to
> add functionality or cases which aren't handled very well. [...]

My point is not that pgbench should do tcp-b-real by default, but that it
should be possible to do it, and currently this is not possible. People
rely on what pgbench is doing so it should not be (significantly) changed,
at least in its default behavior.

> I'm not quite sure that I follow why you feel that CASE would be too
> difficult to implement here..?

Obviously, everything is possible, it just takes more time. I've added the
generic case expression syntax in the attached version.

> [...] If we're going to replace the built-in TPC-B functionality

As stated above, I do *NOT* want to replace the existing functionality, I
just want to make pgbench able to do more in a sensible & useful way for
benchmarking.

I'm not currently planing to add user functions and control structures
outside of expressions in pgbench without a very strong user case.

> [...] Perhaps it'd be difficult to rework pgbench to support it, but I
> do think it's something we should at least be considering and making
> sure we don't wall ourselves off from implementing in the future. Most
> scripting languages do support functions of one form or another.

Sure.

>>> 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.
>
> I don't agree with this- at a minimum, this patch should add regression
> tests for the features that it's adding.

This has not been the case with the previous dozens of patches about
pgbench, but maybe it should start somewhere:-).

There was already one TAP script for pgbench (for testing concurrent oid
insertion, not really a pgbench functionnality), I added another one which
focusses on expressions.

Changes in v6:
  - remove xor operator
  - remove if function
  - make keywords case insensitive (more like SQL)
  - add generic case expression syntax
    (note that the typing is the same strange one as pg, i.e.
     CASE WHEN (RANDOM() > 0.5) THEN 1 ELSE 1.0 END;
     can be either an int or a double, depending, this is not statically typed...
  - add TAP test about pgbench expressions

--
Fabien.

Hello Robert,

> I think it's pretty clear that this patch is not Ready for Committer,

As a reviewer, I do not know when to decide to put something as "ready". 
My opinion is that it is a matter of the reviewer to decide. Whether some 
consensus is actually reached, or whether a committer is going to disagree 
later on, cannot be helped.

> because there's no consensus that we want this, and like Tom and
> Stephen, I'd argue that there are large parts of it we don't want.

> The documentation in the latest patch version mentions XOR and IF
> which we definitely don't want because there is no similar thing in
> SQL,

I have removed these and put CASE WHEN THEN ELSE END instead in v6.

> but in addition to that, I don't think much of an argument has
> been made that any of this is actually useful.

In the TPC-B benchmark, some conditional is needed because under some 
probability an account must be chosen in the *same* branch as the teller, 
otherwise in the *other* branches.

> I'm skeptical about the notion that giving pgbench a vast repertoire of 
> mathematical functions is a good idea.  What does that actually let us 
> do that is useful and not possible today?

I do not see a vast "repertoire" of functions. There are "usual" int 
operators, logical operators, and a few functions.

About the one added in this patch:

bitwise operations: I have seen some use to create a non uniform random 
from a random one. Once one operator is put in, there is no reason not to 
put the others...

exp & ln: could be used to tweak distributions.

conditional: see above.

I have not put trigonometric functions because I could not think of a 
use in a benchmarking context.

> I'm happy to see pgbench made better in a variety of ways, but I don't
> really see why that particular thing is useful.  Perhaps I'm just
> missing something.

I'm trying to add features that are IMO useful for benchmarking.

When doing so, someone says "hay, you put a double expression, you must 
put double variables". Although I can see the point of double expressions 
for passing ints into some transformations, I can't see a double variable 
really useful in any benchmark, but there it is, it is a side effect of 
the process, and it is somehow to have orthogonal features.

-- 
Fabien.

> I've got no objection to a more-nearly-TPC-B script as an option.

Good, because adding a "per-spec" tpc-b as an additional builtin option is 
one of my intentions, once pgbench is capable of it.

> But why do you feel the need to pull the default script out into
> a separate file?  Seems to me that just adds maintenance complexity,
> and the need for pgbench to have a notion of a library directory,
> for little gain.

I tend to agree on this point. Now it could be possible to make pgbench 
look for "builtin" scripts in a predefined location so that they are found 
easilly, but I'm not sure there would be a significant added value wrt the 
current status.

-- 
Fabien.

Fabien,

* Fabien COELHO (coelho@cri.ensmp.fr) wrote:
> >I've got no objection to a more-nearly-TPC-B script as an option.
>
> Good, because adding a "per-spec" tpc-b as an additional builtin
> option is one of my intentions, once pgbench is capable of it.

I believe it would be really helpful to have the more-nearly-TPC-B
script written using these new capabilities of pgbench to see that
a) the new capabilities actually allow for this, b) there aren't other
things which are needed, c) to provide an actual use-case for these new
capabilities.

Thanks!

Stephen

Hello Stephen,

> * Fabien COELHO (coelho@cri.ensmp.fr) wrote:
>>> I've got no objection to a more-nearly-TPC-B script as an option.
>>
>> Good, because adding a "per-spec" tpc-b as an additional builtin
>> option is one of my intentions, once pgbench is capable of it.
>
> I believe it would be really helpful to have the more-nearly-TPC-B
> script written using these new capabilities of pgbench to see that
> a) the new capabilities actually allow for this, b) there aren't other
> things which are needed, c) to provide an actual use-case for these new
> capabilities.

Here are the details:

(1) The required schema is slightly different : currently the type used 
for holding balances is not wide enough per the TCP-B standard, this mean 
maybe having an option to do "pgbench -i --standard-tpcb" which would 
generate the right schema, probably it should just change a few INTEGER to 
INT8, or maybe use NUMERIC(10). I have not done such a patch yet.


(2) The benchmark specification requires the client application to get 
hold of query results, which are currently discarded by pgbench, so 
pgbench does not really comply. I have submitted a patch to do that, see:
  https://commitfest.postgresql.org/11/669/


(3) The expression lines, especially with a CASE syntax, are quite long, 
allowing continuations would be nice, I have submitted a patch to do so:
  https://commitfest.postgresql.org/11/807/


(4) As stated above, conditions are needed. Given the above extensions, 
the following script would work and comply in 2 round trips and uses two
tests and two integer comparisons, added by the patch under discussion.
It also needs to get hold of two results (the branch teller and the final 
balance).
 -- choose teller id \set tid random(1, 10 * :scale) -- get an account branch, used if not the same as teller \set abid
random(1;:scale - 1) -- get an account in-branch number \set laid random(1, 100000) -- select amount \set delta
random(-999999,+999999) -- let us now start the stuff BEGIN \; -- get the teller's branch SELECT bid \into tbid   FROM
pgbench_tellersWHERE tid = :tid ; -- if random < 0.85 account is in teller's branch, else in a *different* branch \set
bidCASE \            WHEN random(0, 99) < 85 THEN :tbid \            ELSE :abid + (:abid < :tbid) \          END \set
aid:laid + 100000 * :bid -- now move the money and return the final balance UPDATE pgbench_accounts SET abalance =
abalance+ :delta WHERE aid = :aid \; -- Maybe it is better to use "RETURNING aid" in the previous UPDATE? SELECT
abalance\into abalance   FROM pgbench_accounts WHERE aid = :aid \; UPDATE pgbench_tellers SET tbalance = tbalance +
:deltaWHERE tid = :tid \; UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid \; INSERT INTO
pgbench_history(tid, bid, aid, delta, mtime)   VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP) \; END;
 


(5) The above "composite" queries (\;) do not work with -M prepared, which 
means (a) either doing independent queries and getting a lot of added 
latency, or better (b) make -M prepared work with composite queries, which 
I have not done yet.


Basically the 3 patches under submission allow to write the above working 
TPC-B script, but more patches are needed for the schema to comply and for 
-M prepared to work as well. I would prefer to wait for all pieces to be 
there before adding an example script. I do not think that one large patch 
mixing everything makes much sense from an engineering point of view, even 
if it makes sense from a feature point of view.

-- 
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> [ re TPC-B ]

> (1) The required schema is slightly different : currently the type used 
> for holding balances is not wide enough per the TCP-B standard, this mean 
> maybe having an option to do "pgbench -i --standard-tpcb" which would 
> generate the right schema, probably it should just change a few INTEGER to 
> INT8, or maybe use NUMERIC(10). I have not done such a patch yet.

The whole question of the database setup is an interesting one.  If we
were to do anything at all here, I'd want to see not only the table
schemas and initial population, but also the hard-wired "vacuum" logic,
somehow made not so hard-wired.  I have no good ideas about that.  The
SQL commands could possibly be taken from scripts, but what of all the
work that's gone into friendly progress reporting for table loading?

> (2) The benchmark specification requires the client application to get 
> hold of query results, which are currently discarded by pgbench, so 
> pgbench does not really comply. I have submitted a patch to do that, see:

I find this completely bogus.  The data is delivered to the client,
ie it's inside the pgbench process.  What's the grounds for arguing
that something else has to happen to it?
        regards, tom lane

Hello Tom,

>> (2) The benchmark specification requires the client application to get
>> hold of query results, which are currently discarded by pgbench, so
>> pgbench does not really comply. I have submitted a patch to do that, see:
>
> I find this completely bogus.  The data is delivered to the client,
> ie it's inside the pgbench process.

Hmmm... It is delivered to libpq, and the client discards it... In a 
benchmarking context I think that this is not exactly the same:

For instance, one could implement a library protocol that would tell that 
the result is ready without actually transfering the result, getting a 
slight benefit by not doing so.

In order to avoid that kind of doubtful optimization, the benchmark 
requires that the final balance is returned to the "test driver", which is 
the client application, and not some subsystem linked to the database. I 
think that this is a pretty sensible requirement.

Moreover, the teller's branch must be used in some case, not sure how to 
do that without getting this value out anyway...

> What's the grounds for arguing that something else has to happen to it?

In my view the "ground" is the benchmarking specification which wants to 
ensure that the tester/implementers/vendors cannot cheat to get better 
than deserved performance results...

-- 
Fabien.

Hello Tom,

I comment here on the first part of your remarks. I've answered the second 
part in another mail.

>> (1) The required schema is slightly different : currently the type used
>> for holding balances is not wide enough per the TCP-B standard, this mean
>> maybe having an option to do "pgbench -i --standard-tpcb" which would
>> generate the right schema, probably it should just change a few INTEGER to
>> INT8, or maybe use NUMERIC(10). I have not done such a patch yet.
>
> The whole question of the database setup is an interesting one.
> If we were to do anything at all here, I'd want to see not only the 
> table schemas and initial population, but also the hard-wired "vacuum" 
> logic, somehow made not so hard-wired.  I have no good ideas about that. 
> The SQL commands could possibly be taken from scripts, but what of all 
> the work that's gone into friendly progress reporting for table loading?

I'm unconvince by the current status, especially the default behaviors. I 
think it should do a good sensible representative job, and not be a 
minimum installation.

For instance, the default setup does not use foreign keys. It should be 
the reverse, foreign keys should be included by default and an option 
should be used to lessen the schema quality.

Also, given the heavy UPDATE nature of the pgbench test, a non 100% 
default fill factor on some tables would make sense.

The "friendly progress reporting" only applies to the initial insert: the 
vacuum, primary key and possibly foreign key alterations also take a 
significant time but are not included in the progress report. On the one 
hand that makes sense because pgbench has no clue about the progression of 
these tasks, but on the other hand it means that the "friendly" stops 
halfway in the setup. The default progress reporting is much too verbose 
on any modern hardware, the quiet mode should be the default, or even the 
only option.

Note that I'm not really planing to change any of this because it would 
probably be rejected as it is a significant behavioral change, but I find 
it annoying anyway.

-- 
Fabien

On Sat, Oct 8, 2016 at 12:58 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>
> Hello Tom,
>
> I comment here on the first part of your remarks. I've answered the second
> part in another mail.
>
>>> (1) The required schema is slightly different : currently the type used
>>> for holding balances is not wide enough per the TCP-B standard, this mean
>>> maybe having an option to do "pgbench -i --standard-tpcb" which would
>>> generate the right schema, probably it should just change a few INTEGER
>>> to
>>> INT8, or maybe use NUMERIC(10). I have not done such a patch yet.
>>
>>
>> The whole question of the database setup is an interesting one.
>> If we were to do anything at all here, I'd want to see not only the table
>> schemas and initial population, but also the hard-wired "vacuum" logic,
>> somehow made not so hard-wired.  I have no good ideas about that. The SQL
>> commands could possibly be taken from scripts, but what of all the work
>> that's gone into friendly progress reporting for table loading?
>
>
> I'm unconvince by the current status, especially the default behaviors. I
> think it should do a good sensible representative job, and not be a minimum
> installation.
>
> For instance, the default setup does not use foreign keys. It should be the
> reverse, foreign keys should be included by default and an option should be
> used to lessen the schema quality.
>
> Also, given the heavy UPDATE nature of the pgbench test, a non 100% default
> fill factor on some tables would make sense.
>

FWIW, sometime back I have seen that with fill factor 80, at somewhat
moderate client counts (32) on 192 - Hyper Threaded m/c, the
performance is 20~30% better, but at higher client counts, it was same
as 100 fill factor.  I think if go by your theory, one could also
argue to have non-default values autovacuum threshold parameters.
pgbench already has a parameter to specify non-default fillfactor and
I think that is sufficient for anyone to do performance testing.

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

Hello Amit.

>> Also, given the heavy UPDATE nature of the pgbench test, a non 100% default
>> fill factor on some tables would make sense.
>
> FWIW, sometime back I have seen that with fill factor 80, at somewhat
> moderate client counts (32) on 192 - Hyper Threaded m/c, the
> performance is 20~30% better, but at higher client counts, it was same
> as 100 fill factor.

The 20-30% figure is consistent with figures I collected 2 years ago about 
fill factor on HDD, see the beginning run of:

http://blog.coelho.net/database/2014/08/23/postgresql-fillfactor-and-update.html

Although I found that the advantages is reduced after some time because 
once a page has got an update it has some free space which can be taken 
advantage of later on, if the space was not reclaimed by vacuum.

I cannot understand why there would be no advantage with more clients, 
though...

Alas, performance testing is quite sensitive to many details:-(

-- 
Fabien.

Hello Haribabu,

>> Alas, performance testing is quite sensitive to many details:-(

> The current status of the patch and recent mail thread discussion doesn't
> represent the same.

The same what?

The discussion was about a particular test in a particular setting for a 
particular load, the fact that reducing the latency has a limited effect 
in that case is a fact in life. I have produced other settings where the 
effect was very important. The patch has no down side AFAICS.

> Closed in 2016-11 commitfest with "returned with feedback" status.
> Please feel free to update the status once you submit the updated patch.

Given the thread discussions, I do not understand why this "ready for 
committer" patch is switched to "return with feedback", as there is 
nothing actionnable, and I've done everything required to improve the 
syntax and implementation, and to justify why these functions are useful.

I'm spending time to try to make something useful of pgbench, which 
require a bunch of patches that work together to improve it for new use 
case, including not being limited to the current set of operators.

This decision is both illogical and arbitrary.

-- 
Fabien.

Hello,

> Sorry for the changing the status of the patch against to the current 
> status. While going through the recent mails, I thought that there is 
> some disagreement from committer.

If so, I'm willing to explain again why these operators are useful for 
writing some benchmarks, for instance, this paragraph taken randomly from 
the TPC-B specification, on page 16:

"""  The Account_ID is generated as follows:  • A random number X is generated within [0,1]  • If X<0.85 or branches =
1,a random Account_ID is selected over all <Branch_ID> accounts.  • If X>=0.85 and branches > 1, a random Account_ID is
selectedover all non-<Branch_ID> accounts.
 
"""

This extracy suggests clearly that having some comparison operators and 
some ability to act upon the comparison result is required to implement 
this particular benchmark, which is beyond pgbench current capabilities.

> Moved to next CF with "ready for committer" status.

Ok. We'll see next time what becomes of it...

-- 
Fabien.

Rebased version after small expression scanner change.

-- 
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 Fri, Dec 2, 2016 at 1:28 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> Closed in 2016-11 commitfest with "returned with feedback" status.
>> Please feel free to update the status once you submit the updated patch.
>
> Given the thread discussions, I do not understand why this "ready for
> committer" patch is switched to "return with feedback", as there is nothing
> actionnable, and I've done everything required to improve the syntax and
> implementation, and to justify why these functions are useful.
>
> I'm spending time to try to make something useful of pgbench, which require
> a bunch of patches that work together to improve it for new use case,
> including not being limited to the current set of operators.
>
> This decision is both illogical and arbitrary.

I disagree.  I think his decision was probably based on this email from me:


-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 24, 2017 at 11:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Dec 2, 2016 at 1:28 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>>> Closed in 2016-11 commitfest with "returned with feedback" status.
>>> Please feel free to update the status once you submit the updated patch.
>>
>> Given the thread discussions, I do not understand why this "ready for
>> committer" patch is switched to "return with feedback", as there is nothing
>> actionnable, and I've done everything required to improve the syntax and
>> implementation, and to justify why these functions are useful.
>>
>> I'm spending time to try to make something useful of pgbench, which require
>> a bunch of patches that work together to improve it for new use case,
>> including not being limited to the current set of operators.
>>
>> This decision is both illogical and arbitrary.
>
> I disagree.  I think his decision was probably based on this email from me:

(argh, sent too soon)

https://www.postgresql.org/message-id/CA+Tgmoa0zp4A+S+KosaV4QfDz-wA56vLpH8me86rmpsnkvWc2w@mail.gmail.com

Nobody responded to that, and I have not seen any committer say that
they are in favor of this.  Against that, three committers (Tom,
Stephen, me) have taken the view that they are opposed to at least
some parts of it.  No changes to the patch have resulted from those
complaints.  So this is just submitting the same thing over and over
again and hoping that the fourth or fifth committer who looks at this
is going to have a different opinion than the first three, or fail to
notice the previous discussion.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Dec 2, 2016 at 1:28 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>>> This decision is both illogical and arbitrary.

>> I disagree.  I think his decision was probably based on this email from me:

> (argh, sent too soon)
> https://www.postgresql.org/message-id/CA+Tgmoa0zp4A+S+KosaV4QfDz-wA56vLpH8me86rmpsnkvWc2w@mail.gmail.com

> Nobody responded to that, and I have not seen any committer say that
> they are in favor of this.  Against that, three committers (Tom,
> Stephen, me) have taken the view that they are opposed to at least
> some parts of it.  No changes to the patch have resulted from those
> complaints.  So this is just submitting the same thing over and over
> again and hoping that the fourth or fifth committer who looks at this
> is going to have a different opinion than the first three, or fail to
> notice the previous discussion.

I concur that this is expanding pgbench's expression language well beyond
what anybody has shown a need for.  I'm also concerned that there's an
opportunity cost here, in that the patch establishes a precedence ordering
for its new operators, which we'd have a hard time changing later.  That's
significant because the proposed precedence is different from what you'd
get for similarly-named operators on the backend side.  I realize that
it's trying to follow the C standard instead, but do we really want to
open that can of worms right now?  That's a decision I'd much rather put
off if we need not make it now.

I'd be okay with the parts of this that duplicate existing backend syntax
and semantics, but I don't especially want to invent further than that.
        regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> Maybe consider writing a full patch series that culminates with this
> additional builtin option being added as the final patch - breaking out
> this (and probably other) "requirements" patches separately to aid in
> review.  The presentation of just "add new stuff that seems useful" opens
> too much room for isolated discussion without knowing what the big picture
> requires.  At worse you'll at least have a working version of a forked
> pgbench that can do what you want.

> As it stands right now you haven't provided enough context for this patch
> and only the social difficulty of actually marking a patch rejected has
> prevented its demise in its current form - because while it has interesting
> ideas its added maintenance burden for -core without any in-core usage.
> But it you make it the first patch in a 3-patch series that implements the
> per-spec tpc-b the discussion moves away from these support functions and
> into the broader framework in which they are made useful.

I think Fabien already did post something of the sort, or at least
discussion towards it, and there was immediately objection as to whether
his idea of TPC-B compliance was actually right.  I remember complaining
that he had a totally artificial idea of what "fetching a data value"
requires.  So a full patch series would be a good idea in that we could
discuss whether the requirements are correct before we get into the nitty
gritty of implementing them.
        regards, tom lane

On Tue, Jan 24, 2017 at 12:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'd be okay with the parts of this that duplicate existing backend syntax
> and semantics, but I don't especially want to invent further than that.

I agree, and I think that's pretty much what we all said back in
October, and the patch hasn't been revised since then to match those
comments.  Perhaps I'm in a grumpy mood today, but I've got enough
patches to review from people who are willing to revise their patches
in response to feedback to spend very much time on patches from people
who aren't.  I realize that it can be frustrating to have to defer to
a committer when it's a 1-1 tie between you and the person with git
access - is that really a consensus?  But in this case, 3 separate
committers weighed in on this thread to very politely give essentially
the same feedback and that is certainly a consensus.  Not only was the
patch not revised, but the very idea that the patch might need to be
revised before further consideration was greeted with indignation.

Let's mark this Returned with Feedback and move on.  We've only got a
week left in the CommitFest anyhow and there are lots of other things
that still need work (and which actually have been revised to match
previous feedback).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jan 25, 2017 at 6:38 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> Let's mark this Returned with Feedback and move on.  We've only got a
> week left in the CommitFest anyhow and there are lots of other things
> that still need work (and which actually have been revised to match
> previous feedback).

Done as such, let's move on.
-- 
Michael

>>> I'm spending time to try to make something useful of pgbench, which 
>>> require a bunch of patches that work together to improve it for new 
>>> use case, including not being limited to the current set of operators.
>>>
>>> This decision is both illogical and arbitrary.
>>
>> I disagree.  I think his decision was probably based on this email from me:
>
> https://www.postgresql.org/message-id/CA+Tgmoa0zp4A+S+KosaV4QfDz-wA56vLpH8me86rmpsnkvWc2w@mail.gmail.com

> Nobody responded to that,

The answer is on the same day a direct reply that you can check here:

https://www.postgresql.org/message-id/alpine.DEB.2.20.1610041941150.24533%40lancre

The short version is: I have removed XOR and replaced "if" with the SQL 
CASE syntax, and provided justification for the added operators in a 
benchmarking context, i.e. some kind of test is necessary for TPC-B 2.0.0. 
For conditions, logical expressions are needed. Bitwise operators are used 
to skew distribution in some benchmarks (TPC-C as I recall). Functions 
ln/exp could be used for the same purpose, but I can remove those two if 
this is a blocker.

-- 
Fabien.

Hello Tom,

> I concur that this is expanding pgbench's expression language well beyond
> what anybody has shown a need for.

As for the motivation, I'm assuming that pgbench should provide features 
necessary to implement benchmarks, so I'm adding operators that appear in 
standard benchmark specifications.

From TPC-B 2.0.0 section 5.3.5:

  | The Account_ID is generated as follows:
  | • A random number X is generated within [0,1]
  | • If X<0.85 or branches = 1, a random Account_ID is selected over all
  |   <Branch_ID> accounts.
  | • If X>=0.85 and branches > 1, a random Account_ID is selected over all
  |   non-<Branch_ID> accounts.

The above uses a condition (If), logic (or, and), comparisons (=, <, >=).

From TPC-C 5.11 section 2.1.6, a bitwise-or operator is used to skew a 
distribution:

  | NURand (A, x, y) = (((random (0, A) | random (x, y)) + C) % (y - x + 1)) + x

And from section 5.2.5.4 of same, some time is computed based on a 
logarithm:

  | Tt = -log(r) * µ

> I'm also concerned that there's an opportunity cost here, in that the 
> patch establishes a precedence ordering for its new operators, which 
> we'd have a hard time changing later. That's significant because the 
> proposed precedence is different from what you'd get for similarly-named 
> operators on the backend side. I realize that it's trying to follow the 
> C standard instead,

Oops. I just looked at the precedence from a C parser, without realizing 
that precedence there was different from postgres SQL implementation:-( 
This is a bug on my part.

> I'd be okay with the parts of this that duplicate existing backend syntax
> and semantics, but I don't especially want to invent further than that.

Okay. In the two latest versions sent, discrepancies from that were bugs, 
I was trying to conform.

Version 8 attached hopefully fixes the precedence issue raised above:

  - use precedence taken from "backend/gram.y" instead of C. I'm not sure
    that it is wise that pg has C-like operators with a different
    precedence, but this is probably much too late...

And fixes the documentation:

  - remove a non existing anymore "if" function documentation which made
    Robert assume that I had not taken the hint to remove it. I had!

  - reorder operator documentation by their pg SQL precedence.

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

> I agree, and I think that's pretty much what we all said back in
> October, and the patch hasn't been revised since then to match those
> comments.

Hmmm. It really had been revised, although I forgot to remove the "if" doc 
but I had remove the if function.

-- 
Fabien.

>> As it stands right now you haven't provided enough context for this patch
>> and only the social difficulty of actually marking a patch rejected has
>> prevented its demise in its current form - because while it has interesting
>> ideas its added maintenance burden for -core without any in-core usage.
>> But it you make it the first patch in a 3-patch series that implements the
>> per-spec tpc-b the discussion moves away from these support functions and
>> into the broader framework in which they are made useful.
>
> I think Fabien already did post something of the sort, or at least 
> discussion towards it,

Yep.

> and there was immediately objection as to whether his idea of TPC-B 
> compliance was actually right. I remember complaining that he had a 
> totally artificial idea of what "fetching a data value" requires.

Yep.

I think that the key misunderstanding is that you are honest and assume 
that other people are honest too. This is naïve: There is a long history 
of vendors creatively "cheating" to get better than deserve benchmark 
results. Benchmark specifications try to prevent such behaviors by laying 
careful requirements and procedures.

In this instance, you "know" that when pg has returned the result of the 
query the data is actually on the client side, so you considered it is 
fetched. That is fine for you, but from a benchmarking perspective with 
external auditors your belief is not good enough.

For instance, the vendor could implement a new version of the protocol 
where the data are only transfered on demand, and the result just tells 
that the data is indeed somewhere on the server (eg on "SELECT abalance" 
it could just check that the key exists, no need to actually fetch the 
data from the table, so no need to read the table, the index is 
enough...). That would be pretty stupid for real application performance, 
but the benchmark would could get better tps by doing so.

Without even intentionnaly cheating, this could be part of a useful 
"streaming mode" protocol option which make sense for very large results 
but would be activated for a small result.

Another point is that decoding the message may be a little expensive, so 
that by not actually extracting the data into the client but just keeping 
it in the connection/OS one gets better performance.

Thus, TPC-B 2.0.0 benchmark specification says:

"1.3.2 Each transaction shall return to the driver the Account_Balance 
resulting from successful commit of the transaction.

Comment: It is the intent of this clause that the account balance in the 
database be returned to the driver, i.e., that the application retrieve 
the account balance."

For me the correct interpretation of "the APPLICATION retrieve the account 
balance" is that the client application code, pgbench in this context, did 
indeed get the value from the vendor code, here "libpq" which is handling 
the connection.

Having the value discarded from libpq by calling PQclear instead of 
PQntuples/PQgetvalue/... skips a key part of the client code that no real 
application would skip. This looks strange and is not representative of 
real client code: as a potential auditor, because of this I would not 
check the corresponding item in the audit check list:
 "11.3.1.2 Verify that transaction inputs and outputs satisfy Clause 1.3."

So the benchmark implementation would not be validated.


Another trivial reason to be able to actually retrieve data is that for 
benchmarking purpose it is very easy to want to test a scenario where you 
want to do different things based on data received, which imply that the 
data can be manipulated somehow on the benchmarking client side, which is 
currently not possible.

-- 
Fabien.

Bonjour Michaël, Hello Robert,

>> Let's mark this Returned with Feedback and move on.  We've only got a
>> week left in the CommitFest anyhow and there are lots of other things
>> that still need work (and which actually have been revised to match
>> previous feedback).
>
> Done as such, let's move on.

Hmmm.

I think that there is a misunderstanding, most of which being my fault.

I have really tried to do everything that was required from committers, 
including revising the patch to match all previous feedback.

Version 6 sent on Oct 4 did include all fixes required at the time (no if, 
no unusual and operators, TAP tests)... However I forgot to remove some 
documentation about the removed stuff, which made Robert think that I had 
not done it. I apologise for this mistake and the subsequent 
misunderstanding:-(

The current v8 sent on Jan 25 should only implement existing server-side 
stuff, including with the same precedence as pointed out by Tom.

So for the implementation side I really think that I have done exactly all 
that was required of me by committers, although sometimes with bugs or 
errors, my apology, again...


As for the motivation, which is another argument, I cannot do more than 
point to actual published official benchmark specifications that do 
require these functions. I'm not inventing anything or providing some 
useless catalog of math functions.

If pgbench is about being seated on a bench and running postgres on your 
laptop to get some heat, my mistake... I thought it was about 
benchmarking, which does imply a few extra capabities.

If the overall feedback is to be undestood as "the postgres community does 
not think that pgbench should be able to be used to implement benchmarks 
such as TPC-B", then obviously I will stop efforts to improve it for that 
purpose.


To conclude:

IMHO the relevant current status of the patch should be "Needs review" and 
possibly "Move to next CF".

If the feedback is "we do not want pgbench to implement benchmarks such as 
TPC-B", then indeed the proposed features are not needed and the status 
should be "Rejected".

In any case, "Returned with feedback" does not really apply.

A+

-- 
Fabien.

Fabien,

* Fabien COELHO (coelho@cri.ensmp.fr) wrote:
> I think that there is a misunderstanding, most of which being my fault.

No worries, it happens. :)

> I have really tried to do everything that was required from
> committers, including revising the patch to match all previous
> feedback.

Thanks for continuing to try to work through everything.  I know it can
be a difficult process, but it's all towards a (hopefully) improved and
better PG.

> Version 6 sent on Oct 4 did include all fixes required at the time
> (no if, no unusual and operators, TAP tests)... However I forgot to
> remove some documentation about the removed stuff, which made Robert
> think that I had not done it. I apologise for this mistake and the
> subsequent misunderstanding:-(

Ok, that helps clarify things.  As does the rest of your email, for me,
anyway.

> If pgbench is about being seated on a bench and running postgres on
> your laptop to get some heat, my mistake... I thought it was about
> benchmarking, which does imply a few extra capabities.

I believe we do want to improve pgbench and your changes are generally
welcome when it comes to adding useful capabilities.  Your explanation
was also helpful about the specific requirements.

> IMHO the relevant current status of the patch should be "Needs
> review" and possibly "Move to next CF".

For my 2c, at least, while I'm definitely interested in this, it's not
nearly high enough on my plate with everything else going on to get any
attention in the next few weeks, at least.

I do think that, perhaps, this patch may deserve a bit of a break, to
allow people to come back to it with a fresh perspective, so perhaps
moving it to the next commitfest would be a good idea, in a Needs Review
state.

Thanks!

Stephen

Hello Stephen,

> For my 2c, at least, while I'm definitely interested in this, it's not
> nearly high enough on my plate with everything else going on to get any
> attention in the next few weeks, at least.

Fine with me.

> I do think that, perhaps, this patch may deserve a bit of a break, to 
> allow people to come back to it with a fresh perspective, so perhaps 
> moving it to the next commitfest would be a good idea, in a Needs Review 
> state.

Great, thanks. I'll move it if nobody objects.

-- 
Fabien.

Hello,

> For my 2c, at least, while I'm definitely interested in this, it's not
> nearly high enough on my plate with everything else going on to get any
> attention in the next few weeks, at least.
>
> I do think that, perhaps, this patch may deserve a bit of a break, to
> allow people to come back to it with a fresh perspective, so perhaps
> moving it to the next commitfest would be a good idea, in a Needs Review
> state.

So, let's try again for the next CF...

Here is a v9 which includes some more cleanup, hopefully in the expected 
direction which is to make pgbench expressions behave as SQL expressions, 
and I hope taking into account all other feedback as well.


CONTEXT

Pgbench has been given an expression parser (878fdcb8) which allows to use 
full expressions instead of doing one-at-a-time operations. This parser 
has been extended with functions (7e137f84) & double type (86c43f4e). The 
first batch of functions was essentially a poc about how to add new 
functions with various requirements. Pgbench default "tpcb-like" test 
takes advantage of these additions to reduce the number of lines it needs.


MOTIVATION

This patch aims at providing actually useful functions for benchmarking. 
The functions and operators provided here are usual basic operations. They 
are not chosen randomly, but are simply taken from existing benchmarks:

In TPC-B 2.0.0 section 5.3.5 and TPC-C 5.11 section 2.5.1.2, the selection 
of accounts uses a test (if ...), logical conditions (AND, OR) and 
comparisons (<, =, >=, >).

In TPC-C 5.11 section 2.1.6, a bitwise or (|) is used to skew a 
distribution based on two uniform distributions.

In TPC-C 5.11 section 5.2.5.4, a log function is used to determine "think 
time", which can be truncated (i.e. "least" function, already in pgbench).


CONTENTS

The attached patch provides a consistent set of functions and operators 
based on the above examples, with operator precedence taken from postgres 
SQL parser:

- "boolean" type support is added, because it has been requested that 
pgbench should be as close as SQL expressions as possible. This induced 
some renaming as some functions & struct fields where named "num" because 
they where expecting an int or a double, but a boolean is not really a 
numeral.

- SQL comparisons (= <> < > <= >=) plus pg SQL "!=", which result in a 
boolean.

- SQL logical operators (and or not) on booleans.

- SQL bitwise operators taken from pg: | & # << >> ~.

- mod SQL function as a synonymous for %.

- ln and exp SQL functions.

- SQL CASE/END conditional structure.

The patch also includes documentation and additional tap tests.
A test script is also provided.

This version is strict about typing, mimicking postgres behavior. For 
instance, using an int as a boolean results in a error. It is easy to make 
it more tolerant to types, which was the previous behavior before it was 
suggested to follow SQL behavior.

Together with another submitted patch about retrieving query results, the 
added capabilities allow to implement strictly conforming TPC-B 
transactions.

-- 
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 2/4/17 4:51 AM, Fabien COELHO wrote:
> 
> Hello,
> 
>> For my 2c, at least, while I'm definitely interested in this, it's not
>> nearly high enough on my plate with everything else going on to get any
>> attention in the next few weeks, at least.
>>
>> I do think that, perhaps, this patch may deserve a bit of a break, to
>> allow people to come back to it with a fresh perspective, so perhaps
>> moving it to the next commitfest would be a good idea, in a Needs Review
>> state.
> 
> So, let's try again for the next CF...
> 
> Here is a v9 which includes some more cleanup, hopefully in the expected
> direction which is to make pgbench expressions behave as SQL
> expressions, and I hope taking into account all other feedback as well.
> 
> 
> CONTEXT
> 
> Pgbench has been given an expression parser (878fdcb8) which allows to
> use full expressions instead of doing one-at-a-time operations. This
> parser has been extended with functions (7e137f84) & double type
> (86c43f4e). The first batch of functions was essentially a poc about how
> to add new functions with various requirements. Pgbench default
> "tpcb-like" test takes advantage of these additions to reduce the number
> of lines it needs.
> 
> 
> MOTIVATION
> 
> This patch aims at providing actually useful functions for benchmarking.
> The functions and operators provided here are usual basic operations.
> They are not chosen randomly, but are simply taken from existing
> benchmarks:
> 
> In TPC-B 2.0.0 section 5.3.5 and TPC-C 5.11 section 2.5.1.2, the
> selection of accounts uses a test (if ...), logical conditions (AND, OR)
> and comparisons (<, =, >=, >).
> 
> In TPC-C 5.11 section 2.1.6, a bitwise or (|) is used to skew a
> distribution based on two uniform distributions.
> 
> In TPC-C 5.11 section 5.2.5.4, a log function is used to determine
> "think time", which can be truncated (i.e. "least" function, already in
> pgbench).
> 
> 
> CONTENTS
> 
> The attached patch provides a consistent set of functions and operators
> based on the above examples, with operator precedence taken from
> postgres SQL parser:
> 
> - "boolean" type support is added, because it has been requested that
> pgbench should be as close as SQL expressions as possible. This induced
> some renaming as some functions & struct fields where named "num"
> because they where expecting an int or a double, but a boolean is not
> really a numeral.
> 
> - SQL comparisons (= <> < > <= >=) plus pg SQL "!=", which result in a
> boolean.
> 
> - SQL logical operators (and or not) on booleans.
> 
> - SQL bitwise operators taken from pg: | & # << >> ~.
> 
> - mod SQL function as a synonymous for %.
> 
> - ln and exp SQL functions.
> 
> - SQL CASE/END conditional structure.
> 
> The patch also includes documentation and additional tap tests.
> A test script is also provided.
> 
> This version is strict about typing, mimicking postgres behavior. For
> instance, using an int as a boolean results in a error. It is easy to
> make it more tolerant to types, which was the previous behavior before
> it was suggested to follow SQL behavior.
> 
> Together with another submitted patch about retrieving query results,
> the added capabilities allow to implement strictly conforming TPC-B
> transactions.

This patch applies cleanly and compiles at cccbdde with some whitespace
issues.

$ patch -p1 < ../other/pgbench-more-ops-funcs-9.patch
(Stripping trailing CRs from patch.)
patching file doc/src/sgml/ref/pgbench.sgml
(Stripping trailing CRs from patch.)
patching file src/bin/pgbench/exprparse.y
(Stripping trailing CRs from patch.)
patching file src/bin/pgbench/exprscan.l
(Stripping trailing CRs from patch.)
patching file src/bin/pgbench/pgbench.c
(Stripping trailing CRs from patch.)
patching file src/bin/pgbench/pgbench.h
(Stripping trailing CRs from patch.)
patching file src/bin/pgbench/t/002_pgbench.pl

Any reviewers want to have a look?

-- 
-David
david@pgmasters.net

Hello David,

> This patch applies cleanly and compiles at cccbdde with some whitespace 
> issues.
>
> $ patch -p1 < ../other/pgbench-more-ops-funcs-9.patch
> (Stripping trailing CRs from patch.)

My guess is that your mailer changed the eol-style of the file when saving 
it:
  sh> sha1sum pg-patches/pgbench-more-ops-funcs-9.patch  608a601561f4cba982f0ce92df30d1868338342b

ISTM that standard mime-type of *.patch and *.diff is really 
"text/x-diff", so my ubuntu laptop is somehow right to put that in 
"/etc/mime.types", but this seems to have anoying consequences at least on 
Macs.

-- 
Fabien.

Hi,

On 2017-03-16 12:21:31 -0400, David Steele wrote:
> Any reviewers want to have a look?

Unfortunately there wasn't much of that.  I think that this patch
atm doesn't have sufficient design agreement to be considered for
v10.  As the current CF has formally ended, I think we'll have to punt
this to v11.

Greetings,

Andres Freund

> Here is a v9 which includes some more cleanup, hopefully in the expected 
> direction which is to make pgbench expressions behave as SQL 
> expressions, and I hope taking into account all other feedback as well.
>
>
> CONTEXT
>
> Pgbench has been given an expression parser (878fdcb8) which allows to use 
> full expressions instead of doing one-at-a-time operations. This parser has 
> been extended with functions (7e137f84) & double type (86c43f4e). The first 
> batch of functions was essentially a poc about how to add new functions with 
> various requirements. Pgbench default "tpcb-like" test takes advantage of 
> these additions to reduce the number of lines it needs.
>
>
> MOTIVATION
>
> This patch aims at providing actually useful functions for benchmarking. The 
> functions and operators provided here are usual basic operations. They are 
> not chosen randomly, but are simply taken from existing benchmarks:
>
> In TPC-B 2.0.0 section 5.3.5 and TPC-C 5.11 section 2.5.1.2, the selection of 
> accounts uses a test (if ...), logical conditions (AND, OR) and comparisons 
> (<, =, >=, >).
>
> In TPC-C 5.11 section 2.1.6, a bitwise or (|) is used to skew a distribution 
> based on two uniform distributions.
>
> In TPC-C 5.11 section 5.2.5.4, a log function is used to determine "think 
> time", which can be truncated (i.e. "least" function, already in pgbench).
>
>
> CONTENTS
>
> The attached patch provides a consistent set of functions and operators based 
> on the above examples, with operator precedence taken from postgres SQL 
> parser:
>
> - "boolean" type support is added, because it has been requested that pgbench 
> should be as close as SQL expressions as possible. This induced some renaming 
> as some functions & struct fields where named "num" because they where 
> expecting an int or a double, but a boolean is not really a numeral.
>
> - SQL comparisons (= <> < > <= >=) plus pg SQL "!=", which result in a 
> boolean.
>
> - SQL logical operators (and or not) on booleans.
>
> - SQL bitwise operators taken from pg: | & # << >> ~.
>
> - mod SQL function as a synonymous for %.
>
> - ln and exp SQL functions.
>
> - SQL CASE/END conditional structure.
>
> The patch also includes documentation and additional tap tests.
> A test script is also provided.
>
> This version is strict about typing, mimicking postgres behavior. For 
> instance, using an int as a boolean results in a error. It is easy to make it 
> more tolerant to types, which was the previous behavior before it was 
> suggested to follow SQL behavior.
>
> Together with another submitted patch about retrieving query results, the 
> added capabilities allow to implement strictly conforming TPC-B transactions.

Given the time scale to get things through, or eventually not,
here is an update I was initially planning to submit later on.

On top of new functions, operators and the boolean type provided in v9:

  - improve boolean conversion for "yes", "on" and other variants,
    in line with pg general behavior.

  - add support for NULL, including IS test variants.

  - conditions are quite permissive i.e. non zero numericals are true
    on a test.

  - TAP tests are removed.
    I think there must be a TAP test, but the pgbench testing
    infrastructure is currently not very adequate.
    I've submitted an independent patch to enhance it:

        https://commitfest.postgresql.org/14/1118/

    that I suggest should be considered first. Once there is such
    convenient infra, I would update this patch to take advantage of it.

  - it cleans up a few things in the implementation

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

Hello Pavel,

> I am watching this patch - it looks so there are not problems.

Great.

> I found only issue in documentation - new CASE expression is not 
> documented.

Hmmm. Actually there was a rather very discreet one in v10:

  +      SQL <literal>CASE</> generic conditional expressions

I've improved it in attached v11:
  - add a link to the CASE full documentation
  - add an example expression with CASE ...

Also, if the "pgbench - improve tap test infrastructure" patch get to be 
committed, I'll add coverage for these operators and functions instead of 
the external pgbench test scripts I provided.

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

>> [doc about CASE...]
>
> I've improved it in attached v11:
> - add a link to the CASE full documentation
> - add an example expression with CASE ...

Do you think I should improve the doc further?

-- 
Fabien.

> The patch looks ok,

Ok.

> but the main issue is missing regress tests.

Yes, I agree.

> I know so it is another patch. But it should be placed differently

> 1. first patch - initial infrastructure for pgbench regress tests
> 2. this patch + related regress tests

Yep. I have no control about the display order, committers are too few and 
overbooked, pgbench is not necessarily a priority, so I can only wait for 
the non-regression test improvements to get committed some day before 
updating/adding tests for the other patches in the queue (query result in 
variable, utf8 variable names...).

-- 
Fabien.

On 5/24/17 03:14, Fabien COELHO wrote:
> I've improved it in attached v11:
>   - add a link to the CASE full documentation
>   - add an example expression with CASE ...

This patch needs (at least) a rebase for the upcoming commit fest.

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

Hello Peter,

> On 5/24/17 03:14, Fabien COELHO wrote:
>> I've improved it in attached v11:
>>   - add a link to the CASE full documentation
>>   - add an example expression with CASE ...
>
> This patch needs (at least) a rebase for the upcoming commit fest.

Here is a rebase.

It still passes my tests.

From my point of view this patch is mostly ok, after 16 months in the 
pipe.

ISTM that it is not tagged "ready" because there should be tap tests 
attached. However the current tap tests in pgbench are very poor, 
basically nothing at all is tested. There is a patch 
(https://commitfest.postgresql.org/14/1118/) also submitted for adding a 
significant tap test infrastructure, and if it gets through the idea is to 
update this operator patch to cover the different new functions and 
operators. It could be done in reverse order also, i.e. if the operator 
patch get through the tap test patch would be updated to also test the new 
functions and operators. The status of the tap-test patch is that it 
really needs to be tested on Windows.

Note that someone might object that they do not need these operators and 
functions in pgbench so they are useless, hence the patch should be 
rejected. My answer is that standard benchmarks, eg TPC-B, actually use 
these operator classes (bitwise, logical, ln...) so they are needed if 
pgbench is to be used to implement said benchmarks. And if this is not 
desirable, then what is the point of pgbench?

A renew interest is that "\if" commands have recently been added to 
"psql", an "\if" calls for logical expressions, so a next step would be to 
move the expression capability as a shared tool in front-end utils so that 
it may be used both by "pgbench" and "psql". Also, I'll probably submit an 
"\if" extension to pgbench backslash command language, as it is also 
definitely a useful tool in a benchmark script.

-- 
Fabien.

> Here is a rebase.

Argh, sorry, missing attachement... Here it is really.

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

Hello Pavel,

Here is a v13. No code changes, but TAP tests added to maintain pgbench 
coverage to green.


Summary of patch contents:

This patch extends pgbench expressions syntax while keeping 
compatibility with SQL expressions.

It adds support for NULL and BOOLEAN, as well as assorted logical, 
comparison and test operators (AND, <>, <=, IS NULL...).

A CASE construct is provided which takes advantage of the added BOOLEAN.

Integer and double functions and operators are also extended: bitwise 
operators (<< & ...), exp/ln, mod() as synonymous to % (matching pg).

Added TAP tests maintain pgbench source coverage to green (if you ignore 
lexer & parser generated files...).


Future plans include extending and synchronizing psql & pgbench variable 
and expression syntaxes:
  - move expression parsing and evaluation in fe_utils,
    which would allow to
  - extend psql with some \let i <expression> cliend-side syntax
    (ISTM that extending the \set syntax cannot be upward compatible)
    and probably handle \let as a synonymous to \set in pgbench.
  - allow \if <expression> in psql instead of just \if <boolean>
  - add \if ... support to pgbench
  - maybe add TEXT type support to the expression engine, if useful
  - maybe add :'var" and :"var" support to pgbench, if useful

There are already patches in the queue for:
  - testing whether a variable is defined in psql
    feature could eventually be added to pgbench as well
  - adding \gset (& \cset) to pgbench to get output of possibly
    combined queries into variables, which can be used for making
    decisions later in the script.

-- 
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 v13. No code changes, but TAP tests added to maintain pgbench
>> coverage to green.
>>
>> Summary of patch contents: [...]

> 1. there are no any problem with compilation, patching
> 2. all tests passed
> 3. doc is ok
>
> I'll mark this patch as ready for commiter

Ok. Thanks for the reviews.

-- 
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 v13. No code changes, but TAP tests added to maintain pgbench
>>> coverage to green.

Here is a v14, which is just a rebase after the documentation xml-ization.

-- 
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 v13. No code changes, but TAP tests added to maintain pgbench
>>>> coverage to green.
>
> Here is a v14, which is just a rebase after the documentation xml-ization.

Regenerated v15 that applies cleanly on head. No changes.

-- 
Fabien.

> I'm not sure why it wasn't failing before, but I have issues building the
> doc:
>
> +   <xref linkend="pgbench-operators"> are built into
> <application>pgbench</application>
> Missing '/' to close the xref

Indeed, missing xml-ization. The format was still SGML when the patch was 
developed.

> +      <entry><literal>1 & 3</literal></entry>
> Expecting ';' as the previous use (&)

Indeed, a typo.

>> Regenerated v15 that applies cleanly on head. No changes.

Attached v16 fixes those two errors. I regenerated the documentation with 
the new xml toolchain, and made "check" overall and in pgbench.

Thanks for the debug,

-- 
Fabien.

> Attached v16 fixes those two errors. I regenerated the documentation with the 
> new xml toolchain, and made "check" overall and in pgbench.

Attached v17 is a rebase after the zipfian commit.

-- 
Fabien.

Huh, you are fast. Rebase patch during half an hour.

I haven't objection about patch idea, but I see some gotchas in coding.

1) /* Try to convert variable to numeric form; return false on failure */
static bool
makeVariableValue(Variable *var)

as now, makeVariableValue() convert value of eny type, not only numeric

2) In makeVariableValue():
if (pg_strcasecmp(var->svalue, "null") == 0)
...
else if (pg_strncasecmp(var->svalue, "true", slen)

mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
pg_strncasecmp("tru", "true", 1) will  be 0. It may be good for 't' of 'f' but 
it seems too free grammar to accept  'tr' or 'fa' or  even 'o' which actually 
not known to be on or off.

3) It seems to me that Variable.has_value could be eliminated and then new 
PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This 
allows to shorten code and make it more readable, look
         setBoolValue(&var->value, true);
         var->has_value = true;
The second line actually doesn't needed. Although I don't insist to fix that.

I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I 
remember a collision in JavaScript with undefined and null variable.


4) valueTypeName()
it checks all possible PgBenchValue.type but believes that field could contain 
some another value. In all other cases it treats as error or assert.




Fabien COELHO wrote:
> 
>> Attached v16 fixes those two errors. I regenerated the documentation with the 
>> new xml toolchain, and made "check" overall and in pgbench.
> 
> Attached v17 is a rebase after the zipfian commit.
> 

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Hello Teodor,

> Huh, you are fast. Rebase patch during half an hour.

Hmmm... just lucky, and other after lunch tasks were more demanding.

> I haven't objection about patch idea, but I see some gotchas in coding.
>
> 1) /* Try to convert variable to numeric form; return false on failure */
> static bool
> makeVariableValue(Variable *var)
>
> as now, makeVariableValue() convert value of eny type, not only numeric

Indeed, the comments need updating. I found a few instances.

> 2) In makeVariableValue():
> if (pg_strcasecmp(var->svalue, "null") == 0)
> ...
> else if (pg_strncasecmp(var->svalue, "true", slen)
>
> mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
> pg_strncasecmp("tru", "true", 1) will  be 0.

Yep, but it cannot be called like that because slen == 
strlen(var->svalue).

> It may be good for 't' of 'f' but it seems too free grammar to accept 
> 'tr' or 'fa' or even 'o' which actually not known to be on or off.

Yes, it really works like that. I tried to make something clearer than 
"src/bin/psql/variable.c". Maybe I did not succeed.

I have added a comment and use some shortened versions in tests, with 
the -D option.

> 3) It seems to me that Variable.has_value could be eliminated and then new 
> PGBT_NOT_SET is added to PgBenchValueType enum as a first (=0) value. This 
> allows to shorten code and make it more readable, look
>        setBoolValue(&var->value, true);
>        var->has_value = true;
> The second line actually doesn't needed. Although I don't insist to fix that.

I agree that the redundancy should be avoided. I tried to keep 
"is_numeric" under some form, but there is no added value doing that.

> I actually prefer PGBT_NOT_SET instead of kind of PGBT_UNDEFINED, because I 
> remember a collision in JavaScript with undefined and null variable.

I used "PGBT_NO_VALUE" which seemed clearer otherwise a variable may be 
set and its value would not "not set" which would look strange.

> 4) valueTypeName()
> it checks all possible PgBenchValue.type but believes that field could 
> contain some another value. In all other cases it treats as error or assert.

Ok. Treated as an internal error with an assert.

-- 
Fabien.

>> 2) In makeVariableValue():
>> if (pg_strcasecmp(var->svalue, "null") == 0)
>> ...
>> else if (pg_strncasecmp(var->svalue, "true", slen)
>>
>> mixing of pg_strcasecmp and pg_strNcasecmp. And, IMHO, result of
>> pg_strncasecmp("tru", "true", 1) will  be 0.
> 
> Yep, but it cannot be called like that because slen == strlen(var->svalue).
sorry, mistyped
pg_strncasecmp("tru", "true", 3) will  be 0.

> 
>> It may be good for 't' of 'f' but it seems too free grammar to accept 'tr' or 
>> 'fa' or even 'o' which actually not known to be on or off.
> 
> Yes, it really works like that. I tried to make something clearer than 
> "src/bin/psql/variable.c". Maybe I did not succeed.
Ok, I see. Current coding accepts truexxx, falsexxx, yesxx, noxxx but doesn't 
accept offxxx and onxxx. Not so consistent as it could be. Also it doesn't 
accept 1 and 0 as psql does, but it's obviously why.

> I used "PGBT_NO_VALUE" which seemed clearer otherwise a variable may be set and 
> its value would not "not set" which would look strange.
Agree


Sorry, but I found more notices:
1) '~' and unary '-' should be commented, it's not so easy to guess about how 
they actually implemented (-1 XOR value, remember that -1 is 0xfffff....)

2)
-   | expr '%' expr         { $$ = make_op(yyscanner, "%", $1, $3); }
+   | expr '%' expr         { $$ = make_op(yyscanner, "mod", $1, $3); }

why is MOD operation renamed? Do I miss something in thread?



Looking to psql and pgbench scripting implementation, isn't it better to 
integrate lua in psql & pgbench?

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Hello Teodor,

>>> It may be good for 't' of 'f' but it seems too free grammar to accept 'tr' 
>>> or 'fa' or even 'o' which actually not known to be on or off.
>> 
>> Yes, it really works like that. I tried to make something clearer than 
>> "src/bin/psql/variable.c". Maybe I did not succeed.

> Ok, I see. Current coding accepts truexxx, falsexxx, yesxx, noxxx but doesn't 
> accept offxxx and onxxx. Not so consistent as it could be.

I've checked, but truexxx is not accepted as true. I have added a test 
case which fails on "malformed variable", i.e. it went up to scanning a 
double. When comparing ("truexxx", "true", 7) the fifth char is different, 
so it is != 0. Or I'm missing something.

> Also it doesn't accept 1 and 0 as psql does, but it's obviously why.

Yep. I have added a comment that it will be an int, and if a boolean is 
needed it would work as expected.

> Sorry, but I found more notices:
> 1) '~' and unary '-' should be commented, it's not so easy to guess about how 
> they actually implemented (-1 XOR value, remember that -1 is 0xfffff....)

Ok, I agree that it looks strange. I have added comments for both. I have 
replaced -1 by 0xffff.... so that the code is hopefully clearer.

> 2)
> -   | expr '%' expr         { $$ = make_op(yyscanner, "%", $1, $3); }
> +   | expr '%' expr         { $$ = make_op(yyscanner, "mod", $1, $3); }
>
> why is MOD operation renamed? Do I miss something in thread?

Because I have added MOD as an explicit function to match SQL, so now % is 
just a shorthand for calling it. Before the patch there was only the '%' 
operator. Changing the name is enough for the function to be found.

   pg> SELECT 11 % 3, MOD(11, 3);
   2 | 2

> Looking to psql and pgbench scripting implementation, isn't it better to 
> integrate lua in psql & pgbench?

Hmmm... if it starts on this slope, everyone will have its opinion (lua, 
tcl, python, ruby, perl, insert-script-name-here...) and it must interact 
with SQL, I'm not sure how to embed SQL & another language cleanly. So the 
idea is just to extend backslash command capabilities of psql & pgbench, 
preferably consistently, when need (i.e. use cases) arises.

Attached a new version with these changes.

-- 
Fabien.

> I've checked, but truexxx is not accepted as true. I have added a test case 
> which fails on "malformed variable", i.e. it went up to scanning a double. When 
> comparing ("truexxx", "true", 7) the fifth char is different, so it is != 0. Or 
> I'm missing something.
Oh, my fault. I've missed that. Thank you for test

> 
> Ok, I agree that it looks strange. I have added comments for both. I have 
> replaced -1 by 0xffff.... so that the code is hopefully clearer.
I changed 0xff constant  to  ~INT64CONST(0), seems, it's more consistent way.
Also I remove some whitespaces in exprparse.y. Fixed version in attachment.
> 
>> Looking to psql and pgbench scripting implementation, isn't it better to 
>> integrate lua in psql & pgbench?
> 
> Hmmm... if it starts on this slope, everyone will have its opinion (lua, tcl, 
> python, ruby, perl, insert-script-name-here...) and it must interact with SQL, 
> I'm not sure how to embed SQL & another language cleanly. So the idea is just to 
> extend backslash command capabilities of psql & pgbench, preferably 
> consistently, when need (i.e. use cases) arises.

Actually, I prefer to see single scripting implementation in both psql and 
pgbench, but I suppose nobody has a power to do it in foreseen future. And, may 
be, it's not a very good way  to invent one script language instead of using one 
of bunch of them, but, again, I'm afraid several months/years discussion about 
how and which one to embed. But scripting is needed now, I believe, at least I 
see several test scenarios which can not be implemented with current pgbench and 
this patch allows to do it.

So, I intend to push thish patch in current state. I saw several objections from 
commiters in thread, but, seems, that objections are lifted. Am I right?

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

Hello Teodor,

>> replaced -1 by 0xffff.... so that the code is hopefully clearer.
> I changed 0xff constant  to  ~INT64CONST(0), seems, it's more consistent way.
> Also I remove some whitespaces in exprparse.y. Fixed version in attachment.

Fine, quite readable this way.

> Actually, I prefer to see single scripting implementation in both psql and 
> pgbench,

I'll push for the implementations are to share more stuff in the future.

For instance the pgbench-if patch shares the conditional stack 
implementation. I intend to move pgbench expression engine as a shared 
front-end util, once its capabilites are extended and stable, which is 
basically after this patch, so that client side expressions can be used in 
psql.

Now, psql & pgbench contexts are slightly different, with an interactive 
thing which must evaluate on the fly on one side and a scripting thing on 
the other, so it would not be easy to share everything or to do everything 
the same way.

> but I suppose nobody has a power to do it in foreseen future. And, 
> may be, it's not a very good way  to invent one script language instead of 
> using one of bunch of them, but, again, I'm afraid several months/years 
> discussion about how and which one to embed. But scripting is needed now, I 
> believe, at least I see several test scenarios which can not be implemented 
> with current pgbench and this patch allows to do it.

That is exactly why I'm pushing different things into pgbench (\gset, 
\if, ...), to improve capabilities wrt to benchmarking.

> So, I intend to push thish patch in current state. I saw several objections 
> from commiters in thread, but, seems, that objections are lifted. Am I right?

I think so.

-- 
Fabien.

Hello Teodor,

> So, I intend to push thish patch in current state. I saw several objections 
> from commiters in thread, but, seems, that objections are lifted. Am I right?

Here is a rebase after "pow" addition.

-- 
Fabien.

> Here is a rebase after "pow" addition.

Huh, you are fast.

Investigating your patch I found that all arguments of logical AND/OR and CASE 
are always evaluated. Seems, it should not be for pair of reasons:
- computing of unneeded args could be too expensive
- computing of unneeded args could complicate scripting code, look:
\set zy 0
\set yz case when :zy = 0 then -1 else (1 / :zy) end
This example will fail although programmer tried to check forbidden value

case when 1>0 then 1 when 1/0 > 0 then 0 else -1 end --  fails too

SQL doesn't evaluate unneeded arguments:
select case when 1>0 then 33 when 1/0 > 0 then -33 else null end;
  case
------
    33

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

> Investigating your patch I found that all arguments of logical AND/OR and 
> CASE are always evaluated. Seems, it should not be for pair of reasons:
>
> [...]
>
> SQL doesn't evaluate unneeded arguments:

Here is a version with some lazy evaluation for and, or & case.

-- 
Fabien.

>> SQL doesn't evaluate unneeded arguments:
>
> Here is a version with some lazy evaluation for and, or & case.

v23 is a rebase.

-- 
Fabien.

Thank you, pushed

Fabien COELHO wrote:
> 
>>> SQL doesn't evaluate unneeded arguments:
>>
>> Here is a version with some lazy evaluation for and, or & case.
> 
> v23 is a rebase.
> 

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

> Thank you, pushed

Thanks!

-- 
Fabien.

Teodor Sigaev <teodor@sigaev.ru> writes:
> Thank you, pushed

Some of the Windows buildfarm members aren't too happy with this.

            regards, tom lane

> Some of the Windows buildfarm members aren't too happy with this.

Indeed.

Windows prettyprinting of double inserts a spurious "0" at the beginning 
of the exponent. Makes it look like an octal.

Here is a patch to fix it, which I cannot test on Windows.

-- 
Fabien.