Re: pgbench: new feature allowing to launch shell commands - Mailing list pgsql-hackers

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