Re: PATCH: pgbench - aggregation of info written into log

On 30 Srpen 2012, 23:47, Robert Haas wrote:
> On Thu, Aug 30, 2012 at 3:25 PM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
>> On 30 Srpen 2012, 18:02, Robert Haas wrote:
>>> On Fri, Aug 24, 2012 at 5:25 PM, Tomas Vondra <tv(at)fuzzy(dot)cz> wrote:
>>>> This patch is a bit less polished (and more complex) than the other
>>>> pgbench patch I've sent a while back, and I'm not sure how to handle
>>>> the
>>>> Windows branch. That needs to be fixed during the commit fest.
>>>
>>> What's the problem with the Windows branch?
>>
>> Well, there are comments about how timestamp does not work on Windows
>> (filling 0), and I'm not sure how that affects the patch (e.g.
>> determining
>> the aggregation interval). I have no Windows workstation available so I
>> can't actually try that.
>
> Hmm. That seems like it might be something we need to fix first...
>
>>> Could you un-cuddle your brances to follow the PostgreSQL style, omit
>>> braces around single-statement blocks, and remove the spurious
>>> whitespace changes?
>>
>> OK, will do.
>>
>>> Instead of having both use_log_agg and naggseconds, I think you can
>>> get by with just having a single variable that is zero if aggregation
>>> is not in use and is otherwise the aggregation period. On a related
>>> note, you can't specify -A without an associated value, so there is no
>>> point in documenting a "default". As with your other patch, I suggest
>>> using a long option name like --latency-aggregate-interval and then
>>> making the name of the variable in the code match the option name,
>>> with s/-/_/g, for clarity.
>>
>> Why --latency-aggregate-interval? It has nothing to do with latencies,
>> it's merely number of transactions per interval.
>
> Oh, I thought it was modifying the behavior of -l.

It does, but AFAIK the "-l" means logging. I suppose
"--aggregate-interval" would be a good option name, I don't see a reason
to put there the additional word when there are other aggregated values
(e.g. num of transactions).

T.