Re: [PATCH] pgbench: new feature allowing to launch shell commands

Hi all,

Here is a short patch implementing a new feature in pgbench so as to allow
shell commands to be launched in a transaction file of pgbench.
the user has just to add at the beginning of the command line in his
transaction file \shell + the command wanted.

As an example of transaction:
Begin;
[Transaction instructions]
Prepare transaction ‘TXID’;
\shell ls ~/pg_twophase;
Commit prepared ‘TXID’;

This patch was particularly useful in order to determine the size of state
files flushed to disk for prepared but not committed transactions.
As an addition, I added a new default transaction in the code that permits
to launch a 2PC transaction with a random prepare identifier of the format
Txxx.

I also created a page in postgresql's wiki about this feature.
Please refer to this link:
http://wiki.postgresql.org/wiki/Pgbench:_shell_command

Regards,

--
Michael Paquier

NTT OSSC

Sorry I forgot to attach the the patch.

Regards,

Michael

On Fri, Aug 7, 2009 at 12:23 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com>wrote:

> Hi all,
>
> Here is a short patch implementing a new feature in pgbench so as to allow
> shell commands to be launched in a transaction file of pgbench.
> the user has just to add at the beginning of the command line in his
> transaction file \shell + the command wanted.
>
> As an example of transaction:
> Begin;
> [Transaction instructions]
> Prepare transaction ‘TXID’;
> \shell ls ~/pg_twophase;
> Commit prepared ‘TXID’;
>
> This patch was particularly useful in order to determine the size of state
> files flushed to disk for prepared but not committed transactions.
> As an addition, I added a new default transaction in the code that permits
> to launch a 2PC transaction with a random prepare identifier of the format
> Txxx.
>
> I also created a page in postgresql's wiki about this feature.
> Please refer to this link:
> http://wiki.postgresql.org/wiki/Pgbench:_shell_command
>
> Regards,
>
> --
> Michael Paquier
>
> NTT OSSC
>

--
Michael Paquier

NTT OSSC

On Thu, Aug 6, 2009 at 11:26 PM, Michael
Paquier<michael(dot)paquier(at)gmail(dot)com> wrote:
> Sorry I forgot to attach the the patch.

Please add your patches at
https://commitfest.postgresql.org/action/commitfest_view/open

...Robert

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:

> > Here is a short patch implementing a new feature in pgbench so as to allow
> > shell commands to be launched in a transaction file of pgbench.
> > \shell ls ~/pg_twophase;

+1 for \shell command itself, but does the performance fit for your purpose?
Spawning a new process is not so cheap, no?

-1 for -P option because it is too narrow purpose and 'ls' and '/tmp/'
is not portable. We don't need to include your workload because you can
use -f FILENAME to run your benchmark script.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Michael Paquier escribió:

> I also created a page in postgresql's wiki about this feature.
> Please refer to this link:
> http://wiki.postgresql.org/wiki/Pgbench:_shell_command

Please don't use colons in wiki page names. Pgbench_shell_command
should be fine.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Yes it dramatically decreases the transaction flow.
This function has not been implemented at all for performance but for
analysis purposes.
I used it mainly to have a look at state files size in pg_twophase for
transactions that are prepared but not committed.

Regards

On Fri, Aug 7, 2009 at 12:55 PM, Itagaki Takahiro <
itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:

>
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
>
> > > Here is a short patch implementing a new feature in pgbench so as to
> allow
> > > shell commands to be launched in a transaction file of pgbench.
> > > \shell ls ~/pg_twophase;
>
> +1 for \shell command itself, but does the performance fit for your
> purpose?
> Spawning a new process is not so cheap, no?
>
> -1 for -P option because it is too narrow purpose and 'ls' and '/tmp/'
> is not portable. We don't need to include your workload because you can
> use -f FILENAME to run your benchmark script.
>
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center
>
>
>

--
Michael Paquier

NTT OSSC

Thanks a lot for all of your pieces of advice.
I modified the name of the page as well as I deleted the parts linked to the
-P option.
It just consisted in deleting the right parts.

Here is the lighted version.

--
Michael Paquier

NTT OSSC

Michael,

I just wanted to follow-up on your pgbench patch. The latest version
that I see is from August 13th. Is that the correct patch to be
reviewing? Do you have any other updates on it?

Thanks!

Stephen

On Tue, Sep 15, 2009 at 09:53:11PM -0400, Stephen Frost wrote:
> Michael,
>
> I just wanted to follow-up on your pgbench patch. The latest version
> that I see is from August 13th. Is that the correct patch to be
> reviewing? Do you have any other updates on it?
>
> Thanks!
>
> Stephen

Hi!

Some comments about this patch:

- You have DOS-style carriage returns, which interfere with the patch application on Unix systems.
- We'd like to see return specifically return a value (lines 1008 and 1022 in the patched version)
- We'd like to see something done with the return value from system (line 1026 in patched version)
- pg_bench functions as expected, however, your example script given on the wiki page for this patch fails (http://wiki.postgresql.org/wiki/Pgbench:_shell_command). Can we have an example that works so we can check it out? It's not really clear to us how this will be useful to others.

Thanks!

Gabrielle & Dan

Hi all,

Sorry for my late reply.
There is no other update for this patch since the 13th of August, at least
until today. The new patch is attached
By the way I worked on the comments that Dan and Gabriel pointed out.
I added a check on system such as to prevent an error from this side.
By the way, I noticed that the way return values are managed in doCustom and
in process_commands is different. Such as to make an homogeneous code,
return values are managed the same way in both functions in the patch,
explaining why I did not return a specific value when file commands are
treated in doCustom.

Here is also as wanted a simple transaction that permits to use this
function:
\set nbranches :scale
\set naccounts 100000 * :scale
\setrandom aid 1 :naccounts
\setrandom bid 1 :nbranches
\setrandom delta -5000 5000
\setrandom txidrand 0 10000
START TRANSACTION;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
PREPARE TRANSACTION ':txidrand';
\shell ls -ll $PGDATA/pg_twophase
COMMIT PREPARED ':txidrand';

The shell command I use here permits to scan the 2PC state files written in
pg_twophase.
You will notice that in this case files have a size of 540B. Also please do
not forget to set PGDATA.

The new functionnality proposed here is just for analysis purposes. Even if
it decreased the performance of pgbench, it is interesting to have a simple
benchmark that permits to analyse precisely the system while transaction are
being run.

Regards,

On Thu, Sep 17, 2009 at 09:56:44AM +0900, Michael Paquier wrote:
> Hi all,
>
> Sorry for my late reply.
> There is no other update for this patch since the 13th of August, at least
> until today. The new patch is attached
> By the way I worked on the comments that Dan and Gabriel pointed out.
> I added a check on system such as to prevent an error from this side.
> By the way, I noticed that the way return values are managed in doCustom and
> in process_commands is different. Such as to make an homogeneous code,
> return values are managed the same way in both functions in the patch,
> explaining why I did not return a specific value when file commands are
> treated in doCustom.

You really should be returning a value at the point since the function
signature defines a return type. If not the function should be void,
which it cannot be in this context since it is used for boolean tests
elsewhere. The returns in question are all part of error blocks and
should return false.

>
> Here is also as wanted a simple transaction that permits to use this
> function:
> \set nbranches :scale
> \set naccounts 100000 * :scale
> \setrandom aid 1 :naccounts
> \setrandom bid 1 :nbranches
> \setrandom delta -5000 5000
> \setrandom txidrand 0 10000
> START TRANSACTION;
> UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
> SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
> UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
> PREPARE TRANSACTION ':txidrand';
> \shell ls -ll $PGDATA/pg_twophase
> COMMIT PREPARED ':txidrand';

I must be doing something wrong when I am running this script. It is
still throwing errors. Would you mind showing me the pgbench command you
used to run it?

--
--Dan

>
> You really should be returning a value at the point since the function
> signature defines a return type. If not the function should be void,
> which it cannot be in this context since it is used for boolean tests
> elsewhere. The returns in question are all part of error blocks and
> should return false.
>

OK I got your point, I missed the part managing with CState, which is tested
after doCustom.
Another version of the patch is attached with this email.

I must be doing something wrong when I am running this script. It is
> still throwing errors. Would you mind showing me the pgbench command you
> used to run it?
>
> Of course, here it is the list of commands I use:
pgbench -i dbname (in case your database is called dbname)
pgbench -c 10 -t 10 -f transaction_file_name.data dbname (customer and
transaction numbers can also bet set as you want).

Regards,

--
Michael Paquier

NTT OSSC

On Fri, Sep 18, 2009 at 03:10:14PM +0900, Michael Paquier wrote:
> >
> > You really should be returning a value at the point since the function
> > signature defines a return type. If not the function should be void,
> > which it cannot be in this context since it is used for boolean tests
> > elsewhere. The returns in question are all part of error blocks and
> > should return false.
> >
>
> OK I got your point, I missed the part managing with CState, which is tested
> after doCustom.
> Another version of the patch is attached with this email.
>
> I must be doing something wrong when I am running this script. It is
> > still throwing errors. Would you mind showing me the pgbench command you
> > used to run it?
> >
> > Of course, here it is the list of commands I use:
> pgbench -i dbname (in case your database is called dbname)
> pgbench -c 10 -t 10 -f transaction_file_name.data dbname (customer and
> transaction numbers can also bet set as you want).
>
> Regards,
>
> --
> Michael Paquier
>
> NTT OSSC

OK, thank you for sending me the patch and the relivent commands. You're
still not returning false where you should be, but that wasn't the issue
I am seeing. I believe there is an issue in either the custom script you
posted on the wiki or pgbench itself. Here is the script I am running,
you might recongnize it :)

\set nbranches :scale
\set ntellers 10 * :scale
\set naccounts 100000 * :scale
\setrandom aid 1 :naccounts
\setrandom bid 1 :nbranches
\setrandom tid 1 :ntellers
\setrandom delta -5000 5000
\setrandom txidrand 0 10000
START TRANSACTION;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
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);
PREPARE TRANSACTION ':txidrand';
\shell ls -ll >> /tmp/log_data`date '+%Y%m%d'`_`date '+%k%M'`
COMMIT PREPARED ':txidrand';

So I get this output with and with out the shell command in there,
against the unpatched and patched version on pgbench. The commands you
sent will not work with this script since it is using prepared
statements. I am using this command to run the script.

./contrib/pgbench/pgbench -c 10 -j 2 -M prepared -f custom.script test

From which, I get the following output:

starting vacuum...end.
Client 0 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 1 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 2 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 5 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 8 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 6 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 7 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 3 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 4 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
Client 9 aborted in state 15: ERROR: bind message supplies 1 parameters, but prepared statement "P0_15" requires 0
transaction type: Custom query
scaling factor: 1
query mode: prepared
number of clients: 10
number of threads: 2
number of transactions per client: 10
number of transactions actually processed: 0/100
tps = 0.000000 (including connections establishing)
tps = 0.000000 (excluding connections establishing)

Since it occurs on both versions, I'm having some trouble identifying
whether this is a bug in the script or a bug in pgbench. Any help is
appreciated.

--
--Dan

Hi,

Sorry for my late reply again :o)
You will find my answers on-the-line.

> > You really should be returning a value at the point since the function
> > signature defines a return type. If not the function should be void,
> > which it cannot be in this context since it is used for boolean tests
> > elsewhere. The returns in question are all part of error blocks and
> > should return false.

The function doCustom is defined with a void.
I agree that it is far cleaner to return a value but by returning in my own
code part a boolean or an integer, it will have a large impact on other
parts of the code that are dealing with the original command types of
pgbench.
By the way, the errors created by doCustom are managed through CState for
each customer, so letting it like it is is far sufficient and has no impact
on other parts of the code.

> So I get this output with and with out the shell command in there,
> against the unpatched and patched version on pgbench. The commands you
> sent will not work with this script since it is using prepared
> statements. I am using this command to run the script.

I made on my side a couple of tests to see what is happening in the code.
The problem you saw is not linked to the patch I wrote.
In fact when you are using the prepared statement mode, PQprepare seems not
to be able to manage with the parameter I put in my transaction script at
"prepare transaction" and "commit prepared" stages. This parameter is the
random ID used for prepared transaction.
If you try an end script such as:
PREPARE TRANSACTION 'T';
\shell ls -ll ~/pgsql/data
COMMIT PREPARED 'T';
It will work without any problem in both prepared and single query mode.

For 1 customer, there is no issue. However, by increasing the number of
customers, it will increase the risk for a customer to prepare a transaction
who is using a same 2pc ID, resulting in a couple of output errors.
Using the original script I wrote is OK in single query mode, as 2pc ID is
not managed by PQprepare but at pgbench level. This way pgbench will not
create errors, as it builds an individual query with a random ID one by one.
The prepared query mode prepares the same transaction for all the customers
of pgbench, and I think that PQprepare cannot manage 2pc IDs while preparing
a transaction. Parameters in SQL queries is OK, but not at prepare states.
That would explain why in this case, the system was looking for a parameter
that is not delivered initially.

Besides, you can also make tests without 2pc transactions, such as:
\shell ls -ll /home/ioltas/usr/pgsql/data
END;
In this case also it works finely in both prepared and single query mode (at
least on my side :)).

By looking at the documentation of pgbench, prepared query mode is used for
performance purposes only. The pgbench shell feature tends to slow down all
the process as it has been created for analysis purposes only, so the single
query mode is enough to my mind if you want to study the interactions of
your system and postgres.

Regards,

Michael

On Tue, 22 Sep 2009, Michael Paquier wrote:

> Besides, you can also make tests without 2pc transactions, such as:
> \shell ls -ll /home/ioltas/usr/pgsql/data
> END;

I think that demonstrating the pgbench shell feature with this 2PC example
is working against your patch being even considered, much less accepted.
It's way too complicated, and the idea you're pouplarizing with it that
the script should save whatever it does in the external filesystem is
messy. You'll need a simpler one that still accomplishes something useful
that can go into the docs no matter what, the problems with prepared
transactions you're reporting in your messages are just the beginning of
issues that come from presenting this as the "hello, world" of how to use
the shell feature.

Most of the cases I can think of for situations where I'd like to call the
shell in a pgbench script require doing something useful with the result
that is returned. For example, when I'm picking a row at random, I often
want to skew which row that is, to simulate the common real-world
situation where a fraction of customers generate most of the transactions
(the Pareto principle).

What I'd consider closer to a useful implementation of this feature is
that you could call a shell script and use the first line of its output to
set one of the pgbench internal variables. The range of return codes is
way too limited for those to help here. Here's what that might look like,
assuming "skewedrand" is our script that does that given the range it
needs to operate over:

\set nbranches :scale
\set ntellers 10 * :scale
\set naccounts 100000 * :scale
\setshell aid skewedrand 1 :naccounts
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;

(I'm not sure if setshell would need to interpret the output as an integer
for this to work right, there may be some int vs. string considerations
here)

If you got something like that working first before moving onto these more
complicated examples and I think you'll have an easier time of things.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

See attached a patch of this setshell feature.

If you use in a script file something like:
/setshell param_set setshellparam.sh
pgbench reads from the shell script setshellparam.sh the first output value,
verifies if it is an integer, then manages it as a pgbench parameter.
I did not take into account you 4th and 5th arguments, so it is just a basic
implementation.

Depending on the statistical model you want to use for you pgbench tests, it
is possible to add other parameters as on the call prototype you proposed.

Regards,

Michael

On Wed, Sep 23, 2009 at 8:17 AM, Greg Smith <gsmith(at)gregsmith(dot)com> wrote:

> On Tue, 22 Sep 2009, Michael Paquier wrote:
>
> Besides, you can also make tests without 2pc transactions, such as:
>> \shell ls -ll /home/ioltas/usr/pgsql/data
>> END;
>>
>
> I think that demonstrating the pgbench shell feature with this 2PC example
> is working against your patch being even considered, much less accepted.
> It's way too complicated, and the idea you're pouplarizing with it that the
> script should save whatever it does in the external filesystem is messy.
> You'll need a simpler one that still accomplishes something useful that can
> go into the docs no matter what, the problems with prepared transactions
> you're reporting in your messages are just the beginning of issues that come
> from presenting this as the "hello, world" of how to use the shell feature.
>
> Most of the cases I can think of for situations where I'd like to call the
> shell in a pgbench script require doing something useful with the result
> that is returned. For example, when I'm picking a row at random, I often
> want to skew which row that is, to simulate the common real-world situation
> where a fraction of customers generate most of the transactions (the Pareto
> principle).
>
> What I'd consider closer to a useful implementation of this feature is that
> you could call a shell script and use the first line of its output to set
> one of the pgbench internal variables. The range of return codes is way too
> limited for those to help here. Here's what that might look like, assuming
> "skewedrand" is our script that does that given the range it needs to
> operate over:
>
> \set nbranches :scale
> \set ntellers 10 * :scale
> \set naccounts 100000 * :scale
> \setshell aid skewedrand 1 :naccounts
> SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
>
> (I'm not sure if setshell would need to interpret the output as an integer
> for this to work right, there may be some int vs. string considerations
> here)
>
> If you got something like that working first before moving onto these more
> complicated examples and I think you'll have an easier time of things.
>
> --
> * Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD
>

--
Michael Paquier

NTT OSSC

On Tue, 22 Sep 2009, Michael Paquier wrote:

> The function doCustom is defined with a void.

I see this in pgbench.c:

/* return false iff client should be disconnected */
static bool
doCustom(CState *st, instr_time *conn_time)

I think you need to increase the verbosity of the error messages when
you're working on this code, because when I compile I get a slew of these
errors pointing to the problem too:

pgbench.c:1009: warning: return with no value, in function returning non-void

The fix is that when there's an error, you need to do this:

return clientDone(st, false);

To indicate to DoCustom that things have gone badly and it shouldn't try
executing anything else in the script.

Your patch didn't apply cleanly either, but I think that's not your fault.
There's a lot of code that looks quite similar in this area and both "git
apply" and "patch" seemed confused. Probably needs more context around
the diff next time.

> See attached a patch of this setshell feature. If you use in a script
> file something like:
> /setshell param_set setshellparam.sh
> pgbench reads from the shell script setshellparam.sh the first output
> value, verifies if it is an integer, then manages it as a pgbench
> parameter. I did not take into account you 4th and 5th arguments, so it
> is just a basic implementation.

That part looks fine so far, but what actually needs to happen here is
that the rest of the line after the name of the script should be passed to
the script as part of its command line.

This patch is moving in the right direction, it still needs some work.
So far my list of concerns is:

* Make the patch diff apply cleanly to HEAD (I'm past this)

* Update the return logic (already fixed in my copy, just need to update
all the "return" statements with no value)

* Make "setshell" pass through the rest of the command line (I could fix
this, but don't really want to)

* We need a much simpler example how this patch can be helpful and used
(this I will do, I have a couple of ideas)

* The documentation needs to be updated with that example and the rest of
the details on what you've added. If you plan to submit more patches in
the future, you probably want to get familiar with this eventually, and I
encourage you to take a shot at it. I have some other pgbench docs fixes
to apply anyway soon, so I wouldn't mind taking care of this too once
we're close to code that can be committed.

We're trying to close up this CommitFest, so I'm going to mark this one
"returned with feedback" for now. I've got it sitting in my personal git
repo how and can help out with the details. I'll be glad to work with you
to get it into shape for the next CommitFest, where I think it can be a
useful feature to add.

--
* Greg Smith gsmith(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD

>
>
>
> I see this in pgbench.c:
>
> /* return false iff client should be disconnected */
> static bool
> doCustom(CState *st, instr_time *conn_time)
>
> I think you need to increase the verbosity of the error messages when
> you're working on this code, because when I compile I get a slew of these
> errors pointing to the problem too:
> pgbench.c:1009: warning: return with no value, in function returning
> non-void
> The fix is that when there's an error, you need to do this:
>
> return clientDone(st, false);
>
Thanks for the tip. In fact I based my patch on postgres 8.4.1 and not on
the head of the git repository.
This explains why I did not go through the error messages returned by
doCustom.
The new version of the patch attached to this email has been made directly
from the git repository. It looks definitely cleaner this time.

I tried to clean the patch so as to pass the rest of the command line also,
so as not to have to do it later.
A short update of the setshell feature is also available on PostgreSQL's
wiki at the page pgbench shell command.

About the potential examples, I can also write a short script and put that
on the wiki.
As you said previously, the Pareto example is possible, but also why not
thinking about other statistical distributions? a Gaussian or a Poisson
distribution? There are many possibilities.

Regards,

--
Michael Paquier

NTT OSSC