On Jul29, 2010, at 00:48 , Greg Smith wrote:
> Finally got around to taking a longer look at your patch, sorry about the delay here. Patch itself seems to work on
simpletests anyway (more on the one suspect bit below). You didn't show what the output looks like, so let's start with
thatbecause it is both kind of neat and not what I expected from your description. Here's the sort of extra stuff added
tothe end of the standard output when you toggle this feature on:
>
> <snipped output>
>
> From the way you described the patch, I had thought that you were just putting this information into the log files or
somethinglike that. In fact, it's not in the log files; it just shows up in this summary at the end. I'm not sure if
that'sbest or not. Some might want to see how the per-statement variation varies over time. Sort of torn on whether the
summaryalone is enough detail or not. Let me play with this some more and get back to you on that.
I think the two features are pretty much orthogonal, even though they'd make use of the same per-statement
instrumentationmachinery.
I created the patch to tune the wal_writer for the synchronous_commit=off case - the idea being that the COMMIT should
bevirtually instantaneous if the wal_writer writes old wal buffers out fast enough.
I haven't yet used pgbench's log output feature, so I can't judge how useful the additional of per-statement data to
thatlog is, and what the format should be. However, if you think it's useful and can come up with a sensible format,
I'dbe happy to add that feature to the patch.
> Now onto the nitpicking. With the standard Ubuntu compiler warnings on I get this:
>
> pgbench.c:1588: warning: ‘i’ may be used uninitialized in this function
>
> If you didn't see that, you may want to double-check how verbose you have your compiler setup to be; maybe you just
missedit (which is of course what reviewers are here for). Regardless, the troublesome bit is this:
>
> int i;
>
> commands = process_commands(&buf[i]);
Fixed. That was a leftover of the trimming and comment skipping logic, which my patch moves to process_command.
Updated patch is attached.
Thanks for your extensive review &
best regards,
Florian Pflug
