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

Hi,

I'm looking into this as a committer.  It seems that this is the
newest patch and the reviewer(Pavel) stated that it is ready for
commit. However, I noticed that this patch adds a Linux/UNIX only
feature(not available on Windows). So I would like to ask cores if
this is ok or not.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp

> Hi,
> 
> attached is a v5 of this patch. Details below:
> 
> On 8.12.2012 16:33, Andres Freund wrote:
>> Hi Tomas,
>> 
>> On 2012-11-27 14:55:59 +0100, Pavel Stehule wrote:
>>>> attached is a v4 of the patch. There are not many changes, mostly some
>>>> simple tidying up, except for handling the Windows.
>> 
>> After a quick look I am not sure what all the talk about windows is
>> about? instr_time.h seems to provide all you need, even for windows? The
>> only issue of gettimeofday() for windows seems to be that it is that its
>> not all that fast an not too high precision, but that shouldn't be a
>> problem in this case?
>> 
>> Could you expand a bit on the problems?
> 
> As explained in the previous message, this is an existing problem with
> unavailable timestamp. I'm not very fond of adding Linux-only features,
> but fixing that was not the goal of this patch.
> 
>>>>> * I had a problem with doc
>> 
>> The current patch has conflict markers in the sgml source, there seems
>> to have been some unresolved merge. Maybe that's all that causes the
>> errors?
>> 
>> Whats your problem with setting up the doc toolchain?
> 
> Yeah, my fault because of incomplete merge. But the real culprit was a
> missing refsect2. Fixed.
> 
>> 
>>> issues:
>>>
>>> * empty lines with invisible chars (tabs) + and sometimes empty lines
>>> after and before {}
> 
> Fixed (I've removed the lines etc.)
> 
>>>
>>> * adjustment of start_time
>>>
>>> +                                               * the desired interval */
>>> +                                               while (agg->start_time
>>> + agg_interval < INSTR_TIME_GET_DOUBLE(now))
>>> +
>>> agg->start_time = agg->start_time + agg_interval;
>>>
>>> can "skip" one interval - so when transaction time will be larger or
>>> similar to agg_interval - then results can be strange. We have to know
>>> real length of interval
>> 
>> Could you post a patch that adresses these issues?
> 
> So, in the end I've rewritten the section advancing the start_time. Now
> it works so that when skipping an interval (because of a very long
> transaction), it will print lines even for those "empty" intervals.
> 
> So for example with a transaction file containing a single query
> 
>    SELECT pg_sleep(1.5);
> 
> and an interval length of 1 second, you'll get something like this:
> 
> 1355009677 0 0 0 0 0
> 1355009678 1 1501028 2253085056784 1501028 1501028
> 1355009679 0 0 0 0 0
> 1355009680 1 1500790 2252370624100 1500790 1500790
> 1355009681 1 1500723 2252169522729 1500723 1500723
> 1355009682 0 0 0 0 0
> 1355009683 1 1500719 2252157516961 1500719 1500719
> 1355009684 1 1500703 2252109494209 1500703 1500703
> 1355009685 0 0 0 0 0
> 
> which is IMHO the best way to deal with this.
> 
> I've fixed several minor issues, added a few comments.
> 
> regards
> Tomas