leave UUIDs as 16-byte binary for better compatibility

[saurik]

The current decode_value_bin converts UUIDs to their hex representation: in addition to not being a very efficient implementation of the conversion process (io:format and then list_to_binary), both of the Erlang UUID libraries I know of use 16-byte binaries as their native storage format; so, if you have code that internally uses UUIDs for anything, you likely end up finding yourself taking the hex representation being returned by pgsql and running it back through a UUID parser to get the actual/normal binary representation. If it isn't considered possible to just change this (as it would, of course, break existing users who are currently re-parsing the hex UUIDs), maybe this could be made an option? ;P (Does the library maybe do this because the text representation of a UUID happens to be the hex representation? I'd personally rather see those get parsed back to the binary format--again, for better compatibility with Erlang UUID libraries--than the other way around.)

[pguyot]

Indeed, changing this will break the API. The choice was made to match our own UUID library as well as PostgreSQL's text output representation of UUIDs.

https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/uuid.c#L59

PostgreSQL expects UUIDs as 16 bytes in the binary format, so you currently can pass 16 bytes binaries as bind parameters. For decoding UUIDs, an option would be required. Text UUIDs from PostgreSQL should then be converted to 16 bytes, while binary UUIDs would be passed as is to the Erlang client.

[saurik]

I am sorry, potentially due to just being too tired or something, I am having a difficult time understanding how to proceed based on your response. I was looking for one of 1) "no, this driver will not support that, maintain a private branch for your own use", 2) "sure, that sounds great, I have a particular way I want to do options, so I'll look into adding an option", 3) "sure, that sounds great, if you put together a patch I'll considering merging it" (potentially with some instruction or pointer for how you'd like to see the option provided), or 4) no response (which would have been perfectly reasonable, of course ;P). From your comment all I am so far able to gather is "if this were to do be done it would require an option" (which is pretty much what I said in my original comment ;P), but not what my next step should be to see that happen.

[pguyot]

Jay,

Sorry for not being clear. Basically, I meant we're not likely to change the default format because of our usage of the driver. As a matter of fact, all open feature requests are based on how to adapt the driver to other formats. And indeed, if we had infinite resources we could implement all these options ourselves (this is your 2).

We welcome pull requests and would gladly merge your changes if they respect the way the driver works, and look sound enough (typically with tests). I don't have any instruction regarding this beyond what has been discussed here (i.e. it ought to be an option), but this change seems trivial enough.

[tuscland]

I would like to contribute a change to the way UUIDs are treated in pgsql, however I don't see an easy way to make it happen as a runtime option. Please feel free to make suggestions.

[asabil]

I have created an experimental fork of pgsql that breaks the backward compatibility and avoid encoding UUIDs as hex strings among other things here https://github.com/soundrop/pgsql

I am also not sure how to make these changes in a backward compatible way. Maybe @pguyot could provide more insights?

[tuscland]

@asabil, looks nice!

[tuscland]

Hello @asabil,

I don't believe keep backward compatibility is a must.
While you are at it, what about removing deprecated (ODBC) APIs?

See https://github.com/soundrop/pgsql/blob/master/src/pgsql_connection.erl#L52

% Compatibility (deprecated) API
sql_query/2,
sql_query/3,
sql_query/4,
param_query/3,
param_query/4,
param_query/5,

Best,
Cam

[asabil]

These functions are used heavily in the test suite. I wanted to remove them, but then I would have had to fix all the tests

[pguyot]

A way to avoid breaking backward compatibility could consist in passing an option in open function, and then pass it around like integer_datetimes. Ideally, the IntegerDateTimes argument could be replaced with a record of encoding/decoding options. As you can see, it is passed to pgsql_protocol:encode_bind_message/6 for encoding and pgsql_protocol:decode_row/4 for decoding.

We're not likely to merge changes breaking backward compatibility.