pgbench - add \if support

Started by Fabien COELHOover 8 years ago29 messageshackers
Jump to latest
#1Fabien COELHO
coelho@cri.ensmp.fr

This patch adds \if support to pgbench, similar to psql's version added
in March.

This patch brings a consistent set of features especially when combined
with two other patches already in the (slow) CF process:

- https://commitfest.postgresql.org/10/596/ .. /15/985/
adds support for booleans expressions (comparisons, logical
operators, ...). This enhanced expression engine would be useful
to allow client-side expression in psql.

- https://commitfest.postgresql.org/10/669/ .. /15/669/
adds support for \gset, so that pgbench can interact with a database
and extract something into a variable, instead of discarding it.

This patch adds a \if construct so that an expression on variables,
possibly with data coming from the database, can change the behavior of a
script.

For instance, the following script, which uses features from the three
patches, would implement TPC-B per spec (not "tpcb-like", but really as
specified).

\set tbid random(1, :scale)
\set tid 10 * (:tbid - 1) + random(1, 10)
-- client in same branch as teller at 85%
\if :scale = 1 OR random(0, 99) < 85
-- same branch
\set bid :tbid
\else
-- different branch
\set bid 1 + (:tbid + random(1, :scale - 1)) % :scale
\endif
\set aid :bid * 100000 + random(1, 100000)
\set delta random(-999999, 999999)
BEGIN;
UPDATE pgbench_accounts
SET abalance = abalance + :delta WHERE aid = :aid
RETURNING abalance AS balance \gset
UPDATE pgbench_tellers
SET tbalance = tbalance + :delta WHERE 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;

The patch moves the conditional stack infrastructure from psql to
fe_utils, so that it is available to both psql & pgbench.

A partial evaluation is performed to detect structural errors (eg missing
endif, else after else...) when the script is parsed, so that such errors
cannot occur when a script is running.

A new automaton state is added to quickly step over false branches.

TAP tests ensure reasonable coverage of the feature.

--
Fabien

Attachments:

pgbench-if-1.patchtext/x-diff; name=pgbench-if-1.patchDownload+693-306
#2Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Fabien COELHO (#1)
Re: pgbench - add \if support

Mostly a rebase after zipfian function commit.

This patch adds \if support to pgbench, similar to psql's version added in
March.

This patch brings a consistent set of features especially when combined with
two other patches already in the (slow) CF process:

- https://commitfest.postgresql.org/10/596/ .. /15/985/
adds support for booleans expressions (comparisons, logical
operators, ...). This enhanced expression engine would be useful
to allow client-side expression in psql.

- https://commitfest.postgresql.org/10/669/ .. /15/669/
adds support for \gset, so that pgbench can interact with a database
and extract something into a variable, instead of discarding it.

This patch adds a \if construct so that an expression on variables, possibly
with data coming from the database, can change the behavior of a script.

For instance, the following script, which uses features from the three
patches, would implement TPC-B per spec (not "tpcb-like", but really as
specified).

\set tbid random(1, :scale)
\set tid 10 * (:tbid - 1) + random(1, 10)
-- client in same branch as teller at 85%
\if :scale = 1 OR random(0, 99) < 85
-- same branch
\set bid :tbid
\else
-- different branch
\set bid 1 + (:tbid + random(1, :scale - 1)) % :scale
\endif
\set aid :bid * 100000 + random(1, 100000)
\set delta random(-999999, 999999)
BEGIN;
UPDATE pgbench_accounts
SET abalance = abalance + :delta WHERE aid = :aid
RETURNING abalance AS balance \gset
UPDATE pgbench_tellers
SET tbalance = tbalance + :delta WHERE 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;

The patch moves the conditional stack infrastructure from psql to fe_utils,
so that it is available to both psql & pgbench.

A partial evaluation is performed to detect structural errors (eg missing
endif, else after else...) when the script is parsed, so that such errors
cannot occur when a script is running.

A new automaton state is added to quickly step over false branches.

TAP tests ensure reasonable coverage of the feature.

--
Fabien.

Attachments:

pgbench-if-2.patchtext/x-diff; name=pgbench-if-2.patchDownload+695-306
#3Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Fabien COELHO (#2)
Re: pgbench - add \if support

Another rebase after the pow function commit.

Mostly a rebase after zipfian function commit.

--
Fabien.

Attachments:

pgbench-if-3.patchtext/x-diff; name=pgbench-if-3.patchDownload+693-305
#4Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Fabien COELHO (#3)
Re: pgbench - add \if support

Another rebase to try to please the patch tester.

--
Fabien.

Attachments:

pgbench-if-4.patchtext/x-diff; name=pgbench-if-4.patchDownload+693-305
#5Vik Fearing
vik@postgresfriends.org
In reply to: Fabien COELHO (#4)
Re: pgbench - add \if support

On 01/04/2018 07:32 AM, Fabien COELHO wrote:

Another rebase to try to please the patch tester.

Thank you. I plan on reviewing this over the weekend.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

#6Dmitry Dolgov
9erthalion6@gmail.com
In reply to: Vik Fearing (#5)
Re: pgbench - add \if support

On 4 January 2018 at 07:32, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Another rebase to try to please the patch tester.

Thanks for working on this. I had just a quick look at it, but I hope I'll have
time to post a proper review. In the meantime I'm wondering what am I doing
wrong here (I see a similar example in your first message)?

```
-- test.sql
\if random(0, 99) < 85
\set test 1
\else
\set test 2
\endif
select :test;
```

```
$ pgbench -s 10 -f test.sql
test.sql:1: unexpected character (<) in command "if"
\if random(0, 99) < 85
^ error found here
```

I'm using `pgbench-if-4.patch`, and everything is fine for simple
conditions like `\if 1`.

#7Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Dmitry Dolgov (#6)
Re: pgbench - add \if support

Hello Dmitry,

Thanks for working on this. I had just a quick look at it, but I hope
I'll have time to post a proper review. In the meantime I'm wondering
what am I doing wrong here (I see a similar example in your first
message)?

```
-- test.sql
\if random(0, 99) < 85
\set test 1
\else
\set test 2
\endif
select :test;
```

```
$ pgbench -s 10 -f test.sql
test.sql:1: unexpected character (<) in command "if"
\if random(0, 99) < 85
^ error found here

Sure.

What you are trying to do is the result of combining the pgbench-if patch
and the pgbench-more-ops-and-funcs patch.

There is also with the ability to put the result of a SELECT into a
variable, which would then enable doing some if about data coming
from the database.

https://commitfest.postgresql.org/16/985/
https://commitfest.postgresql.org/16/669/
https://commitfest.postgresql.org/16/1385/

These are distinct entries in the CF, because they do quite distinct
things, and interact weakly one with the other.

However, it really makes full sense when they are all available together,
so I put an example which combines all three. "\if 1" is not that
interesting in itself, obviously.

Everytime I sent a (relatively) big patch in the past I was asked to cut
it in bites, which is tiresome when everything is intermixed, so now I'm
doing the bytes first.

--
Fabien.

#8Dmitry Dolgov
9erthalion6@gmail.com
In reply to: Fabien COELHO (#7)
Re: pgbench - add \if support

On 8 January 2018 at 19:36, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

What you are trying to do is the result of combining the pgbench-if patch
and the pgbench-more-ops-and-funcs patch.

Oh, I see. I missed the first message about this patch, sorry. But for some
reason I can't apply both patches (pgbench-more-ops-funcs-23.patch and
pgbench-if-4.patch) in any order:

Hunk #24 FAILED at 2824.
Hunk #25 succeeded at 5178 (offset 251 lines).
1 out of 25 hunks FAILED -- saving rejects to file
src/bin/pgbench/pgbench.c.rej
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/pgbench/pgbench.h
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/pgbench/t/001_pgbench_with_server.pl
Hunk #2 FAILED at 226.
Hunk #3 FAILED at 287.
Hunk #4 succeeded at 448 (offset 41 lines).
Hunk #5 succeeded at 505 (offset 41 lines).
Hunk #6 succeeded at 516 (offset 41 lines).
2 out of 6 hunks FAILED -- saving rejects to file
src/bin/pgbench/t/001_pgbench_with_server.pl.rej

Is there any other dependency I should apply?

#9Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Dmitry Dolgov (#8)
Re: pgbench - add \if support

Hello Dmitry,

What you are trying to do is the result of combining the pgbench-if patch
and the pgbench-more-ops-and-funcs patch.

Oh, I see. I missed the first message about this patch, sorry. But for some
reason I can't apply both patches (pgbench-more-ops-funcs-23.patch and
pgbench-if-4.patch) in any order:

Hunk #24 FAILED at 2824. [...]

Indeed, they interfere.

Is there any other dependency I should apply?

No, the patches really conflict in minor ways. They are expected to be
reviewed independently. If/when one feature is committed, I('ll) update
the remaining patches so that they still work.

If I produce dependent patches ISTM that it makes a more complicated
review, so the patches are less likely to be reviewed and maybe finally
committed.

I just wanted to point out that the the features are more interestings
when combined than on their own.

--
Fabien.

#10Vik Fearing
vik@postgresfriends.org
In reply to: Fabien COELHO (#1)
Re: pgbench - add \if support

On 11/25/2017 10:33 PM, Fabien COELHO wrote:

This patch adds \if support to pgbench, similar to psql's version added
in March.

This patch brings a consistent set of features especially when combined
with two other patches already in the (slow) CF process:

�- https://commitfest.postgresql.org/10/596/ .. /15/985/
�� adds support for booleans expressions (comparisons, logical
�� operators, ...). This enhanced expression engine would be useful
�� to allow client-side expression in psql.

�- https://commitfest.postgresql.org/10/669/ .. /15/669/
�� adds support for \gset, so that pgbench can interact with a database
�� and extract something into a variable, instead of discarding it.

This patch adds a \if construct so that an expression on variables,
possibly with data coming from the database, can change the behavior of
a script.

I have given this patch a pretty good shake and I'm happy with it. I
did not test it with the other two patches, only on its own.

A partial evaluation is performed to detect structural errors (eg
missing endif, else after else...) when the script is parsed, so that
such errors cannot occur when a script is running.

Very good.

A new automaton state is added to quickly step over false branches.

This one took me a little while to understand while reading the patch,
but mostly because of how diff doesn't handle moving things around.

TAP tests ensure reasonable coverage of the feature.

And the documentation seems sufficient, as well.

It's a shame this feature uses \elif instead of \elsif to be closer to
plpgsql, but I suppose this ship already sailed when psql chose \elif,
and I think it is correct that this patch follows psql.

Marking as ready for committer.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

#11Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Vik Fearing (#10)
Re: pgbench - add \if support

Hello Vik,

This patch adds a \if construct so that an expression on variables,
possibly with data coming from the database, can change the behavior of
a script.

I have given this patch a pretty good shake and I'm happy with it. I
did not test it with the other two patches, only on its own.

As noted by Dmitry, they intefere slightly one with the other so it
would require some conflict resolution.

[...]

Marking as ready for committer.

Ok, thanks.

--
Fabien.

#12Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Vik Fearing (#10)
Re: pgbench - add \if support

A new automaton state is added to quickly step over false branches.

This one took me a little while to understand while reading the patch,
but mostly because of how diff doesn't handle moving things around.

"git diff -w --patience" may help.

Marking as ready for committer.

Here is a rebase. I made some tests use actual expressions instead of just
0 and 1. No other changes.

--
Fabien.

#13Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Fabien COELHO (#12)
Re: pgbench - add \if support

Here is a rebase. I made some tests use actual expressions instead of just 0
and 1. No other changes.

Sigh. Better with the attachment. Sorry for the noise.

--
Fabien.

Attachments:

pgbench-if-5.patchtext/x-diff; charset=us-ascii; name=pgbench-if-5.patchDownload+674-306
#14Teodor Sigaev
teodor@sigaev.ru
In reply to: Fabien COELHO (#13)
Re: pgbench - add \if support

Hi!

Hm, isn't already commited when/case/then/else syntax do the same? If not, could
it be added to existing synax?

Fabien COELHO wrote:

Here is a rebase. I made some tests use actual expressions instead of just 0
and 1. No other changes.

Sigh. Better with the attachment. Sorry for the noise.

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

#15Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Teodor Sigaev (#14)
Re: pgbench - add \if support

Hm, isn't already commited when/case/then/else syntax do the same?

No, not strictly. The "CASE WHEN" is an if *within* an expression:

\set i CASE WHEN condition THEN val1 ELSE val2 END

The \if is at the script level, like psql already available version, which
can change what SQL is sent.

\if condition
SOME SQL
\else
OTHER SQL
\endif

You could achieve the CASE semantics with some \if:

\if condition
\set i val1
\else
\set i val2
\endif

But the reverse is not possible.

--
Fabien.

#16Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Fabien COELHO (#13)
Re: pgbench - add \if support

Here is a rebase. I made some tests use actual expressions instead of just
0 and 1. No other changes.

Sigh. Better with the attachment. Sorry for the noise.

Here is a very minor rebase.

--
Fabien.

Attachments:

pgbench-if-6.patchtext/x-diff; name=pgbench-if-6.patchDownload+673-305
#17Craig Ringer
craig@2ndquadrant.com
In reply to: Fabien COELHO (#16)
Re: pgbench - add \if support

On 16 January 2018 at 06:28, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Here is a rebase. I made some tests use actual expressions instead of just

0 and 1. No other changes.

Sigh. Better with the attachment. Sorry for the noise.

Here is a very minor rebase.

I'm a smidge worried about this. It seems like psql is growing a scripting
language. Do we want to go our own way with a kind of organically grown
scripting system? Or should we be looking at embedding client-side
scripting in a more structured, formal way instead? Embed a lua interpreter
or something?

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#18Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Craig Ringer (#17)
Re: pgbench - add \if support

Helo Craig,

I'm a smidge worried about this. It seems like psql is growing a
scripting language.

The patch is about aligning pgbench with psql, which already has \if.

Do we want to go our own way with a kind of organically grown
scripting system? Or should we be looking at embedding client-side
scripting in a more structured, formal way instead? Embed a lua interpreter
or something?

My 0.02€ is that the point is to deal with useful/needed simple client
capabilities while integrating gracefully with bare server-side executed
SQL.

As for useful client-side capabilities, for both psql & pgbench ISTM that
it is more in line with a limited cpp-like thing: include, expressions,
variables, conditions... maybe minimal error handling. No loop.

As for a language interpreter, it would raise the question of which
language (lua, tcl, python, perl, VB, sh, R, ...) and the graceful (upward
compatible) integration of any such language: eg how do have pieces of
bare SQL and any other existing language would require some scanning
conventions that do not exist.

psql & pgbench already have ":x" variables. psql has the ability to set
variable from SQL (\gset), and pgbench could do limited expressions to set
these variables with (\set), which have been extended to be more complete
, and there was use cases which motivate an (\if).

ISTM enough to align both tools for reasonnably simple use cases that
could arise when running a basic SQL script of bench. If you have
something really complicated, then full fledge programming is the answer,
which cannot be bare-SQL compatible.

So the answer is that it is okay to aim at "limited" scripting because it
covers useful use cases.

--
Fabien.

#19Pavel Stehule
pavel.stehule@gmail.com
In reply to: Craig Ringer (#17)
Re: pgbench - add \if support

2018-01-21 23:31 GMT+01:00 Craig Ringer <craig@2ndquadrant.com>:

On 16 January 2018 at 06:28, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Here is a rebase. I made some tests use actual expressions instead of

just 0 and 1. No other changes.

Sigh. Better with the attachment. Sorry for the noise.

Here is a very minor rebase.

I'm a smidge worried about this. It seems like psql is growing a scripting
language. Do we want to go our own way with a kind of organically grown
scripting system? Or should we be looking at embedding client-side
scripting in a more structured, formal way instead? Embed a lua interpreter
or something?

few scripting features doesn't mean scripting language. \if in psql is nice
feature that reduce duplicate code, unreadable code, and helps with
deployment and test scripts. pgbench and psql should to have similar
environment - and I am thinking so \if should be there.

Using Lua is not bad idea - in psql too - I though about it much, but in
this case is better to start from zero.

Regards

Pavel

Show quoted text

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

#20Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Pavel Stehule (#19)
Re: pgbench - add \if support

few scripting features doesn't mean scripting language. \if in psql is nice
feature that reduce duplicate code, unreadable code, and helps with
deployment and test scripts. pgbench and psql should to have similar
environment - and I am thinking so \if should be there.

Using Lua is not bad idea - in psql too - I though about it much, but in
this case is better to start from zero.

Yep. Having another versatile (interactive) client would not be a bad
thing. I'm still wondering how to conciliate any scripting language with
"bare SQL". The backslash-oriented syntax already used for psql & pgbench
seems the only available option. Otherwise ISTM that it is back to a
standard library oriented client access with import, connect, exec...
whatever set of function already provided by standard libraries (psycopg
for python, ...).

--
Fabien.

#21Pavel Stehule
pavel.stehule@gmail.com
In reply to: Fabien COELHO (#20)
#22Andres Freund
andres@anarazel.de
In reply to: Fabien COELHO (#16)
#23Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Andres Freund (#22)
#24Daniel Verite
daniel@manitou-mail.org
In reply to: Fabien COELHO (#23)
#25Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Daniel Verite (#24)
#26Andres Freund
andres@anarazel.de
In reply to: Fabien COELHO (#25)
#27Teodor Sigaev
teodor@sigaev.ru
In reply to: Fabien COELHO (#15)
#28Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Teodor Sigaev (#27)
#29Teodor Sigaev
teodor@sigaev.ru
In reply to: Fabien COELHO (#28)