From 1ca3e749e869ede45d51aaab87ad3d9dc116c076 Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Mon, 2 Mar 2020 15:40:45 -0800 Subject: [PATCH] pgwire: fix decimal decoding with trailing zeroes In addition to the release note, I have also modified the docker runfile to support elixir tests, and also updated the elixir tests to be run. Release note (bug fix): In previous PRs, drivers that do not truncate trailing zeroes for decimals in the binary format end up having inaccuracies of up to 10^4 during the decode step. In this PR, we fix the error by truncating the trailing zeroes as appropriate. This fixes known incorrect decoding cases with Postgrex in Elixir. --- pkg/acceptance/adapter_test.go | 9 +++ pkg/acceptance/cluster/certs.go | 4 +- pkg/acceptance/testdata/Dockerfile | 12 ++- .../testdata/elixir/test_crdb/.formatter.exs | 4 + .../testdata/elixir/test_crdb/.gitignore | 24 ++++++ .../testdata/elixir/test_crdb/mix.exs | 22 +++++ .../testdata/elixir/test_crdb/mix.lock | 6 ++ .../elixir/test_crdb/test/decimal_test.exs | 63 +++++++++++++++ .../elixir/test_crdb/test/test_helper.exs | 1 + pkg/acceptance/util_docker.go | 2 +- pkg/sql/pgwire/encoding_test.go | 10 +++ pkg/sql/pgwire/pgwirebase/encoding.go | 80 +++++++++++++------ pkg/sql/pgwire/testdata/encodings.json | 21 +++++ 13 files changed, 229 insertions(+), 29 deletions(-) create mode 100644 pkg/acceptance/testdata/elixir/test_crdb/.formatter.exs create mode 100644 pkg/acceptance/testdata/elixir/test_crdb/.gitignore create mode 100644 pkg/acceptance/testdata/elixir/test_crdb/mix.exs create mode 100644 pkg/acceptance/testdata/elixir/test_crdb/mix.lock create mode 100644 pkg/acceptance/testdata/elixir/test_crdb/test/decimal_test.exs create mode 100644 pkg/acceptance/testdata/elixir/test_crdb/test/test_helper.exs diff --git a/pkg/acceptance/adapter_test.go b/pkg/acceptance/adapter_test.go index ed28828d2450..e8ff07ba24b1 100644 --- a/pkg/acceptance/adapter_test.go +++ b/pkg/acceptance/adapter_test.go @@ -55,6 +55,15 @@ func TestDockerJava(t *testing.T) { testDockerFail(ctx, t, "java", []string{"sh", "-c", "cd /mnt/data/java && mvn -o foobar"}) } +func TestDockerElixir(t *testing.T) { + s := log.Scope(t) + defer s.Close(t) + + ctx := context.Background() + testDockerSuccess(ctx, t, "elixir", []string{"sh", "-c", "cd /mnt/data/elixir/test_crdb && mix local.hex --force && mix deps.get && psql -c 'CREATE DATABASE IF NOT EXISTS testdb' && mix test"}) + testDockerFail(ctx, t, "elixir", []string{"sh", "-c", "cd /mnt/data/elixir/test_crdb && mix local.hex --force && mix deps.get && mix thisshouldfail"}) +} + func TestDockerNodeJS(t *testing.T) { s := log.Scope(t) defer s.Close(t) diff --git a/pkg/acceptance/cluster/certs.go b/pkg/acceptance/cluster/certs.go index 7b02b1fd79df..8eef01fb0d0f 100644 --- a/pkg/acceptance/cluster/certs.go +++ b/pkg/acceptance/cluster/certs.go @@ -39,12 +39,12 @@ func GenerateCerts(ctx context.Context) func() { // Root user. maybePanic(security.CreateClientPair( certsDir, filepath.Join(certsDir, security.EmbeddedCAKey), - 512, 48*time.Hour, false, security.RootUser, true /* generate pk8 key */)) + 1024, 48*time.Hour, false, security.RootUser, true /* generate pk8 key */)) // Test user. maybePanic(security.CreateClientPair( certsDir, filepath.Join(certsDir, security.EmbeddedCAKey), - 512, 48*time.Hour, false, "testuser", true /* generate pk8 key */)) + 1024, 48*time.Hour, false, "testuser", true /* generate pk8 key */)) // Certs for starting a cockroach server. Key size is from cli/cert.go:defaultKeySize. maybePanic(security.CreateNodePair( diff --git a/pkg/acceptance/testdata/Dockerfile b/pkg/acceptance/testdata/Dockerfile index d548e46b0bdd..d7beb77ad16b 100644 --- a/pkg/acceptance/testdata/Dockerfile +++ b/pkg/acceptance/testdata/Dockerfile @@ -1,4 +1,4 @@ -FROM ubuntu:artful-20170916 +FROM ubuntu:18.04 # This Dockerfile bundles several language runtimes and test frameworks into one # container for our acceptance tests. @@ -49,12 +49,18 @@ RUN apt-get install --yes --no-install-recommends openjdk-8-jdk \ RUN curl -fsSL https://dl.yarnpkg.com/debian/pubkey.gpg > /etc/apt/trusted.gpg.d/yarn.asc \ && echo "deb https://dl.yarnpkg.com/debian/ stable main" > /etc/apt/sources.list.d/yarn.list \ && curl -fsSL https://packages.microsoft.com/keys/microsoft.asc > /etc/apt/trusted.gpg.d/microsoft.asc \ - && echo "deb https://packages.microsoft.com/repos/microsoft-ubuntu-zesty-prod zesty main" > /etc/apt/sources.list.d/microsoft.list \ + && echo "deb https://packages.microsoft.com/repos/microsoft-ubuntu-bionic-prod/ bionic main" > /etc/apt/sources.list.d/microsoft.list \ + && apt-get install --yes --no-install-recommends gnupg \ + && curl https://packages.erlang-solutions.com/erlang-solutions_2.0_all.deb > erlang-solutions_2.0_all.deb && dpkg -i erlang-solutions_2.0_all.deb \ && apt-get update \ && apt-get install --yes --no-install-recommends \ - dotnet-sdk-2.0.0 \ + dotnet-sdk-2.1 \ + dotnet-runtime-2.1 \ expect \ + elixir \ + esl-erlang \ libc6-dev \ + libcurl4 \ libpq-dev \ libpqtypes-dev \ make \ diff --git a/pkg/acceptance/testdata/elixir/test_crdb/.formatter.exs b/pkg/acceptance/testdata/elixir/test_crdb/.formatter.exs new file mode 100644 index 000000000000..d2cda26eddc9 --- /dev/null +++ b/pkg/acceptance/testdata/elixir/test_crdb/.formatter.exs @@ -0,0 +1,4 @@ +# Used by "mix format" +[ + inputs: ["{mix,.formatter}.exs", "{config,lib,test}/**/*.{ex,exs}"] +] diff --git a/pkg/acceptance/testdata/elixir/test_crdb/.gitignore b/pkg/acceptance/testdata/elixir/test_crdb/.gitignore new file mode 100644 index 000000000000..7dc11ecb065c --- /dev/null +++ b/pkg/acceptance/testdata/elixir/test_crdb/.gitignore @@ -0,0 +1,24 @@ +# The directory Mix will write compiled artifacts to. +/_build/ + +# If you run "mix test --cover", coverage assets end up here. +/cover/ + +# The directory Mix downloads your dependencies sources to. +/deps/ + +# Where third-party dependencies like ExDoc output generated docs. +/doc/ + +# Ignore .fetch files in case you like to edit your project deps locally. +/.fetch + +# If the VM crashes, it generates a dump, let's ignore it too. +erl_crash.dump + +# Also ignore archive artifacts (built via "mix archive.build"). +*.ez + +# Ignore package tarball (built via "mix hex.build"). +debug-*.tar + diff --git a/pkg/acceptance/testdata/elixir/test_crdb/mix.exs b/pkg/acceptance/testdata/elixir/test_crdb/mix.exs new file mode 100644 index 000000000000..e3a480c0b634 --- /dev/null +++ b/pkg/acceptance/testdata/elixir/test_crdb/mix.exs @@ -0,0 +1,22 @@ +defmodule Cockroach.MixProject do + use Mix.Project + + def project do + [ + app: :debug, + version: "0.1.0", + start_permanent: Mix.env() == :prod, + deps: deps() + ] + end + + def application do + [ + extra_applications: [:logger] + ] + end + + defp deps do + [{:postgrex, "~> 0.13", hex: :postgrex_cdb, override: true}] + end +end diff --git a/pkg/acceptance/testdata/elixir/test_crdb/mix.lock b/pkg/acceptance/testdata/elixir/test_crdb/mix.lock new file mode 100644 index 000000000000..e1f3a9c0edd8 --- /dev/null +++ b/pkg/acceptance/testdata/elixir/test_crdb/mix.lock @@ -0,0 +1,6 @@ +%{ + "connection": {:hex, :connection, "1.0.4", "a1cae72211f0eef17705aaededacac3eb30e6625b04a6117c1b2db6ace7d5976", [:mix], [], "hexpm", "4a0850c9be22a43af9920a71ab17c051f5f7d45c209e40269a1938832510e4d9"}, + "db_connection": {:hex, :db_connection, "1.1.3", "89b30ca1ef0a3b469b1c779579590688561d586694a3ce8792985d4d7e575a61", [:mix], [{:connection, "~> 1.0.2", [hex: :connection, repo: "hexpm", optional: false]}, {:poolboy, "~> 1.5", [hex: :poolboy, repo: "hexpm", optional: true]}, {:sbroker, "~> 1.0", [hex: :sbroker, repo: "hexpm", optional: true]}], "hexpm", "5f0a16a58312a610d5eb0b07506280c65f5137868ad479045f2a2dc4ced80550"}, + "decimal": {:hex, :decimal, "1.8.1", "a4ef3f5f3428bdbc0d35374029ffcf4ede8533536fa79896dd450168d9acdf3c", [:mix], [], "hexpm", "3cb154b00225ac687f6cbd4acc4b7960027c757a5152b369923ead9ddbca7aec"}, + "postgrex": {:hex, :postgrex_cdb, "0.13.5", "83f818ea1b959d176c694bb60c36f42080c36a7413f0d3b05d58ef2093fe17f5", [:mix], [{:connection, "~> 1.0", [hex: :connection, repo: "hexpm", optional: false]}, {:db_connection, "~> 1.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.0", [hex: :decimal, repo: "hexpm", optional: false]}], "hexpm", "1a01ba25472ad33bafbac6f042fde2dbab93e23bdaa49ffa3722926165c1052f"}, +} diff --git a/pkg/acceptance/testdata/elixir/test_crdb/test/decimal_test.exs b/pkg/acceptance/testdata/elixir/test_crdb/test/decimal_test.exs new file mode 100644 index 000000000000..5d84bc68edc1 --- /dev/null +++ b/pkg/acceptance/testdata/elixir/test_crdb/test/decimal_test.exs @@ -0,0 +1,63 @@ +defmodule Cockroach.TestDecimal do + use ExUnit.Case + + test "handles decimal correctly" do + ssl_opts = [certfile: System.get_env("PGSSLCERT"), keyfile: System.get_env("PGSSLKEY")] + {port, _} = Integer.parse(System.get_env("PGPORT") || "26257") + {start_ok, pid} = Postgrex.start_link( + hostname: System.get_env("PGHOST") || "localhost", + username: System.get_env("PGUSER") || "root", + password: "", + database: "testdb", + port: port, + ssl: (System.get_env("PGSSLCERT") != nil && true) || false, + ssl_opts: ssl_opts + ) + assert start_ok + for dec <- [ + Decimal.new("0"), + Decimal.new("0.0"), + Decimal.new("0.00"), + Decimal.new("0.000"), + Decimal.new("0.0000"), + Decimal.new("0.00000"), + Decimal.new("0.00001"), + Decimal.new("0.000012"), + Decimal.new("0.0000012"), + Decimal.new("0.00000012"), + Decimal.new("0.00000012"), + Decimal.new(".00000012"), + Decimal.new("1"), + Decimal.new("1.0"), + Decimal.new("1.000"), + Decimal.new("1.0000"), + Decimal.new("1.00000"), + Decimal.new("1.000001"), + Decimal.new("12345"), + Decimal.new("12345.0"), + Decimal.new("12345.000"), + Decimal.new("12345.0000"), + Decimal.new("12345.00000"), + Decimal.new("12345.000001"), + Decimal.new("12340"), + Decimal.new("123400"), + Decimal.new("1234000"), + Decimal.new("12340000"), + Decimal.new("12340.00"), + Decimal.new("123400.00"), + Decimal.new("1234000.00"), + Decimal.new("12340000.00"), + Decimal.new("1.2345e-10"), + Decimal.new("1.23450e-10"), + Decimal.new("1.234500e-10"), + Decimal.new("1.2345000e-10"), + Decimal.new("1.23450000e-10"), + Decimal.new("1.234500000e-10"), + ] do + {result_ok, result} = Postgrex.query(pid, "SELECT CAST($1 AS DECIMAL)", [dec]) + assert result_ok + [[ret]] = result.rows + assert ret == dec + end + end +end diff --git a/pkg/acceptance/testdata/elixir/test_crdb/test/test_helper.exs b/pkg/acceptance/testdata/elixir/test_crdb/test/test_helper.exs new file mode 100644 index 000000000000..869559e709ea --- /dev/null +++ b/pkg/acceptance/testdata/elixir/test_crdb/test/test_helper.exs @@ -0,0 +1 @@ +ExUnit.start() diff --git a/pkg/acceptance/util_docker.go b/pkg/acceptance/util_docker.go index 73603a0c4de9..8db351cc38cf 100644 --- a/pkg/acceptance/util_docker.go +++ b/pkg/acceptance/util_docker.go @@ -58,7 +58,7 @@ func testDockerSuccess(ctx context.Context, t *testing.T, name string, cmd []str const ( // Iterating against a locally built version of the docker image can be done // by changing acceptanceImage to the hash of the container. - acceptanceImage = "docker.io/cockroachdb/acceptance:20180416-090309" + acceptanceImage = "docker.io/cockroachdb/acceptance:20200303-091324" ) func testDocker( diff --git a/pkg/sql/pgwire/encoding_test.go b/pkg/sql/pgwire/encoding_test.go index e20a732e80f5..2db30b23cdc6 100644 --- a/pkg/sql/pgwire/encoding_test.go +++ b/pkg/sql/pgwire/encoding_test.go @@ -206,6 +206,16 @@ func TestExoticNumericEncodings(t *testing.T) { {apd.New(100000000, 0), []byte{0, 2, 0, 2, 0, 0, 0, 0, 0, 1, 0, 0}}, {apd.New(100000000, 0), []byte{0, 3, 0, 2, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0}}, {apd.New(100000001, 0), []byte{0, 3, 0, 2, 0, 0, 0, 0, 0, 1, 0, 0, 0, 1}}, + // Elixir/Postgrex combinations. + {apd.New(1234, 0), []byte{0, 2, 0, 0, 0, 0, 0, 0, 0x4, 0xd2, 0, 0}}, + {apd.New(12340, -1), []byte{0, 2, 0, 0, 0, 0, 0, 1, 0x4, 0xd2, 0, 0}}, + {apd.New(1234123400, -2), []byte{0, 3, 0, 1, 0, 0, 0, 2, 0x4, 0xd2, 0x4, 0xd2, 0, 0}}, + {apd.New(12340000, 0), []byte{0, 3, 0, 1, 0, 0, 0, 0, 0x4, 0xd2, 0, 0, 0, 0}}, + {apd.New(123400000, -1), []byte{0, 3, 0, 1, 0, 0, 0, 1, 0x4, 0xd2, 0, 0, 0, 0}}, + {apd.New(12341234000000, -2), []byte{0, 4, 0, 2, 0, 0, 0, 2, 0x4, 0xd2, 0x4, 0xd2, 0, 0, 0, 0}}, + // Postgrex inspired -- even more trailing zeroes! + {apd.New(0, 0), []byte{0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}, + {apd.New(1234123400, -2), []byte{0, 4, 0, 1, 0, 0, 0, 2, 0x4, 0xd2, 0x4, 0xd2, 0, 0, 0, 0}}, } evalCtx := tree.MakeTestingEvalContext(nil) diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go index 882617d147ba..2c7290f5e500 100644 --- a/pkg/sql/pgwire/pgwirebase/encoding.go +++ b/pkg/sql/pgwire/pgwirebase/encoding.go @@ -447,10 +447,21 @@ func DecodeOidDatum( if alloc.pgNum.Ndigits > 0 { decDigits := make([]byte, 0, int(alloc.pgNum.Ndigits)*PGDecDigits) - nextDigit := func() error { + for i := int16(0); i < alloc.pgNum.Ndigits; i++ { if err := binary.Read(r, binary.BigEndian, &alloc.i16); err != nil { - return err + return nil, err } + // Each 16-bit "digit" can represent a 4 digit number. + // In the case where each digit is not 4 digits, we must append + // padding to the beginning, i.e. + // * "1234" stays "1234" + // * "123" becomes "0123" + // * "12" becomes "0012" + // * "1" becomes "0001" + // * "0" becomes "0000" + // * "123456" becomes ["0012", "3456"] + // * "123456.789" becomes ["0012", "3456", "7890"] + // * "120123.45678" becomes ["0012", "0123", "4567", "8000"] numZeroes := PGDecDigits for i16 := alloc.i16; i16 > 0; i16 /= 10 { numZeroes-- @@ -458,37 +469,60 @@ func DecodeOidDatum( for ; numZeroes > 0; numZeroes-- { decDigits = append(decDigits, '0') } - return nil - } - - for i := int16(0); i < alloc.pgNum.Ndigits-1; i++ { - if err := nextDigit(); err != nil { - return nil, err - } if alloc.i16 > 0 { decDigits = strconv.AppendUint(decDigits, uint64(alloc.i16), 10) } } - // The last digit may contain padding, which we need to deal with. - if err := nextDigit(); err != nil { - return nil, err - } - Dscale := (alloc.pgNum.Ndigits - (alloc.pgNum.Weight + 1)) * PGDecDigits - if overScale := Dscale - alloc.pgNum.Dscale; overScale > 0 { - Dscale -= overScale - for i := int16(0); i < overScale; i++ { - alloc.i16 /= 10 - } - } - if alloc.i16 > 0 { - decDigits = strconv.AppendUint(decDigits, uint64(alloc.i16), 10) + // In the case of padding zeros at the end, we may have padded too many + // digits in the loop. This can be determined if the weight (defined as + // number of 4 digit groups left of the decimal point - 1) + the scale + // (total number of digits on the RHS of the decimal point) is less + // than the number of digits given. + // + // In Postgres, this is handled by the "remove trailing zeros" in + // `make_result_opt_error`, as well as `trunc_var`. + // Any zeroes are implicitly added back in when operating on the decimal + // value. + // + // Examples (with "," in the digit string for clarity): + // * for "1234", we have digits ["1234", "0"] for scale 0, which would + // make the digit string "1234,0000". For scale 0, we need to cut it back + // to "1234". + // * for "1234.0", we have digits ["1234", "0"] for scale 1, which would + // make the digit string "1234,0000". For scale 1, we need to cut it back + // to "1234.0". + // * for "1234.000000" we have digits ["1234", "0", "0"] with scale 6, + // which would make the digit string "1234,0000,0000". We need to cut it + // back to "1234,0000,00" for this to be correct. + // * for "123456.00000", we have digits ["12", "3456", "0", "0"] with + // scale 5, which would make digit string "0012,3456,0000,0000". We need + // to cut it back to "0012,3456,0000,0" for this to be correct. + // * for "123456.000000000", we may have digits ["12", "3456", "0", "0", "0"] + // with scale 5, which would make digit string "0012,3456,0000,0000". + // We need to cut it back to "0012,3456,0000,0" for this to be correct. + // + // This is handled by the below code, which truncates the decDigits + // such that it fits into the desired dscale. To do this: + // * ndigits [number of digits provided] - (weight+1) gives the number + // of digits on the RHS of the decimal place value as determined by + // the given input. Note dscale can be negative, meaning we truncated + // the leading zeroes at the front, giving a higher exponent (e.g. 0042,0000 + // can omit the trailing 0000, giving dscale of -4, which makes the exponent 4). + // * if the digits we have in the buffer on the RHS, as calculated above, + // is larger than our calculated dscale, truncate our buffer to match the + // desired dscale. + dscale := (alloc.pgNum.Ndigits - (alloc.pgNum.Weight + 1)) * PGDecDigits + if overScale := dscale - alloc.pgNum.Dscale; overScale > 0 { + dscale -= overScale + decDigits = decDigits[:len(decDigits)-int(overScale)] } + decString := string(decDigits) if _, ok := alloc.dd.Coeff.SetString(decString, 10); !ok { return nil, pgerror.Newf(pgcode.Syntax, "could not parse string %q as decimal", decString) } - alloc.dd.Exponent = -int32(Dscale) + alloc.dd.Exponent = -int32(dscale) } switch alloc.pgNum.Sign { diff --git a/pkg/sql/pgwire/testdata/encodings.json b/pkg/sql/pgwire/testdata/encodings.json index ff56cad6137c..b698f1fe7ac5 100644 --- a/pkg/sql/pgwire/testdata/encodings.json +++ b/pkg/sql/pgwire/testdata/encodings.json @@ -531,6 +531,27 @@ "TextAsBinary": [52, 50], "Binary": [0, 1, 0, 0, 0, 0, 0, 0, 0, 42] }, + { + "SQL": "'42.0'::decimal", + "Oid": 1700, + "Text": "42.0", + "TextAsBinary": [52, 50, 46, 48], + "Binary": [0, 1, 0, 0, 0, 0, 0, 1, 0, 42] + }, + { + "SQL": "'420000'::decimal", + "Oid": 1700, + "Text": "420000", + "TextAsBinary": [52, 50, 48, 48, 48, 48], + "Binary": [0, 1, 0, 1, 0, 0, 0, 0, 0, 42] + }, + { + "SQL": "'420000.0'::decimal", + "Oid": 1700, + "Text": "420000.0", + "TextAsBinary": [52, 50, 48, 48, 48, 48, 46, 48], + "Binary": [0, 1, 0, 1, 0, 0, 0, 1, 0, 42] + }, { "SQL": "'{-000.000,-0000021234.23246346000000,-1.2,.0,.1,.1234}'::decimal[]", "Oid": 1231,