Re: pgbench: new feature allowing to launch shell commands

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