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

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?

Well, in the current pgbench.c, there's this piece of code

#ifndef WIN32
/* This is more than we really ought to know about instr_time */
fprintf(logfile, "%d %d %.0f %d %ld %ld\n",
st->id, st->cnt, usec, st->use_file,
(long) now.tv_sec, (long) now.tv_usec);
#else
/* On Windows, instr_time doesn't provide a timestamp anyway */
fprintf(logfile, "%d %d %.0f %d 0 0\n",
st->id, st->cnt, usec, st->use_file);
#endif

and the Windows-related discussion in this patch is about the same issue.

As I understand it the problem seems to be that INSTR_TIME_SET_CURRENT
does not provide the timestamp at all.

I was thinking about improving this by combining gettimeofday and
INSTR_TIME, but I have no Windows box to test it on :-(

>
>>>> * 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?

Ah, I see. Probably my fault, I'll fix that.

> Whats your problem with setting up the doc toolchain?
>
>> issues:
>>
>> * empty lines with invisible chars (tabs) + and sometimes empty lines
>> after and before {}
>>
>> * 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?

Yes, I'll work on it today/tomorrow and I'll send an improved patch.

Tomas