pgbench - add \if support

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

pgbench - add \if support

Fabien COELHO-3

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

pgbench-if-1.patch (54K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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.

pgbench-if-2.patch (54K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

Another rebase after the pow function commit.

> Mostly a rebase after zipfian function commit.

--
Fabien.

pgbench-if-3.patch (54K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

Another rebase to try to please the patch tester.

--
Fabien.

pgbench-if-4.patch (54K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Vik Fearing-4
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

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Dmitry Dolgov
> On 4 January 2018 at 07:32, Fabien COELHO <[hidden email]> 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`.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Dmitry Dolgov
> On 8 January 2018 at 19:36, Fabien COELHO <[hidden email]> 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?

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Vik Fearing-4
In reply to this post by Fabien COELHO-3
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

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3
In reply to this post by Vik Fearing-4

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

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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

pgbench-if-5.patch (53K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Teodor Sigaev
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: [hidden email]
                                                    WWW: http://www.sigaev.ru/

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3
In reply to this post by Fabien COELHO-3

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

pgbench-if-6.patch (53K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Craig Ringer-3
On 16 January 2018 at 06:28, Fabien COELHO <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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.
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Pavel Stehule
In reply to this post by Craig Ringer-3


2018-01-21 23:31 GMT+01:00 Craig Ringer <[hidden email]>:
On 16 January 2018 at 06:28, Fabien COELHO <[hidden email]> 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

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

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - add \if support

Fabien COELHO-3

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

12
Previous Thread Next Thread