feat: support pgx in extended mode #82
Conversation
Adds support and tests for PGX in extended query mode. This also fixes some minor issues with sending bytea values in text format. bytea values in text format should use the newer hex format, as some newer drivers will not understand the older octal format. Also, any '\' characters in a literal in an array must be escaped. The Golang PGX driver will by default use extended mode in combination with DescribeStatement. The problem with DescribeStatement is that we currently do not have any way of really supporting it. We therefore need a patch in PGX to be able to work with it. However, that patch is also valuable for other users of PGX, and hopefully we will be able to get it merged in the main line of PGX.
| @@ -118,6 +118,9 @@ private boolean stringEquivalence(Code arrayElementType) { | |||
| */ | |||
| private String stringify(String value) { | |||
| if (this.isStringEquivalent) { | |||
| if (value.indexOf('\\') > -1) { | |||
| value = value.replace("\\", "\\\\"); | |||
There was a problem hiding this comment.
As these values are stringifyed, they also need to escape any backslashes inside the strings.
| @@ -63,7 +64,7 @@ public int getSqlType() { | |||
|
|
|||
| @Override | |||
| protected String stringParse() { | |||
| return this.item == null ? null : PGbytea.toPGString(this.item.toByteArray()); | |||
| return this.item == null ? null : "\\x" + Utils.toHexString(this.item.toByteArray()); | |||
There was a problem hiding this comment.
PGAdapter should use the newer hex format when sending bytea values as text: https://www.postgresql.org/docs/current/datatype-binary.html#id-1.5.7.12.9
pgx only supports this hex format, and will refuse the older escape format
| @@ -209,7 +209,7 @@ public static int toOid(Type type) { | |||
| case BYTES: | |||
| return Oid.BYTEA; | |||
| case TIMESTAMP: | |||
| return Oid.TIMESTAMP; | |||
| return Oid.TIMESTAMPTZ; | |||
There was a problem hiding this comment.
The timestamps that we return do contain a timezone.
| @@ -62,7 +62,7 @@ public short getParameterFormatCode(int index) { | |||
| @Override | |||
| public short getResultFormatCode(int index) { | |||
| if (this.resultFormatCodes == null || this.resultFormatCodes.isEmpty()) { | |||
| return 0; | |||
| return super.getResultFormatCode(index); | |||
There was a problem hiding this comment.
Bubble up to the super implementation. This does nothing at the moment, but we should implement a smarter default than just return 0 (text) when the client did not specify what it wants. That gives us the freedom to choose the binary format when we think that is better.
| // See https://github.com/googleapis/java-spanner/pull/1691 | ||
| resultSet.next(); | ||
| return new DescribeStatementMetadata(getParameterTypes(), resultSet); | ||
| if (PARSER.isQuery(this.sql)) { |
There was a problem hiding this comment.
We should not try to describe a DML using analyzeQuery. Instead, we should just return the parameter types.
src/test/golang/go.mod
Outdated
| golang.org/x/crypto v0.0.0-20210711020723-a769d52b0f97 // indirect | ||
| golang.org/x/text v0.3.6 // indirect | ||
| ) | ||
|
|
||
| replace github.com/jackc/pgx/v4 => github.com/olavloite/pgx/v4 v4.15.1 |
There was a problem hiding this comment.
This is the patch for the pgx driver that enables skipping the DescribeStatement message, as it is not needed. The JDBC driver already does this.
| @@ -305,7 +305,7 @@ public void testTimestampParsingTimestampValidityChecks() { | |||
| public void testBinaryParsing() { | |||
| ByteArray value = ByteArray.copyFrom(new byte[] {(byte) 0b01010101, (byte) 0b10101010}); | |||
| byte[] byteResult = {(byte) 0b01010101, (byte) 0b10101010}; | |||
| byte[] stringResult = {'U', '\\', '2', '5', '2'}; | |||
| byte[] stringResult = {'\\', 'x', '5', '5', 'a', 'a'}; | |||
There was a problem hiding this comment.
Changed to hex format from the old escape format for bytea values in text format.
| @@ -98,7 +106,9 @@ public static void compile() throws IOException, InterruptedException { | |||
|
|
|||
| private GoString createConnString() { | |||
| return new GoString( | |||
| String.format("postgres://uid:pwd@localhost:%d/test-db", pgServer.getLocalPort())); | |||
| String.format( | |||
| "postgres://uid:pwd@localhost:%d/test-db?statement_cache_capacity=0", | |||
There was a problem hiding this comment.
Do not use a local statement cache in the pgx driver. This is needed to enable skipping the DescribeStatement message.
| // merged. | ||
| // 1. DESCRIBE portal | ||
| // 2. EXECUTE portal | ||
| assertEquals(2, requests.size()); |
There was a problem hiding this comment.
Yay! One less round trip :-)
And when #80 is merged, it will be down to just one round trip.
|
Hey @olavloite, I observe two failures after merging this pr. Can you check those out? Failures: |
|
Where are you seeing this? I just merged the latest changes into this PR, and all tests were successful. |
I cloned support-pgx-extended-mode branch and run ITPgxTest locally. |
Adds support and tests for PGX in extended query mode. This also fixes
some minor issues with sending bytea values in text format. bytea values
in text format should use the newer hex format, as some newer drivers
will not understand the older octal format. Also, any '' characters in
a literal in an array must be escaped.
The Golang PGX driver will by default use extended mode in combination
with DescribeStatement. The problem with DescribeStatement is that we
currently do not have any way of really supporting it. We therefore need
a patch in PGX to be able to work with it. However, that patch is also
valuable for other users of PGX, and hopefully we will be able to get it
merged in the main line of PGX.