Re: [PATCH] pgbench: new feature allowing to launch shell commands

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