From f7fe860c13aa9ed08d91d513ef44f70e126b8c41 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 21 Apr 2021 17:59:51 -0400 Subject: [PATCH] sql/pgwire: fix encoding of decimals with trailing 0s The dscale is defined as number of digits (in base 10) visible after the decimal separator," so a negative dscale makes no sense and is not valid. This was preventing NPGSQL from being able to decode the binary decimals that CockroachDB was sending. Release note (bug fix): The binary encoding of decimals no longer will have negative `dscale` values. This was preventing Npgsql from being able to read some binary decimals from CockroachDB. --- pkg/cmd/generate-binary/main.go | 5 +++ pkg/sql/pgwire/encoding_test.go | 2 +- pkg/sql/pgwire/testdata/encodings.json | 35 +++++++++++++++++++ pkg/sql/pgwire/testdata/pgtest/decimal | 48 ++++++++++++++++++++++++++ pkg/sql/pgwire/types.go | 6 ++++ 5 files changed, 95 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/generate-binary/main.go b/pkg/cmd/generate-binary/main.go index 982dd4728a8e..4e9d5b0f172d 100644 --- a/pkg/cmd/generate-binary/main.go +++ b/pkg/cmd/generate-binary/main.go @@ -200,6 +200,11 @@ var inputs = map[string][]string{ "42.0", "420000", "420000.0", + "6000500000000.0000000", + "10000", + "800000000", + "9E+4", + "99E100", }, "'%s'::float8": { diff --git a/pkg/sql/pgwire/encoding_test.go b/pkg/sql/pgwire/encoding_test.go index a750c0b7b76f..e2bd2ae813d7 100644 --- a/pkg/sql/pgwire/encoding_test.go +++ b/pkg/sql/pgwire/encoding_test.go @@ -156,7 +156,7 @@ func TestEncodings(t *testing.T) { } got := verifyLen(t) if !bytes.Equal(got, tc.TextAsBinary) { - t.Errorf("unexpected text encoding:\n\t%q found,\n\t%q expected", got, tc.Text) + t.Errorf("unexpected text encoding:\n\t%q found,\n\t%q expected", got, tc.TextAsBinary) } } }) diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json index 708ef742aa9b..e1827a2d978c 100644 --- a/pkg/sql/pgwire/testdata/encodings.json +++ b/pkg/sql/pgwire/testdata/encodings.json @@ -580,6 +580,41 @@ "TextAsBinary": [52, 50, 48, 48, 48, 48, 46, 48], "Binary": [0, 1, 0, 1, 0, 0, 0, 1, 0, 42] }, + { + "SQL": "'6000500000000.0000000'::decimal", + "Oid": 1700, + "Text": "6000500000000.0000000", + "TextAsBinary": [54, 48, 48, 48, 53, 48, 48, 48, 48, 48, 48, 48, 48, 46, 48, 48, 48, 48, 48, 48, 48], + "Binary": [0, 2, 0, 3, 0, 0, 0, 7, 0, 6, 0, 5] + }, + { + "SQL": "'10000'::decimal", + "Oid": 1700, + "Text": "10000", + "TextAsBinary": [49, 48, 48, 48, 48], + "Binary": [0, 1, 0, 1, 0, 0, 0, 0, 0, 1] + }, + { + "SQL": "'800000000'::decimal", + "Oid": 1700, + "Text": "800000000", + "TextAsBinary": [56, 48, 48, 48, 48, 48, 48, 48, 48], + "Binary": [0, 1, 0, 2, 0, 0, 0, 0, 0, 8] + }, + { + "SQL": "'9E+4'::decimal", + "Oid": 1700, + "Text": "9E+4", + "TextAsBinary": [57, 69, 43, 52], + "Binary": [0, 1, 0, 1, 0, 0, 0, 0, 0, 9] + }, + { + "SQL": "'99E100'::decimal", + "Oid": 1700, + "Text": "9.9E+101", + "TextAsBinary": [57, 46, 57, 69, 43, 49, 48, 49], + "Binary": [0, 1, 0, 25, 0, 0, 0, 0, 0, 99] + }, { "SQL": "'{-000.000,-0000021234.23246346000000,-1.2,.0,.1,.1234}'::decimal[]", "Oid": 1231, diff --git a/pkg/sql/pgwire/testdata/pgtest/decimal b/pkg/sql/pgwire/testdata/pgtest/decimal index 2ae73557a020..c6c3894e63d4 100644 --- a/pkg/sql/pgwire/testdata/pgtest/decimal +++ b/pkg/sql/pgwire/testdata/pgtest/decimal @@ -35,3 +35,51 @@ ReadyForQuery {"Type":"DataRow","Values":[{"text":"0"}]} {"Type":"CommandComplete","CommandTag":"SELECT 1"} {"Type":"ReadyForQuery","TxStatus":"I"} + +# [0, 1, 0, 1, 0, 0, 0, 0, 0, 1] = binary 1E+4 +# [0, 1, 0, 1, 0, 0, 0, 0, 0, 10] = binary 10E+4 +send +Parse {"Name": "s1", "Query": "SELECT 10000::decimal, $1::decimal, $2::decimal"} +Bind {"DestinationPortal": "p1", "PreparedStatement": "s1", "ParameterFormatCodes": [1], "Parameters": [[0, 1, 0, 1, 0, 0, 0, 0, 0, 1], [0, 1, 0, 1, 0, 0, 0, 0, 0, 10]]} +Execute {"Portal": "p1"} +Sync +---- + +# CockroachDB intentionally uses exponents for decimals like 1E+4, as +# oppposed to Postgres, which returns 10000. +until crdb_only +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"DataRow","Values":[{"text":"10000"},{"text":"1E+4"},{"text":"1.0E+5"}]} +{"Type":"CommandComplete","CommandTag":"SELECT 1"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +until noncrdb_only +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"DataRow","Values":[{"text":"10000"},{"text":"10000"},{"text":"100000"}]} +{"Type":"CommandComplete","CommandTag":"SELECT 1"} +{"Type":"ReadyForQuery","TxStatus":"I"} + +# ResultFormatCodes [1] = FormatBinary +# [0, 1, 0, 1, 0, 0, 0, 0, 0, 1] = binary 1E+4 +# [0, 1, 0, 1, 0, 0, 0, 0, 0, 10] = binary 10E+4 +send +Parse {"Name": "s2", "Query": "SELECT 10000::decimal, $1::decimal, $2::decimal"} +Bind {"DestinationPortal": "p2", "PreparedStatement": "s2", "ParameterFormatCodes": [1], "Parameters": [[0, 1, 0, 1, 0, 0, 0, 0, 0, 1], [0, 1, 0, 1, 0, 0, 0, 0, 0, 10]], "ResultFormatCodes": [1,1, 1]} +Execute {"Portal": "p2"} +Sync +---- + +until +ReadyForQuery +---- +{"Type":"ParseComplete"} +{"Type":"BindComplete"} +{"Type":"DataRow","Values":[{"binary":"00010001000000000001"},{"binary":"00010001000000000001"},{"binary":"0001000100000000000a"}]} +{"Type":"CommandComplete","CommandTag":"SELECT 1"} +{"Type":"ReadyForQuery","TxStatus":"I"} diff --git a/pkg/sql/pgwire/types.go b/pkg/sql/pgwire/types.go index d46d102edf97..0e46e5a8c074 100644 --- a/pkg/sql/pgwire/types.go +++ b/pkg/sql/pgwire/types.go @@ -353,6 +353,12 @@ func (b *writeBuffer) writeBinaryDatum( return ndigit } + // The dscale is defined as number of digits (in base 10) visible + // after the decimal separator, so it can't be negative. + if alloc.pgNum.Dscale < 0 { + alloc.pgNum.Dscale = 0 + } + b.putInt32(int32(2 * (4 + alloc.pgNum.Ndigits))) b.putInt16(alloc.pgNum.Ndigits) b.putInt16(alloc.pgNum.Weight)