Add support for binary_mode

[johto]

Here's my work so far towards making #209 happen. It passes all tests with both binary_mode off and on, which suggests that we don't have enough tests.

This functionality is split into three separate commits:

1. Refactor writeBuf into something which can be used to send multiple messages in a single write() system call. This should reduce the overhead a bit when there are a lot of threads writing at the same time.
2. Decoupling rows and stmt so that a rows object can live without having an associated stmt object. Needed for the single round trip mode.
3. The implementation of binary_mode, where all []byte values are sent over and marked to be "binary", and everything else is sent over in text. Something like this is necessary since if we want to do only a single round-trip to the server per a call to *sql.Query(), we don't know the SQL types of the input parameters.

There's still likely a lot of work to do:

1. There's no documentation
2. There are no additional tests
3. This might break some driver.Valuers which return a byte slice which isn't actually valid as input for the type in binary mode. If we can adopt a general guideline of "string means text, []byte means binary" then things might work out great, but e.g. allocation penalties of such a guideline are not clear to me. This has some impact on e.g. the ongoing work for supporting arrays. Another approach might be to give up on binary mode altogether and instead always send everything over as text, but provide a special Valuer for bytea values (similarly to how we're likely going to have to have a separate values for bytea arrays). Both approached would

I'd really like to stress point 3 directly above; this is not the only way to achieve single roundtrip mode, and at the moment I'm not convinced it's the best one, either. The performance gains from using binary mode have been in my tests almost negligible (though I've mostly tested with bytea values, since this patch doesn't use binary mode for e.g. ints even though it theoretically could), so don't get too hung up on that.

Any thoughts?

[cbandy]

... but provide a special Valuer for bytea values (similarly to how we're likely going to have to have a separate values for bytea arrays). Both approached would

I think some of your comment was lost.

[cgilling]

Hi @johto,

I'm going to try and summarize how I understand things to make sure that I am understanding your concerns correctly.

The problem

It seems to me that the major worry here is that because we are doing single round trip, we can't get any feedback from the server as to what types we should encode our parameters as. If we just assume the type based on the go type, then we will be breaking backwards compatibility because there are some people who rely on being able to pass a []byte into a parameter that is a string, and assume that the library will encode it correctly (or vice versa, or []byte -> UUID)

Current Solution

Since we have a binary_mode now, that is off by default, when it is on, we can now make assumptions of the type based on the go type, and therefore we can encode it correctly without having to get the type information back from the server. This will break backwards compatibility so it is off by default.

My thoughts

I agree that there isn't really a need to use the binary encodings to do this, but rather a way to tell the client that we want it to assume how to encode parameters based on the go type rather than the type on the postgres server. Am I right in assuming that the type that is a problem here is the bytea type, because you can't just send that as a string and have postgres interpret correctly how to do with it.

If this is the case, perhaps rather than having binary_mode we could have something like assume_type or infer_types mode that mean that the user of the client must pass values in a workable type. i.e. a string must be a string and a bytea must be a []byte and a UUID must be a string as we (not so sure about that one). This would allow us to keep using the text protocol if we want and also still have some leeway with being able to pass a string in for integer type parameters as well because we know the postgres server will interpret that correctly.

[cbandy]

"string means text, []byte means binary" then things might work out great,

Right now, this seems like the most sane approach to me. Can you also outline what would happen to the other driver.Value types: int, float, bool and Time?

This has some impact on e.g. the ongoing work for supporting arrays.

Indeed. I'm having trouble finding documentation about the binary format of various types, but I suspect we can discover a lot with a few tests.

The performance gains from using binary mode have been in my tests almost negligible (though I've mostly tested with bytea values,

Interesting. I would have thought skipping hex-encoding would be noticable. Were the values very large?

I think the binary format will really shine with temporal types, which send/recv as int64 (or float8, depending on integer_datetimes ParameterStatus...) I'm too tired to think it through: does ParameterStatus preclude efficient valuing of temporal types?

rather than having binary_mode we could have something like assume_type or infer_types mode that mean that the user of the client must pass values in a workable type

We should also not forget about receiving/scanning values in binary format, or should that be a separate discussion?

a UUID must be a string as we (not so sure about that one).

UUID is a type I care about as well. My initial research indicates the binary format is 16 bytes.

[johto]

I'm going to try and summarize how I understand things to make sure that I am understanding your concerns correctly.

Your summary looks correct to me.

I agree that there isn't really a need to use the binary encodings to do this, but rather a way to tell the client that we want it to assume how to encode parameters based on the go type rather than the type on the postgres server. Am I right in assuming that the type that is a problem here is the bytea type, because you can't just send that as a string and have postgres interpret correctly how to do with it.

Yes, bytea is pretty much the only problem here. All other common types either work nicely, or don't work at all without a Valuer.

If this is the case, perhaps rather than having binary_mode we could have something like assume_type or infer_types mode that mean that the user of the client must pass values in a workable type. i.e. a string must be a string and a bytea must be a []byte and a UUID must be a string as we (not so sure about that one). This would allow us to keep using the text protocol if we want and also still have some leeway with being able to pass a string in for integer type parameters as well because we know the postgres server will interpret that correctly.

I think that's worse than what this PR does. That's essentially the same as saying "[]byte is always bytea, everything else should be a string", which is a proper subset of the functionality here. If we define []byte to be the binary representation of the type, the behavior in the bytea case is the exact same, except as an additional nicety the user can choose to use binary mode for e.g. her own custom types. The downside is that there is already code out there which assumes that []byte and string are interchangeable.

The only two options I'm considering right now are (optional, as implemented here) binary_mode, or a new mode where everything is assumed to be a string, and even bytea values need a Valuer.

[johto]

"string means text, []byte means binary" then things might work out great,

Right now, this seems like the most sane approach to me. Can you also outline what would happen to the other driver.Value types: int, float, bool and Time?

Right now, they're just sent over as text. It wouldn't be too difficult to send them over in binary, but the problem is that can really screw up programs which worked previously. E.g:

package main

import (
    _ "github.com/lib/pq"
    "database/sql"
    "fmt"
)

func main() {
    db, err := sql.Open("postgres", "sslmode=disable host=/tmp binary_mode=on")
    if err != nil {
        panic(err)
    }
    err = db.Ping()
    if err != nil {
        panic(err)
    }
    var v int64
    // assume:  begin; create table foo(bar int4); insert into foo values (1); commit;
    err = db.QueryRow(`SELECT 1 FROM foo WHERE bar = $1`, int32(1)).Scan(&v)
    fmt.Println(err)
}

will produce pq: incorrect binary data format in bind parameter 1, because database/sql only allows int64s to come through, and postgres is expecting an int32 parameter.

rather than having binary_mode we could have something like assume_type or infer_types mode that mean that the user of the client must pass values in a workable type

We should also not forget about receiving/scanning values in binary format, or should that be a separate discussion?

I think the way postgres does this is quite useless. You can only choose to either get all types as binary, or all types as text, so you'd have to also know how to decode values from any types created by extensions you're possibly using. Combine this with the fact that oids of said types change between installations, and you've got yourself a mess.

If there was a way to specify that e.g. only built-in data types should be sent over as binary, and then maybe apps could opt-in for other types if they know which oid is which type and know how to decode, it might be sensible to use the binary mode for receiving values from the database. But given the interface we currently have to work with, I wouldn't spend even a minute on trying to make anything happen on that front.

[johto]

... but provide a special Valuer for bytea values (similarly to how we're likely going to have to have a separate values for bytea arrays). Both approached would

I think some of your comment was lost.

Yeah, I think that was supposed to say "Both approaches would break backwards compatibility" or something similar.

[cbandy]

database/sql only allows int64s to come through, and postgres is expecting an int32 parameter.

Yeesh, makes sense. Only Values that ... remain untouched, really, can be sent in binary format without a ParameterDescription.

I think the way postgres does this is quite useless. You can only choose to either get all types as binary, or all types as text,

The Bind message allows the format of each result column to be specified, but of course we'd need the RowDescription to make it work.

So, I dislike the name "binary_mode" for what is really single-round-trip. With two round trips, we can do lots of things with binary formats.

[cbandy]

This doesn't do much waiting. It looks a lot more like the read* methods (one or two expected messages, else errorf.)

[cbandy]

These small methods are starting to look a lot more like a state machine. Does Postgres publish anything more ... structured about acceptable states/transitions than the Message Flow documentation?

[johto]

I think the way postgres does this is quite useless. You can only choose to either get all types as binary, or all types as text,

The Bind message allows the format of each result column to be specified, but of course we'd need the RowDescription to make it work.

Oh, sorry, you're right. So we could actually do something like this for proper prepared statements, just not in the single round-trip mode. It could be interesting to see whether that would affect performance or not.

[johto]

So, I dislike the name "binary_mode" for what is really single-round-trip. With two round trips, we can do lots of things with binary formats.

Right. Thing is, with two round-trips we can do all of that transparently, and there's no need to have a user-visible mode. "[]byte means binary input" is something that's easier to explain to a user than "single round-trip mode", I think.

[cbandy]

with two round-trips we can do all of that transparently

Almost, I think? A Valuer is supposed to generate the representation for the driver, but we have two representations: text and binary.

"[]byte means binary input"

I think this "binary mode" heuristic is excellent; I really want to explore what can be done with it.

"[]byte means binary input" is something that's easier to explain to a user than "single round-trip mode"

This may be necessary to support single-round-trip, but I'm not yet convinced it is sufficient to trigger single-round-trip. Your example for int32 highlights my concerns. I suspect there are cases where single-round-trip is not the right choice (and I don't like making poor choices on behalf of the user.)

I need to think about the Value types and the lack of a ParameterDescription some more.

[cbandy]

Not true. If I understand correctly,

• A RowDescription after Query or Bind includes the text/binary format indicators; in both cases a portal exists.
• Otherwise, we must expect retrieved values in the formats specified in Bind.

[johto]

Right. But there's currently only support for text mode, so that shouldn't be an issue yet.

[cgilling]

With regards to whether to require Valuers for everything or just send everything as a string to postgres except for []byte, I would vote for the second option because it is a much more usable. If in this mode people don't like the way something is being handled (i.e they store UUID as []byte), then a Valuer could be used to wrap the []byte and convert it to the right format to send over the wire to postgres).

I think encoding everything in the binary format is much more complicated given the troubles you ran into with int64 vs postgres wanting 32-bit. I think the big win is a single round trip, and encoding everything in binary format could be something that is addressed later if it is needed.

[cbandy]

I believe "binary mode" is misleading because this approach precludes the sending of binary formats by the backend. I'm okay with this option being called "binary parameters".

[johto]

This may be necessary to support single-round-trip, but I'm not yet convinced it is sufficient to trigger single-round-trip. Your example for int32 highlights my concerns. I suspect there are cases where single-round-trip is not the right choice (and I don't like making poor choices on behalf of the user.)

I'm fine with just never sending ints in binary with the non-prepared Query(). It's never going to be slower to do a one round-trip with integers sent over as text compared to two round-trips with integers sent over as binary. And if you really want the binary optimizations, you can always Prepare().

[johto]

I believe "binary mode" is misleading because this approach precludes the sending of binary formats by the backend. I'm okay with this option being called "binary parameters".

binary_parameters works for me.

[johto]

with two round-trips we can do all of that transparently

Almost, I think? A Valuer is supposed to generate the representation for the driver, but we have two representations: text and binary.

I'm not sure whether it would ever be sensible for a Valuer to emit binary representations.

[cbandy]

A Valuer is supposed to generate the representation for the driver, but we have two representations: text and binary.

I'm not sure whether it would ever be sensible for a Valuer to emit binary representations.

You are probably right!

[cgilling]

Anything I can do to help push this guy along? Are there tests that I could help write?

[johto]

Anything I can do to help push this guy along? Are there tests that I could help write?

It's mostly missing docs and tests, and I'd be glad if you wanted to help with those.

That said, I have the next four days off so I'm likely going to work on this over those days. Which one would you prefer to work on, so we don't step on each others' toes too much? (Or I guess you can also just sit back and relax if you want.)

[cgilling]

I won't have time till this weekend, I'll check in then to see whats left to do. I think if you worked on the docs first, it would make clear exactly what situations we want to test for and would give me more guidance as to what areas/conditions to test. So if you got that done by the weekend I could pick up some tests then.

[johto]

I won't have time till this weekend, I'll check in then to see whats left to do. I think if you worked on the docs first, it would make clear exactly what situations we want to test for and would give me more guidance as to what areas/conditions to test. So if you got that done by the weekend I could pick up some tests then.

Sounds good to me.

[cgilling]

@johto, any direction as to what I should focus on? if not I'll look and see what tests I can contribute.

[johto]

Sorry, I had way less time available than I expected during last weekend, and I poured all of it into improving Listener and ListenerConn (and didn't finish even that).

@johto, any direction as to what I should focus on? if not I'll look and see what tests I can contribute.

Hmm. I think the way this should work is to have a test-only environment variable which controls whether to use binary mode in tests or not, and then run the tests with binary_mode both on and off in Travis. I think there was one test which didn't work with binary mode on, so that test would either have to be fixed to work with both (possibly not a good idea, I forget which exact test it was). That should cover a lot of the new code.

Then on top of that, a couple of tests for cases which only work with binary_parameters (forcing it on regardless of the env var), e.g. sending a binary []byte blob into a SELECT $1::uuid. And perhaps some error conditions, such as SELECT $1::int with an 8-byte []byte.

Does that sound reasonable?

[johto]

I think there was one test which didn't work with binary mode on, so that test would either have to be fixed to work with both (possibly not a good idea, I forget which exact test it was).

Didn't quite finish my thought there: either fixed, or just t.Skipped if the env var is set.

[cgilling]

Looks like I'll finally have some time to start tackling this today. Just a quick question as I haven't worked on pull requests from other people's forks before. Do you have a recommended way of doing this. I currently have my own fork, and then I cloned that and added yours as a remote so I could pull down the branch. Should I issue to pull request to your branch with my test changes when I'm done?

[johto]

I currently have my own fork, and then I cloned that and added yours as a remote so I could pull down the branch. Should I issue to pull request to your branch with my test changes when I'm done?

I think it's easier if you just tell where I can find the branch and we can work based on commit hashes. E.g. you can say here "pushed [fixes] in 1a2b3c".

[cgilling]

Here is an initial stab cgilling@e5debbe

A few things to note:

• I tested the .travis.yml change with my travisci account
• I didn't find any of the current tests failed when I implemented the binary_mode environment variable test
• bytea -> uuid failed on binary_mode but not on text mode
• I experimented with the two different ways of handling situations failing with binary_mode in TestByteaToInt I check to make sure that the query actually errors out, but in TestByteaToUUID I just skip the test. Not sure which one I actually prefer. The first one seems more thorough, but is definitely uglier and I'm not sure how much it really buys us. Lemme know what you think and I'll change them both to whatever method we want to use.
• I tried to think of a way to get bytea -> text to fail but I couldn't. I tried using Hello 世界 rather than Hello World thinking that maybe the use of non ascii might break something, but it didn't so I didn't leave it in the test.

I need to step away for a while, hopefully will have some more time this weekend to try to think of more cases that would be good to test.

[johto]

The first one seems more thorough, but is definitely uglier and I'm not sure how much it really buys us.

Even if it's not that much, it's just two tests. I'd go the full mile and check the SQLSTATE of the error so that not any error would do.

[johto]

Can we somehow add PGTEST_BINARY_MODE to the matrix so that it's clearer which combination failed in Travis? It's been a while since I've touched Travis, but my recollection is that this should be possible.

Though it'll make the tests run a bit slower due to all of the extra setup/teardown work, but I'd expect there to be a different way of solving that problem.

[johto]

I should have some more time and energy this weekend (it's been a crazy week so far). I'll be focusing on getting this and #359 in before Monday.

[cgilling]

I updated my changes with the feedback you gave. cgilling@49990e3

Unfortunately I don't think I have time today to read through the postgres binary format docs to see if there are any more test cases that I should add. Can you think of anymore off the top of your head?

[johto]

I've been looking at this again, and I'm kind of unhappy about how it rewrites practically the entire code for handling prepared statements and query results. But I guess ultimately this is more the way we want the code to look like, so maybe that's not so bad.

I wonder if this should still be split into multiple commits, where each just takes one part of the code and turns it into the "new" state machine like way of handling the protocol messages. Changes like that should be more easily reviewable, in case something goes wrong and we have to debug this mess a month down the road.

[pkieltyka]

lots of tests help too :) .. a shared test suite for running queries could work well

[johto]

I updated my changes with the feedback you gave. cgilling/pq@49990e3

Unfortunately I don't think I have time today to read through the postgres binary format docs to see if there are any more test cases that I should add. Can you think of anymore off the top of your head?

Not really. I'm happy with these tests if you are.

[cbandy]

I wonder if this should still be split into multiple commits, where each just takes one part of the code and turns it into the "new" state machine like way of handling the protocol messages. Changes like that should be more easily reviewable, in case something goes wrong and we have to debug this mess a month down the road.

Yes, especially if they are just setup/prep for the actual feature here. IMO, refactors like that should not touch tests and should be merged before this feature changes any behavior.

[cgilling]

I agree that refactors should be separate commits when possible but I think that requiring them to be separate pull requests might actually discourage refactoring. The reason I think this is because most refactoring will happen as part of trying to implement something else. This pull request is a good example of that. The reason there was a refactor is because the same code was suddenly needed in multiple places so rather than duplicating code, it was refactored into some common functions for easy re-use. And writeBuf was refactored to make it easier to work with to send multiple commands in a single request.

I would think that the refactor being in a separate commit that can be shown has passing tests would be sufficient, perhaps even with more tests added in places that test coverage was found to be lacking.

@johto, is there anything I can do to help push this along? If you won't be able to get to this for a while I could work on splitting up the code into multiple commits.

[johto]

I'm hoping to commit #364 in some form today, and then move on to finishing this one.

[johto]

Well that plan didn't work out quite that well. I've now implemented this PR in a private branch which I'll push today or tomorrow. How would you like to get acknowledged, cgilling? Just as "cgilling", or with your real name? If the latter, what is it?

[cgilling]

Great news to hear about the progress. Thanks for doing the bulk of the work. Really looking forward to starting to use it. My name is Chris Gilling, I suppose using my real name would be a good idea. Thanks again.

[johto]

I've pushed this in 2997d16, albeit without docs. I can't finish the docs before next week, and I'm tired of having the code sitting on my hard drive unused.

Please give it a go and report any problems you might experience. Thanks for your patience!

[cgilling]

btw, my coworker mentioned this fix in a lightning talk he gave at gophercon: https://sourcegraph.com/blog/live/gophercon2015/123745231430 (the summary isn't really accurate to the main point of his talk)

[johto]

Cool! Are the problems fixed now? I haven't had the chance to play around with this code yet personally :-(

[cgilling]

It definitely works with pgbouncer in transaction mode and we haven't run into any problems. We're now using it for all our postgres db stuff in go, so it passes all our automated tests (unfortunately, haven't gotten any of them to production yet)

[bheemreddy181-zz]

this link is broken https://sourcegraph.com/blog/live/gophercon2015/123745231430

[sjmtan]

It definitely works with pgbouncer in transaction mode and we haven't run into any problems. We're now using it for all our postgres db stuff in go, so it passes all our automated tests (unfortunately, haven't gotten any of them to production yet)

@cgilling - I was wondering, its been 5 years, how has the binary_mode held up? Does it still work for proxies? I ask because it isn't currently documented here: https://pkg.go.dev/github.com/lib/pq#section-documentation

But I would love to use it if its still working :)

[sjmtan]

Oh - I see that binary_parameters is documented, but it still doesn't quite mention if it solves this use case.

[cgilling]

@shannontan-bolt

It has been working. The main reason we wanted it was because the implementation of it played nicely with pgbouncer running in transaction mode, whereas the standard mode doesn't because it would make two round trips which could go to two different connections.

That being said, somewhere in the past ~6 month we migrated over to pgx which is recommended by lib/pq itself: https://github.com/lib/pq#status. That works with pgbouncer in transaction mode out of the box, so I would look to moving over to use that. IIRC it wasn't very much work to move over.

[sjmtan]

Awesome! Thanks so much, @cgilling. I saw that there were some gotchas around JSONB, but with the exception of that, did you have to take into account anything else?

I'm also considering moving over to jackc/pgx - although I think you still need to set some of the statement caching/protocol behaviors. It is a bit harder than I expected though, not quite as drag and drop because we use an ORM that uses lib/pq (GORM v1, GORM v2 uses jackc/pgx).