Re: pgbench: new feature allowing to launch shell commands

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