Re: PATCH: pgbench - merging transaction logs

Sun, 15 Mar 2015 11:22:01 +0100 (CET)
Hello Tomas,

> attached is a patch implementing merging of pgbench logs. These logs are
> written by each thread, so with N threads you get N files with names
>
> pgbench_log.PID
> pgbench_log.PID.1
> ...
> pgbench_log.PID.N
>
> Before analyzing these logs, these files need to be combined. I usually
> ended up wrinting ad-hoc scripts doing that, lost them, written them
> again and so on over and over again.

I've looked at the patch. Although I think that such a feature is somehow
desirable... I have two issues with it: ISTM that

(1) it does not belong to pgbench as such

(2) even if, the implementation is not right

About (1):

I think that this functionnality as implemented does not belong to
pgbench, and does really belong to an external script, which happen not to
be readily available, which is a shame. PostgreSQL should probably provide
such a script, or make it easy to find.

It could belong to pgbench if pgbench was *generating* the merged log file
directly. However I'm not sure this can be done cleanly for the
multi-process "thread emulation" (IN PASSING: which I think this should
really get rid of because I do not know what system does not provide
threads nowadays and would require to have a state of the art pgbench
nevertheless, and this emulation significantly complexify the code by
making things uselessly difficult and/or needed to be implemented twice or
not be provided in some cases).

Another issue raised by your patch is that the log format may be improved,
say by providing a one-field timestamp at the beginning of the line.

About (2):

In the implementation you reimplement a partial merge sort by parsing each
line of each file and merging it with the current result over and over.
ISTM that an implementation should read all files in parallel and merge
them in one pass. The current IO complexity is in p²n where it should be
simply pn... do not use it with a significant number of threads and many
lines... Ok, the generated files are likely to be kept in cache, but
nevertheless.

Also there are plenty of copy paste when scanning for two files, and then
reprinting in all the different formats. The same logic is implemented
twice, once for simple and once for aggregated. This means that updating
or extending the log format later on would require to modify these scans
and prints in many places.

For aggregates, some data in the output may be special values "NaN/-/...",
I am not sure how the implementation would behave in such cases. As lines
that do not match are silently ignored, the result merge would just be non
significant.... should it rather be an error? Try it with a low rate for
instance.

It seems that system function calls are not tested for errors.

> The other disadvantage of the external scripts is that you have to pass
> all the info about the logs (whether the logs are aggregated, whther
> there's throttling, etc.).

I think that is another argument to make a better format, with the a
timestamp ahead? Also, ISTM that it only needs to know whether it is
merging aggregate or simple logs, no more, the other information can be
infered by the number of fields on the line.

> So integrating this into pgbench directly seems like a better approach,
> and the attached patch implements that.

You guessed that I disagree. Note that this is only my own opinion.

In summary, I think that:

(a) providing a clean script would be nice,

(b) if we could get rid of the "thread emulation", pgbench could generate
the merged logs directly and simply, and the option could be provided
then.

--
Fabien.