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(at)gregsmith(dot)com http://www.gregsmith.com Baltimore, MD