Thread: [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

--
Michael Paquier

NTT OSSC

On Thu, Aug 6, 2009 at 11:26 PM, Michael
Paquier<michael.paquier@gmail.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.paquier@gmail.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

--
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
correctpatch 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'snot 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

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,<br /><br />Sorry for my late reply again :o)<br />You will find my answers on-the-line.<br /><br />> > You
reallyshould be returning a value at the point since the function<br /> > > signature defines a return type. If
notthe function should be void,<br /> > > which it cannot be in this context since it is used for boolean
tests<br/> > > elsewhere. The returns in question are all part of error blocks and<br /> > > should return
false.<br/><br />The function doCustom is defined with a void.<br />I agree that it is far cleaner to return a value
butby returning in my own code part a boolean or an integer, it will have a large impact on other parts of the code
thatare dealing with the original command types of pgbench.<br /> By the way, the errors created by doCustom are
managedthrough CState for each customer, so letting it like it is is far sufficient and has no impact on other parts of
thecode.<br /><br />> So I get this output with and with out the shell command in there,<br /> > against the
unpatchedand patched version on pgbench. The commands you<br /> > sent will not work with this script since it is
usingprepared<br /> > statements. I am using this command to run the script.<br /><br />I made on my side a couple
oftests to see what is happening in the code.<br />The problem you saw is not linked to the patch I wrote.<br />In fact
whenyou are using the prepared statement mode, PQprepare seems not to be able to manage with the parameter I put in my
transactionscript at "prepare transaction" and "commit prepared" stages. This parameter is the random ID used for
preparedtransaction.<br /> If you try an end script such as:<br />PREPARE TRANSACTION 'T';<br />\shell ls -ll
~/pgsql/data<br/>COMMIT PREPARED 'T';<br />It will work without any problem in both prepared and single query mode.<br
/><br/>For 1 customer, there is no issue. However, by increasing the number of customers, it will increase the risk for
acustomer to prepare a transaction who is using a same 2pc ID, resulting in a couple of output errors.<br /> Using the
originalscript I wrote is OK in single query mode, as 2pc ID is not managed by PQprepare but at pgbench level. This way
pgbenchwill not create errors, as it builds an individual query with a random ID one by one. The prepared query mode
preparesthe same transaction for all the customers of pgbench, and I think that PQprepare cannot manage 2pc IDs while
preparinga transaction. Parameters in SQL queries is OK, but not at prepare states.<br /> That would explain why in
thiscase, the system was looking for a parameter that is not delivered initially.<br /><br />Besides, you can also make
testswithout 2pc transactions, such as:<br />\shell ls -ll /home/ioltas/usr/pgsql/data<br /> END;<br />In this case
alsoit works finely in both prepared and single query mode (at least on my side :)).<br /><br />By looking at the
documentationof pgbench, prepared query mode is used for performance purposes only. The pgbench shell feature tends to
slowdown all the process as it has been created for analysis purposes only, so the single query mode is enough to my
mindif you want to study the interactions of your system and postgres.<br /><br />Regards,<br /><br />Michael<br /> 

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




--
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@gregsmith.com http://www.gregsmith.com Baltimore, MD