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

@@ -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)

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(

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 \

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}"]
+]

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
+

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

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"},
+}

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

pkg/acceptance/testdata/elixir/test_crdb/test/test_helper.exs

@@ -0,0 +1 @@
+ExUnit.start()

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(

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)

pkg/sql/pgwire/pgwirebase/encoding.go

@@ -447,48 +447,82 @@ 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--
 					}
 					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 {

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,