Re: Patch to show individual statement latencies in pgbench output

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 simple tests anyway (more on the one suspect bit below). You didn't show what the output looks like, so let's start with that because it is both kind of neat and not what I expected from your description. Here's the sort of extra stuff added to the 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 something like 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's best or not. Some might want to see how the per-statement variation varies over time. Sort of torn on whether the summary alone 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 instrumentation machinery.

I created the patch to tune the wal_writer for the synchronous_commit=off case - the idea being that the COMMIT should be virtually 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 that log is, and what the format should be. However, if you think it's useful and can come up with a sensible format, I'd be 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 missed it (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