Horton
Menu

Thread

  1. Re: pgbench: new feature allowing to launch shell commands

    Greg Smith <greg@2ndquadrant.com> — 2009-12-02T06:55:56Z

    Here's a first round of review of the patch submitted at 
http://archives.postgresql.org/message-id/c64c5f8b0910252350w42f7ea13g52a6a88a86143374@mail.gmail.com

This is moving along nicely, and is now working (with some hacking) for 
the one use case I wanted it for.  High-level summary of what I think 
needs to get done before this is ready to commit:

1) Needs tab/space formatting cleaned up
2) "Execution of meta-command failed" errors are a small but serious problem
3) Should consider how :variable interpretation should work in a 
\[set]shell call
4) General code cleanup, including possible refactoring
5) Update pgbench docs to cover new calls.  I hoped to find time to help 
with this, it looks like I'm not going to have it in the near future.
6) Should do basic performance regression testing to confirm this patch 
doesn't impact pgbench results that don't use the new feature.  This 
I'll take care of, I'm not particularly worried about that based on 
what's been changed so far.

Details:

This revision is much improved over the earlier ones.  No problems 
applying the patch, and the basics work as expected.  One bit of code 
formatting nitpicking:  there are some spots where the code doesn't line 
up horizontally as expected.  I'm seeing lines spaced across with tabs 
mixed with ones where spaces were used, at least one line where I think 
you formatted presuming 8-character tabs.  That's kind of painful to 
expect a committer to clean up.  See 
http://wiki.postgresql.org/wiki/Developer_FAQ#What.27s_the_formatting_style_used_in_PostgreSQL_source_code.3F 
for more details about expectations here.

Attached are the scripts I used for testing the feature:

skewed-acct.pl : perl script that takes in the number of accounts and 
picks one with a skewed Pareto-ish distribution:  80% of the time, one 
from the first 20% of the accounts is picked.
skewed-select.sql : pgbench script that calls the script
skewed-test.sh : Little test program to confirm that the Perl code works 
as it's supposed to, for anyone who wants to hack on it for some reason 
(my Perl is miserable)

Here's what I did to test:

sudo ln -s `pwd`/skewed-acct.pl /usr/local/bin
createdb pgbench
pgbench -i -s 10 pgbench
pgbench -T 10 -j 4 -c 8 -S pgbench
pgbench -T 10 -j 4 -c 8 -f skewed-select.sql pgbench

Baseline gives me about 20K selects/second, with the shell call in the 
middle that dropped to around 400/second.  Since we know shell calls are 
expensive that's not too much of a surprise.  It does make an 
interesting statement about the overhead here though--if you want to 
instrument something you expect to execute thousands of times a second, 
that's probably not going to be possible using these calls.  I didn't 
try yet to see how small I could make the shell overhead smaller (using 
a simpler script instead of the current Perl one, minimizing the number 
of characters in the pgbench script)

To watch what was happening, I needed to toggle on "log_statement=all".  
That will confirm that the skew was working properly, which is good to 
see because it didn't as I originally wrote it.  There are two 
functional problems that jumped right out at me with the implementation.

First, sometimes the call fails.  On every run, I'm getting one or more 
of these (note that each of the 4 threads has two clients running 
against it, 0 and 1):

setshell: error getting parameterClient 0 aborted in state 1. Execution 
of meta-command failed.
setshell: error getting parameterClient 1 aborted in state 1. Execution 
of meta-command failed.

Right near the end.  Am guessing there's some last-transaction cleanup 
going awry.  Don't know why that is, and will be curious if it can be 
reproduced.

A much more insidious issues is that shell and setshell don't do 
interpretation of pgbench variables like ":naccounts".  The way I 
originally wrote skewed-select.sql (which you can still see commented 
out in the attached version) it looked like this:

\set naccounts 100000 * :scale
\setshell aid skewed-acct.pl :naccounts
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;

This didn't work through--that was calling the script with the literal 
text ":naccounts" rather than the value for that.  Similarly, when I 
tried to add some debugging bits to the end like this:

\shell echo :aid

That just printed ":aid" rather than the value.  I would like to see 
both of these work as written above (the versions commented out in the 
attached sample).

Now, it's possible to work around that limitation in some cases--the 
attached version just hard-codes in the number of accounts given the 
scale I was testing at.  This isn't really ideal though.  Unfortunately 
for you, the way :variable decoding is handling inside pgbench right now 
doesn't even have to consider things like quoting.  The variable 
decoding is done at the exact places it makes sense at, rather than 
being done more globally.  That means the existing replacement logic 
can't be re-used for this feature.  And we can certainly expect that 
shell commands might have a ":" in them anyway, so doing this right ends 
up leading toward things like making "::" be a ":" that doesn't get 
substituted.

Maybe this whole issue can be avoided as just being more complicated 
than this feature justifies.  I think people will be surprised, and find 
this not as useful as it could be, unless this is done though.

Also, this doesn't work

\setshell :aid skewed-acct.pl 1000000

Which I think will surprise people.  If there's a : as the first 
character of the second field here, it should just get clipped off.

As for more general code review, one thing that jumped out at me was this:

            if (fgets(res, 64, respipe) == NULL)

This should use sizeof(res) instead of hard-coding its size like that here.

So far as general structure goes, actually adding the variable 
substitution complexity to the shell/setshell code is going to make it 
even more complicated, and it already sticks out as badly fitting into 
doCustom as it is.  I suspect this capability will make for a really 
unmanagable result.  Maybe refactor the new shell stuff a bit into 
another subroutine now that you've finished the main guts?  doCustom 
feels like it's grown way beyond its original purpose here with this 
patch, it was on the edge of that before you started--and these two new 
commands are by far the most complicated.

-- 
Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com  www.2ndQuadrant.com

  2. Re: pgbench: new feature allowing to launch shell commands

    Michael Paquier <michael.paquier@gmail.com> — 2009-12-03T04:12:23Z

    Hi,
Sorry if you receive this email a second time, Greg, i didn't notice it has
not been sent to the hackers ML
Thanks for your first review.
I tried to work on most of the issues you noticed

> 1) Needs tab/space formatting cleaned up
This one is done, I adapted my environment to the Postgresql formats. I hope
there is nothing else more linked to that.
> 2) "Execution of meta-command failed" errors are a small but serious
problem
This error appears (n-1) times by using n threads with the j option. As you
said in your previous email there is some thread cleanup when one is
disconnected. This error still appears, I don't know yet which part of the
code is the origin of that. I needs more investigation.
> 3) Should consider how :variable interpretation should work in a
\[set]shell call
It is supported now. I implemented this, I made a test with your pearl
script, my own tests and it worked, at least no error appeared :)
> 4) General code cleanup, including possible refactoring
I didn't modify too much the code, I just noticed a couple of variables
unnecessary and some definitions not in adequacy with pgbench code. Btw,
what I did is included in the patch.
> 5) Update pgbench docs to cover new calls.  I hoped to find time to help
with this, it looks like I'm not going to have it in the near future.
I tried to update the document writing a couple of lines describing simply
the new possible calls setshell and shell. I am not that skilled at sgml
though.
> 6) Should do basic performance regression testing to confirm this patch
doesn't impact pgbench results that don't use the new feature.  This I'll
take care of, I'm not particularly worried about that based on what's been
changed so far.
Do you have an idea of what kind of tests could be done? I don't have so
much experience about common regression tests linked to pgbench.
I also added a second file including a couple of scripts written quickly
generating numbers based on the gauss and pareto density functions. It
cannot be used straightforwardly now, but still it can be a base for
something linked to setshell.

Regards,

Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center

  3. Re: pgbench: new feature allowing to launch shell commands

    Michael Paquier <michael.paquier@gmail.com> — 2009-12-04T00:41:39Z

    I didn't send the good patch yesterday. => --;
Here is the latest version.
Regards,

-- 
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center

  4. Re: pgbench: new feature allowing to launch shell commands

    Greg Smith <greg@2ndquadrant.com> — 2009-12-06T01:07:47Z

    Michael Paquier wrote:
> > 3) Should consider how :variable interpretation should work in a 
> \[set]shell call
> It is supported now. I implemented this, I made a test with your perl 
> script, my own tests and it worked, at least no error appeared :)
It looks like how you did this is a little less complicated than I had 
hoped for.  Let me show you some examples of how I think this should 
work.  Say naccounts = 1000000 and bid=123 already:

\setshell aid skew.sh :naccounts
    runs "skew.sh 1000000"

\setshell aid skew.sh a ::naccounts c
  runs "skew.sh a :naccounts c" - do not substitute the variable if "::" 
appears, just replace with ":".  Otherwise, there's no way to include a 
real ":" in the command line

\setshell aid skew.h aid :naccounts :bid
  runs "shew.sh 1000000 123"

 From looking at the code, I think that only case (1) is supported by 
the bits you added (haven't actually re-tested it since I know you're 
still working).  Also, having that same substitution logic work for both 
shell and setshell should would be nice here.

As far as reducing the amount of stuff in doCustom goes, I think what 
you want for this specific part is a subroutine you can pass a string 
that has some number of :variable references in it, returning a new 
string with them having the substituted values inserted in there.  
That's something I think it would be easier to get right as a standalone 
function first, and then both shell and setshell could use it.

 > Do you have an idea of what kind of tests could be done? I don't have 
so much experience
 > about common regression tests linked to pgbench.

None of the regression tests use pgbench yet, partly because contrib 
modules like it aren't necessarily even built before the main regression 
tests are run.  Also, it's hard to write a pgbench-based test using the 
current pg_regress framework.  The complete non-determinism on the TPS 
scores for example makes it hard to avoid having a diff against any 
standard regression result provided.  I think it would be nice to add a 
very complicated script that uses all these weird features for 
regression test purposes, but it's not so clear how we would enforce 
running it automatically to catch if a future change broke something.

As far as the rest of your concerns, once we get this to code complete 
and all the bugs squashed, I can take a pass at cleaning up your docs 
and making sure there's not any performance regression as part of my 
final review.  What you've added in there so far is good enough for now, 
I just didn't want to do the docs changes from scratch myself (and I 
thought it would be useful for you to get a bit of practice on that too, 
since they're usually required for patch submissions).  If you can make 
the three examples above all work, and get rid of the threading bug, 
I'll be clear to take care of docs review/performanc tests and then pass 
this over for a committer to look at.  It would be nice if the code was 
refactored a bit too first, just because it's less likely to be rejected 
for "the code is ugly" reasons if that's done first.  That sort of 
rejection is always a real possibility with this project, particularly 
for something like this where it's not as obvious to everyone what the 
feature is useful for.

-- 
Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com  www.2ndQuadrant.com

  5. Re: pgbench: new feature allowing to launch shell commands

    Michael Paquier <michael.paquier@gmail.com> — 2009-12-07T04:51:59Z

    Hi,

I took care of making the 3 cases you mentioned working in the new version
of the patch attached. It worked in my case for both shell and setshell
without any problem.
The code has also been reorganized so as to lighten the process in doCustom.
It looks cleaner on this part.

The only remaining issues are the thread bug and the documentation.
For the bug, I am currently looking into it.
I should take a little bit of time, I don't really know yet from where it
comes exactly...
For the documentation, I'll try to write it a little bit more once the code
issues are solved.

Regards

-- 
Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center

  6. Re: pgbench: new feature allowing to launch shell commands

    Michael Paquier <michael.paquier@gmail.com> — 2009-12-07T05:59:16Z

    The threading bug appears when a duration is set for pgbench tests.
Instead of a duration, if a number of xacts is set, this error doesn't
happen.

If i understood the problem well, when the alarm signal comes, all the
threads have to disconnect even the ones looking for a setshell parameter at
this moment, creating the thread error:
setshell: error getting parameter
Client 0 aborted in state 1. Execution of meta-command failed.

I am trying to find an elegant way to solve this, but I can't figure out yet
how to deal with this error as it looks tricky.

  7. Re: pgbench: new feature allowing to launch shell commands

    Michael Paquier <michael.paquier@gmail.com> — 2009-12-07T06:26:54Z

    Please find attached the latest version of the patch,
with the threading bug corrected and the documentation updated as well.

The origin of the bug was the alarm signal. Once the duration is over, all
the threads have to finish and timer_exceeded is set at true.
A control on this variable in setshell process is enough so as not to show
out the thread error.

Regards,

  8. Re: pgbench: new feature allowing to launch shell commands

    Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-12-09T06:12:32Z

    Michael Paquier <michael.paquier@gmail.com> wrote:

> Please find attached the latest version of the patch,
> with the threading bug corrected and the documentation updated as well.

Why do you use BUFFER_SIZE-1 for snprintf?
   snprintf(commandShell, SHELL_COMMAND_SIZE-1, ...)
Trailing nulls are also included in the length, so
   snprintf(commandShell, SHELL_COMMAND_SIZE, ...)
would be ok. (removed -1)

Other parts look fine, except an empty tag <replaceable></> in the
documentation. Is it a typo?

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

  9. Re: pgbench: new feature allowing to launch shell commands

    Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-12-10T08:21:44Z

    > Michael Paquier <michael.paquier@gmail.com> wrote:
> > Please find attached the latest version of the patch,
> > with the threading bug corrected and the documentation updated as well.

Please don't describe your-specific usage of shell command in the documentation

> Why do you use BUFFER_SIZE-1 for snprintf?
>    snprintf(commandShell, SHELL_COMMAND_SIZE-1, ...)
> Trailing nulls are also included in the length, so
>    snprintf(commandShell, SHELL_COMMAND_SIZE, ...)
> would be ok. (removed -1)

I found several bugs and matters.
  * The calcuration of the shell command length is wrong.
  * You cannot assign 0 to variables with \setshell meta command.
  * Comparison with "== true" is a bad manner.
  * You called \shell "shell command" and \setsell "shell script",
    but we don't need the difference. I think "shell command" is enough.

Heavily cleaned up patch attached. Please review it.

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

  10. Re: pgbench: new feature allowing to launch shell commands

    Greg Smith <greg@2ndquadrant.com> — 2009-12-14T20:48:59Z

    Takahiro Itagaki wrote:
> Heavily cleaned up patch attached. Please review it.
>   
This is almost there.  The refactoring work you both did is exactly how 
I was hoping this would end up looking, code-wise, and the feature set 
is basically feature complete except for one small UI concern I have.

Attached is an updated version of my skewed-select.sql test program, 
adjusted to take advantage of the now supported syntax.  Note that in 
order to run this properly, you need to pass the database scale into 
pgbench when you run it like this:

  pgbench -T 10 -j 4 -c 8 -s 10 -f skewed-select.sql pgbench

Because when you use a custom "-f" script, :scale isn't set otherwise.

In the current version of the patch, you can do this:

  \setshell aid skewed-acct.pl :naccounts

But you can't do this:

  \setshell :aid skewed-acct.pl :naccounts

What's worse is that you don't get an error in that second case--it just 
doesn't set the variable.  I thought this was fixed by one of Michael's 
versions here, so maybe this is a regression from the other clean-up?  
Since all the other ways you interact with this type of variable in 
pgbench require the ":", not supporting it in this context seems like a 
recipe for weird problems for those trying to use it.

If we can get an updated version where you can use either syntax for the 
variable name passed to setshell, this one will be ready for commit I 
think.  I was tempted to fix the bug myself, but I seem to be doing 
better as a tester on this patch rather than working on its 
development.  Everything else--docs, code--looks pretty good now. 

Only thing we might add is a warning that you're going to completely 
tank your pgbench results by using the high-overhead shell command if 
your test would otherwise be limited by CPU.  This feature is only 
really useful as far as performance testing goes if you're using it on 
an I/O bound test.  I have some more pgbench hints to apply at some 
point in the future, so it wouldn't be problem to skip this for now; I 
can bundle it into that section later.

-- 
Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com  www.2ndQuadrant.com

  11. Re: pgbench: new feature allowing to launch shell commands

    Michael Paquier <michael.paquier@gmail.com> — 2009-12-15T00:44:07Z

    Hi,

Yeah the grammar ":variable" is used to set a parameter, the user will feel
disorientated if there is no support.
The attached patch supports this new case, based on Itagaki-san's last
version. I also added a warning to tell the user that pgbench is slowing
down when using this feature.

If nobody is against it, I will mark it as ready to commit.

-- 
Regards,

Michael Paquier
NIPPON TELEGRAPH AND
TELEPHONE CORPORATION
NTT Open Source Software Center

  12. Re: pgbench: new feature allowing to launch shell commands

    Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-12-15T01:51:21Z

    Michael Paquier <michael.paquier@gmail.com> wrote:

> Yeah the grammar ":variable" is used to set a parameter, the user will feel
> disorientated if there is no support.
> The attached patch supports this new case, based on Itagaki-san's last
> version.

What is the spec of "\setshell :variable" ?
Literally interpreted, it declares a variable with name ":variable".
But pgbench cannot use such variables because only variables named
with alphabets, numbers, and _ can be accepted. Should we forbid to
assign variables that name contain invalid characters instead?

> I also added a warning to tell the user that pgbench is slowing
> down when using this feature.

This change seems to be nonsense. Do you want to fill your terminal
with such messages?


Proposed patch attached. It checks for variable names whether they
consist of isalnum or _. The check is only performed when a new variable
is assigned to avoid overhead. In normal workload, variables with the
same names are re-used repeatedly. I don't think the additinal check would
decrease performance of pgbench.

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

  13. Re: pgbench: new feature allowing to launch shell commands

    Greg Smith <greg@2ndquadrant.com> — 2009-12-15T06:09:55Z

    Takahiro Itagaki wrote:
> Michael Paquier <michael.paquier@gmail.com> wrote:
>
>   
>> Yeah the grammar ":variable" is used to set a parameter, the user will feel
>> disorientated if there is no support.
>> The attached patch supports this new case, based on Itagaki-san's last
>> version.
>>     
> What is the spec of "\setshell :variable" ?
> Literally interpreted, it declares a variable with name ":variable".
> But pgbench cannot use such variables because only variables named
> with alphabets, numbers, and _ can be accepted. Should we forbid to
> assign variables that name contain invalid characters instead?
>   
After reviewing this a bit more I've realized my idea wasn't very good 
here.  If this is what we're already accepting:

\set nbranches :scale

It's completely reasonable to say that *only* this works:

\setshell aid expr 1 + :naccounts

While this does not:

\setshell :aid expr 1 + :naccounts

And I was wrong to ask that it should.  Sorry to have lead you both 
around over this, my bad.

> Proposed patch attached. It checks for variable names whether they
> consist of isalnum or _. The check is only performed when a new variable
> is assigned to avoid overhead. In normal workload, variables with the
> same names are re-used repeatedly. I don't think the additinal check would
> decrease performance of pgbench.
>   
This is interesting, but we're already past where this should have 
wrapped up and I'm not sure if it's worth fiddling with this code path 
right now.  I think your idea is good, just outside of what we should 
fool with right now and I'm out of time to work on testing it further too.

How about this:  the version you updated at 
http://archives.postgresql.org/pgsql-hackers/2009-12/msg00847.php , your 
pgbenchshell_20091210.patch, worked perfectly for me except for one 
complaint I've since dropped.  How about we move toward committing that 
one, and maybe we look at this whole variable name handling improvement 
as a separate patch later if you think it's worth pursuing?  At this 
point I'd just like to have a version of the shell/setshell feature go 
into the pgbench code without dragging things out further into new 
territory.

--

Greg Smith    2ndQuadrant   Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com  www.2ndQuadrant.com

  14. Re: pgbench: new feature allowing to launch shell commands

    Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2009-12-15T06:23:12Z

    Greg Smith <greg@2ndquadrant.com> wrote:

> How about this:  the version you updated at 
> http://archives.postgresql.org/pgsql-hackers/2009-12/msg00847.php , your 
> pgbenchshell_20091210.patch, worked perfectly for me except for one 
> complaint I've since dropped.  How about we move toward committing that 
> one, and maybe we look at this whole variable name handling improvement 
> as a separate patch later if you think it's worth pursuing?

It's fine idea. I'll commit the previous lite version, and discuss
whether we need the difference patch for fool proof.

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