Re: [PATCH] pgbench --throttle (submission 7 - with lag measurement)

To clarify what state this is all in:  Fabien's latest 
pgbench-throttle-v15.patch is the ready for a committer version.  The 
last two revisions are just tweaking the comments at this point, and his 
version is more correct than my last one.

My little pgbench-delay-finish-v1.patch is a brand new bug fix of sorts 
that, while trivial, isn't ready for commit yet.  I'll start a separate 
e-mail thread and CF entry for that later.  Fabien has jumped into 
initial review comments of that already below, but the throttle feature 
isn't dependent on that.  The finish delay just proves that the latency 
spikes I was getting here aren't directly tied to the throttle feature.

On 7/14/13 5:42 PM, Fabien COELHO wrote:
> * ISTM that the impact of the chosen 1000 should appear somewhere.

I don't have a problem with that, but I didn't see that the little table 
you included was enough to do that.  I think if someone knows how this 
type of random generation works, they don't need the comment to analyze 
the impact.  And if they don't know, that comment alone wasn't enough to 
help them figure it out.  That's why I added some terms that might help 
point the right way for someone who wanted to search for more 
information instead.

The text of pgbench is not really the right place to put a lecture about 
how to generate numbers with a target probability distribution function.  Normally the code comments tries to recommend
referencesfor this sort 
 
of thing instead.  I didn't find a really good one in a quick search though.

> About 123456 12345 vs 123456.012345: My data parser is usually "gnuplot"
> or "my eyes", and in both cases the later option is better:-)

pgbench-tools uses gnuplot too.  If I were doing this again today from 
scratch, I would recommend using the epoch time format compatible with 
it you suggested.  I need to look into this whole topic a little more 
before we get into that though.  This patch just wasn't the right place 
to get into that change.

> About the little patch: Parameter "ok" should be renamed to something
> meaningful (say "do_finish"?).

It's saying if the connection finished "ok" or not.  I think exactly 
what is done with that information is an implementation detail that 
doesn't need to be exposed to the function interface.  We might change 
how this is tied to PQfinish later.

> Also, it seems that when timer is
> exceeded in doCustom it is called with true, but maybe you intended that
> it should be called with false??

The way timeouts are handled right now is a known messy thing.  Exactly 
what you should do with a client that has hit one isn't obvious.  Try 
again?  Close the connection and abort?  The code has a way it handles 
that now, and I didn't want to change it any.

> it is important to remove the connection because it serves as a marker
> to know whether a client must run or not.

This little hack moved around how clients finished enough to get rid of 
the weird issue with your patch I was bothered by.  You may be right 
that the change isn't really correct because of how the connection is 
compared to null as a way to see if it's active.  I initially added a 
more complicated "finished" state to the whole mess that tracked this 
more carefully.  I may need to return to that idea now.

-- 
Greg Smith   2ndQuadrant US    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com