Thread: pgbench - add \if support

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

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.

Another rebase after the pow function commit.

> Mostly a rebase after zipfian function commit.

-- 
Fabien.

Another rebase to try to please the patch tester.

-- 
Fabien.

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

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

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.

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

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.

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

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.

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

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

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/

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

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

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.

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

Hi,


On 2018-01-15 18:28:09 +0100, Fabien COELHO wrote:
> Here is a very minor rebase.

Your recent patches all seem to have windows line-endings. It'd be good
to fix that for the future...

- Andres

Hello Andres,

>> Here is a very minor rebase.
>
> Your recent patches all seem to have windows line-endings.

There does not seem to be any on my side:

   sh> hexdump pgbench-if-6.patch | grep '0[ad]' | head -3
     0000040 6562 636e 2e68 6773 6c6d 690a 646e 7865
     0000060 2031 3031 3630 3434 2d0a 2d2d 6120 642f
     0000080 6770 6562 636e 2e68 6773 6c6d 2b0a 2b2b

There are a few LF (0a) but no CR (0d) visible.

I'd guess that you are running on windows, that the mime-type of the 
attachement is "text/x-diff", which is what my ubuntu box lists as 
appropriate for "*.patch", and that because of "text/" your mail client 
would have decided to switch "\n" to "\r\n" on its own?

> It'd be good to fix that for the future...

What should I do about transformations on the receiving side?

-- 
Fabien.

    Fabien COELHO wrote:

> There does not seem to be any on my side:
>
>   sh> hexdump pgbench-if-6.patch | grep '0[ad]' | head -3
>     0000040 6562 636e 2e68 6773 6c6d 690a 646e 7865
>     0000060 2031 3031 3630 3434 2d0a 2d2d 6120 642f
>     0000080 6770 6562 636e 2e68 6773 6c6d 2b0a 2b2b

FYI when looking at the corresponding raw mailfile as we
subscribers receive it, here's what I got:

(for Message-ID: <alpine.DEB.2.20.1801151827260.11126@lancre>)

==== start of raw mailfile excerpt ====
Here is a very minor rebase.

--
Fabien.
--8323329-978247287-1516037290=:11126
Content-Type: text/x-diff; name=pgbench-if-6.patch
Content-Transfer-Encoding: BASE64
Content-ID: <alpine.DEB.2.20.1801151828090.11126@lancre>
Content-Description:
Content-Disposition: attachment; filename=pgbench-if-6.patch

ZGlmZiAtLWdpdCBhL2RvYy9zcmMvc2dtbC9yZWYvcGdiZW5jaC5zZ21sIGIv
ZG9jL3NyYy9zZ21sL3JlZi9wZ2JlbmNoLnNnbWwNCmluZGV4IDNkZDQ5MmMu
LmMyMDNjNDEgMTAwNjQ0DQotLS0gYS9kb2Mvc3JjL3NnbWwvcmVmL3BnYmVu
Y2guc2dtbA0KKysrIGIvZG9jL3NyYy9zZ21sL3JlZi9wZ2JlbmNoLnNnbWwN
CkBAIC04OTUsNiArODk1LDIxIEBAIHBnYmVuY2ggPG9wdGlvbmFsPiA8cmVw
bGFjZWFibGU+b3B0aW9uczwvcmVwbGFjZWFibGU+IDwvb3B0aW9uYWw+IDxy
==== cut here ====

Now this base64 part piped through `base64 -d` and then `hexdump -C`
gives:
00000000  64 69 66 66 20 2d 2d 67  69 74 20 61 2f 64 6f 63  |diff --git
a/doc|
00000010  2f 73 72 63 2f 73 67 6d  6c 2f 72 65 66 2f 70 67
|/src/sgml/ref/pg|
00000020  62 65 6e 63 68 2e 73 67  6d 6c 20 62 2f 64 6f 63  |bench.sgml
b/doc|
00000030  2f 73 72 63 2f 73 67 6d  6c 2f 72 65 66 2f 70 67
|/src/sgml/ref/pg|
00000040  62 65 6e 63 68 2e 73 67  6d 6c 0d 0a 69 6e 64 65
|bench.sgml..inde|
00000050  78 20 33 64 64 34 39 32  63 2e 2e 63 32 30 33 63  |x
3dd492c..c203c|
00000060  34 31 20 31 30 30 36 34  34 0d 0a 2d 2d 2d 20 61  |41 100644..---
a|
00000070  2f 64 6f 63 2f 73 72 63  2f 73 67 6d 6c 2f 72 65
|/doc/src/sgml/re|
00000080  66 2f 70 67 62 65 6e 63  68 2e 73 67 6d 6c 0d 0a
|f/pgbench.sgml..|
00000090  2b 2b 2b 20 62 2f 64 6f  63 2f 73 72 63 2f 73 67  |+++
b/doc/src/sg|
000000a0  6d 6c 2f 72 65 66 2f 70  67 62 65 6e 63 68 2e 73
|ml/ref/pgbench.s|
000000b0  67 6d 6c 0d 0a 40 40 20  2d 38 39 35 2c 36 20 2b  |gml..@@ -895,6
+|

Line endings are already 0d 0a at this point, which normally means
that they were like that in the mail as you submitted it.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

> 00000040  62 65 6e 63 68 2e 73 67  6d 6c 0d 0a 69 6e 64 65

Hmmm. "0d 0a" looks bad.

I've meddled into "/etc/mime.types" to change "text/x-diff" to 
"text/plain" used by some other mailers. Here is the v6 version again, 
size is 38424 and md5sum is 63d79c0d5a93294f002edc640a3f525b.

-- 
Fabien.

Hi Fabien, Daniel,

On 2018-03-01 18:47:04 +0100, Fabien COELHO wrote:
> 
> > 00000040  62 65 6e 63 68 2e 73 67  6d 6c 0d 0a 69 6e 64 65

Thanks for the analysis.

> Hmmm. "0d 0a" looks bad.

It does. TBQH the aesthetics of seing ^M's all over is what made me
complain ;)


> I've meddled into "/etc/mime.types" to change "text/x-diff" to "text/plain"
> used by some other mailers. Here is the v6 version again, size is 38424 and
> md5sum is 63d79c0d5a93294f002edc640a3f525b.

This does look better. Not quite sure what the problem is, but ...


Greetings,

Andres Freund

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

Patch seems usefull and commitable except comments in conditional.[ch]. I'd like 
to top/header comment in each file more detailed and descriptive. As for now it 
mentions only psql usage without explaining how it is basic or common.



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

Hello Teodor,

> Patch seems usefull and commitable except comments in conditional.[ch]. I'd 
> like to top/header comment in each file more detailed and descriptive. As for 
> now it mentions only psql usage without explaining how it is basic or common.

Indeed, it was not updated.

I've fixed the file names and added a simple description at the beginning 
of the header file, and a one liner in the code file.

Do you think that more is needed?

The patch also needed a rebase after the hash function addition.

-- 
Fabien.

Thank you, pushed

Fabien COELHO wrote:
> 
> Hello Teodor,
> 
>> Patch seems usefull and commitable except comments in conditional.[ch]. I'd 
>> like to top/header comment in each file more detailed and descriptive. As for 
>> now it mentions only psql usage without explaining how it is basic or common.
> 
> Indeed, it was not updated.
> 
> I've fixed the file names and added a simple description at the beginning of the 
> header file, and a one liner in the code file.
> 
> Do you think that more is needed?
> 
> The patch also needed a rebase after the hash function addition.
> 

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