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 references for 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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com