combine two small reads into one

[bgentry]

There's no need to read 1 byte and then immediately read 4 more, rather
than just reading 5 bytes to begin with. Also, with this change rxMsg is
no longer swallowing an error from ReadByte.

On a related note... How much performance profiling and tuning have you done on this package? I recently built que-go using pgx, and during my early benchmarks of it, it doesn't actually appear to be faster than the Ruby version (que). The profiling I've done so far suggests that the vast majority of the time in Go is spent in syscall.Syscall. Here's one of those CPU profiles:

I certainly can't say that this is the fault of pgx, but I'm curious how much perf work you've done.

Thanks!

[jackc]

I've done quite a bit of profiling and optimization several months ago. I used the other Go database adapters as a baseline (See https://github.com/jackc/go_db_bench). At the time pgx was measurably faster in most cases than the other Go adapters. I didn't do any comparison with Ruby.

There is some sort of issue with profiling on Mac (golang/go#6047), but it it doesn't look like it affected you. What you posted is what I would expect. If you notice in particular that it is the syscalls for read and write that are consuming the bulk of the runtime. This indicates that either pgx is not the bottleneck, or that pgx is making too many syscalls. Much of the optimization work I did previously involved minimizing read and write calls so I don't think the issue is unnecessary syscalls.

Given that you are getting the same performance with Go and with Ruby, and that Go is spending a large amount of time waiting for network IO, I suspect that the bottleneck may be PostgreSQL itself. It would be interesting to see the CPU usage of the Go app vs. the PostgreSQL server processes. If one of them is maxed out, and the other is not, that could indicate the bottleneck.

As far as this patch itself, do you have any benchmarks for how much faster it is? In that rxMsg function .r.reader is a *bufio.Reader so I would expect the separate read calls would doing internally about the same thing this patch does externally.

Thanks!

[bgentry]

Sorry for the delay here :)

I haven't had time to dig in much more to my performance issues, but it's good that you've done some profiling and optimization already. It's totally possible that this fix doesn't actually improve performance given that you're using a bufio.Reader. I'm still not sure that the Go and Ruby tests I was running are doing 100% the same thing, so that's where I'll focus more of my time to understand why the numbers don't match.

All that aside, it does seem like this change would be a positive, if only because of the fact that you're no longer swallowing a possible error. I'd certainly be confused if it actually worsened performance, but I have not benchmarked it so can't say for sure.

[jackc]

Good catch on the error. Merged.