Re: pgbench: new feature allowing to launch shell commands

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(at)2ndQuadrant(dot)com www.2ndQuadrant.com

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

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

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(at)2ndQuadrant(dot)com www.2ndQuadrant.com

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

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.

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,

Michael Paquier <michael(dot)paquier(at)gmail(dot)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

> Michael Paquier <michael(dot)paquier(at)gmail(dot)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

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(at)2ndQuadrant(dot)com www.2ndQuadrant.com

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

Michael Paquier <michael(dot)paquier(at)gmail(dot)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

Takahiro Itagaki wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)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(at)2ndQuadrant(dot)com www.2ndQuadrant.com

Greg Smith <greg(at)2ndquadrant(dot)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