Cockroach: NUMERIC data type not decoding properly in C# (NPGSQL)

[igremmerlb]

Describe the problem

Please describe the issue you observed, and any steps we can take to reproduce it:

We have a C# (Net Core) project connecting to a CockroachDB database. Entity Framework (C# ORM) defaults to using DECIMAL/NUMERIC type in CockroachDB for decimal fields in C# (without precision or scale). Reading numbers causes issues when the number is of the form X0000 (e.g. 10000, 20000, 30000). PostgreSQL is interpreted correctly. CockroachDBs values are also interpreted correctly using DBeaver, which uses the Java driver. The issue may be limited to NPGSQL / C#.

To Reproduce

What did you do? Describe in your own words.

Run the following C# code in Net Core (tested with 2.1 and 3.1):
Command line to create project: 'dotnet new console -o npgtest'

namespace npgtest
{
class Program
{
async static Task Main(string[] args)
{
Console.WriteLine("Hello World!");

        // var cs = "Host=localhost;Username=postgres;Password=mysecretpassword;Database=postgres;Port=5432"; // Postgres
        var cs = "Host=localhost;Username=root;Password=root;Database=postgres;Port=26257"; // Cockroach

        using var con = new NpgsqlConnection(cs);
        con.Open();

        await MakeTable(con);
        await InsertValues(con);
        await ReadValues(con);
    }

    private async static Task MakeTable(NpgsqlConnection con)
    {
        using (var cmd = new NpgsqlCommand(@"
            CREATE TABLE IF NOT EXISTS NUMTEST (
                intnum INT NOT NULL PRIMARY KEY,
                decnum NUMERIC NOT NULL
            )", con))
        {
            await cmd.ExecuteNonQueryAsync();
        }
    }

    private async static Task InsertValues(NpgsqlConnection con)
    {
        for(long i=0; i<100000; i+=1000)
        {
            using (var cmd = new NpgsqlCommand(@"
                INSERT INTO NUMTEST VALUES (@i, @d) ON CONFLICT DO NOTHING", con))
            {
                cmd.Parameters.AddWithValue("i", i);
                cmd.Parameters.AddWithValue("d", (decimal) i);
                await cmd.ExecuteNonQueryAsync();
            }
        }
    }

    private async static Task ReadValues(NpgsqlConnection con)
    {
        using (var cmd = new NpgsqlCommand(@"
            SELECT intnum, decnum from NUMTEST", con))
        {
            cmd.CommandTimeout = 0;
            using (DbDataReader reader = await cmd.ExecuteReaderAsync())
            {
                while (await reader.ReadAsync())
                {
                    long number = Convert.ToInt64(reader["intnum"]);
                    decimal decnum = (decimal)reader["decnum"];

                    Console.WriteLine($"{number}, {decnum}");
                }
            }
        }
    }
}

}

The code returns the following output:
1000, 1000
2000, 2000
3000, 3000
4000, 4000
5000, 5000
6000, 6000
7000, 7000
8000, 8000
9000, 9000
10000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001
11000, 11000
12000, 12000
13000, 13000
14000, 14000
15000, 15000
16000, 16000
17000, 17000
18000, 18000
19000, 19000
20000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002
21000, 21000
22000, 22000
23000, 23000
24000, 24000
25000, 25000
26000, 26000
27000, 27000
28000, 28000
29000, 29000
30000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000003
31000, 31000
32000, 32000
33000, 33000
34000, 34000
35000, 35000
36000, 36000
37000, 37000
38000, 38000
39000, 39000
40000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004
41000, 41000
42000, 42000
43000, 43000
44000, 44000
45000, 45000
46000, 46000
47000, 47000
48000, 48000
49000, 49000
50000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005
51000, 51000
52000, 52000
53000, 53000
54000, 54000
55000, 55000
56000, 56000
57000, 57000
58000, 58000
59000, 59000
60000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006
61000, 61000
62000, 62000
63000, 63000
64000, 64000
65000, 65000
66000, 66000
67000, 67000
68000, 68000
69000, 69000
70000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007
71000, 71000
72000, 72000
73000, 73000
74000, 74000
75000, 75000
76000, 76000
77000, 77000
78000, 78000
79000, 79000
80000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008
81000, 81000
82000, 82000
83000, 83000
84000, 84000
85000, 85000
86000, 86000
87000, 87000
88000, 88000
89000, 89000
90000, 0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000009
91000, 91000
92000, 92000
93000, 93000
94000, 94000
95000, 95000
96000, 96000
97000, 97000
98000, 98000
99000, 99000

If possible, provide steps to reproduce the behavior:

1. Set up CockroachDB cluster - localhost single node cluster is fine (tested on Windows)
2. Execute program
3. View bad data return for value = 10000, 20000, etc

Expected behavior
The first number (integer field) should be the same as the second number (numeric type).

Environment:

• CockroachDB version [e.g. 2.0.x] - tested with 20.1.8 on Windows
• Server OS: [e.g. Linux/Distrib] - Windows 10 x64 Pro
• Client app [e.g. cockroach sql, JDBC, ...] - C# NPGSQL Net Core 3.1 (and 2.1)

Additional context
What was the impact?

Corrupted financial data

Add any other context about the problem here.

[blathers-crl[bot]]

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

• @rafiss (found keywords: ORM,ORM)

If we have not gotten back to your issue within a few business days, you can try the following:

• Join our community slack channel and ask on #cockroachdb.
• Try find someone from here if you know they worked closely on the area and CC them.

_{:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[igremmerlb]

Note also that everything works fine if the numeric type is created with precision and scale:
Create table (
...
decnum NUMERIC(30,10) -- (any values seem fine)
)

However, even as of CockroachDB 20.2 with the experimental features enabled it still does not support alter table alter column to change from NUMERIC to NUMERIC(30,10), which makes it complicated to change the field type to work around the issue. It requires an add column, set equal to existing column, drop / recreate column, set value back, etc.

[rafiss]

Thanks for filing this!

@roji: Whenever you have a moment, would you be able to point me to the code in Npgsql that decodes NUMERIC? I think seeing that code will help me determine how to address this in CockroachDB.

[igremmerlb]

Thank you for looking into this!

[YohDeadfall]

@rafiss, in case of type handlers ping me since deal with them most of mine time. Will take a look, but it's weird since 10000 is covered by our tests for example.

[YohDeadfall]

@rafiss, as I see the handler correctly writes and reads values to the buffer. Methods for which you are looking are Read and Write.

Here are results of mine investigation including a minimal repro:

[TestCase(false)]
[TestCase(true)]
public async static Task Cockroach(bool cockroach)
{
    const string Cockroach = "Host=localhost;Username=root;Password=root;Database=postgres;Port=26257";

    using var conn = new NpgsqlConnection(cockroach ? Cockroach : TestUtil.ConnectionString);
    await conn.OpenAsync();

    var expected = 10000M;
    using var cmd = new NpgsqlCommand($"SELECT {expected}::numeric, @d", conn)
    {
        Parameters = { new NpgsqlParameter("d", expected) }
    };

    using var reader = await cmd.ExecuteReaderAsync();
    await reader.ReadAsync();

    var wired = reader.GetFieldValue<decimal>(0);
    var actual = reader.GetFieldValue<decimal>(1);

    Assert.AreEqual(expected, actual);
}

| Backend | Method | Value | Buffer |
| ---------- | ------ | ------ | ------------------------------- |
| PostgreSQL | Write | d | 00 01 00 01 00 00 00 00 00 01 |
| PostgreSQL | Read | wired | 00 01 00 01 00 00 00 00 00 01 |
| PostgreSQL | Read | actual | 00 01 00 01 00 00 00 00 00 01 |
| Cockroach | Write | d | 00 01 00 01 00 00 00 00 00 01 |
| Cockroach | Read | wired | 00 01 00 01 00 00 00 00 00 01 |
| Cockroach | Read | actual | 00 01 00 01 00 00 ff fc 00 01 |

As you can see, PostgreSQL returns both values passed to it correctly. Cockroach works well only when the value created on its side, but for the forwarded one it breaks two bytes which gives an incorrect result.

[rafiss]

Thanks! I've reproduced this in our own testing with the following. (It uses our pgwire protocol DSL)

send
Parse {"Name": "s1", "Query": "SELECT 10000::decimal, $1::decimal"}
Bind {"DestinationPortal": "p1", "PreparedStatement": "s1", "ParameterFormatCodes": [1], "Parameters": [[0, 1, 0, 1, 0, 0, 0, 0, 0, 1]]}
Execute {"Portal": "p1"}
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"BindComplete"}
{"Type":"DataRow","Values":[{"text":"10000"},{"text":"10000"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

# ResultFormatCodes [1] = FormatBinary
send
Parse {"Name": "s2", "Query": "SELECT 10000::decimal, $1::decimal"}
Bind {"DestinationPortal": "p2", "PreparedStatement": "s2", "ParameterFormatCodes": [1], "Parameters": [[0, 1, 0, 1, 0, 0, 0, 0, 0, 1]], "ResultFormatCodes": [1,1]}
Execute {"Portal": "p2"}
Sync
----

until
ReadyForQuery
----
{"Type":"ParseComplete"}
{"Type":"BindComplete"}
{"Type":"DataRow","Values":[{"binary":"00010001000000000001"},{"binary":"00010001000000000001"}]}
{"Type":"CommandComplete","CommandTag":"SELECT 1"}
{"Type":"ReadyForQuery","TxStatus":"I"}

It gets the following differences in the results:

{"Type":"DataRow","Values":[{"text":"10000"},{"text":"1E+4"}]}

{"Type":"DataRow","Values":[{"binary":"00010001000000000001"},{"binary":"000100010000fffc0001"}]}

There is one big difference -- the results in my repro show binary data of: 00 01 00 01 00 00 ff fc 00 01.
But the comment you posted shows that you got back 00 01 00 01 00 00 ff fa 00 01.

So we should get to the bottom of this... 00 01 00 01 00 00 ff fc 00 01 is the binary format of 1E+4, which although physically different than the Postgres value of 10000, is logically equivalent.

But 00 01 00 01 00 00 ff fa 00 01 is the binary format of 0E+6, which definitely is not correct.

[rafiss]

I've opened a draft PR #57049 with my testing so far.

@YohDeadfall as a next step, could you confirm how Npgsql handles the value 1E+4 (and also its binary format 00 01 00 01 00 00 ff fc 00 01).

And would you be able to confirm that you are getting back the value 00 01 00 01 00 00 ff fa 00 01 when testing with CockroachDB?

[YohDeadfall]

@rafiss, you're correct. Should be fc, fixed the message.

00 01 00 01 00 00 ff fc 00 01 is the binary format of 1E+4

Sending that value to PostgreSQL server causes 22P03: invalid scale in external "numeric" value according to the following lines:

#define NUMERIC_DSCALE_MASK         0x3FFF

    if ((value.dscale & NUMERIC_DSCALE_MASK) != value.dscale)
        ereport(ERROR,
                (errcode(ERRCODE_INVALID_BINARY_REPRESENTATION),
                 errmsg("invalid scale in external \"numeric\" value")));

As I can see PostgreSQL doesn't allow negative scales. Is it Cockroach specific?

[rafiss]

I think we may be missing a step in our decimal->binary encoding. Postgres does a truncation step that I don't think we implement the same way: https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/numeric.c#L10734

I tried to reproduce the original bug with the Go pgx driver, which also uses the binary format for prepared statements. That driver did not have this issue. I noticed that the way it encodes 10000 is 00 02 00 01 00 00 00 00 00 01 00 00. And in fact, when I test directly with the CockroachDB wire protocol with that value, CockroachDB does return 00 01 00 01 00 00 00 00 00 01 (which matches Postgres).

@YohDeadfall I'm curious if you can reproduce this issue by using the value 1 0000 0000

One last difference I'll note here which could be related. In PG, the CLI shows:

rafiss@127:postgres> select 10000::decimal, 1e+4::numeric
+-----------+-----------+
| numeric   | numeric   |
|-----------+-----------|
| 10000     | 10000     |
+-----------+-----------+

While CockroachDB shows

root@:26257/defaultdb> select 10000::decimal, 1e+4::numeric;
  numeric | numeric
----------+----------
    10000 | 1E+4

[otan]

Two things that will help short term, if you were willing to fix this NPGSQL side and have this work for both PG/CRDB. Also put down long term fixes for us:

• When CockroachDB encodes and NPGSQL decode the decimal, the scale can be read as an int16, so if you use the negative scale that value will be encoded correctly on your side. Reading it as an uint16 will result in the very many 0 values.
  
  
  
  • Fix from our side: PG seems to avoid negative scale values, we should mimic and fix that.
  
  
• When CockroachDB decodes the value NPGSQL provides, I think we will return 10000 instead of 1E+4 if you make add extra 00 00 at the end (other drivers do this so perhaps this is the problem). I think 10000 0000 is broken too, etc. for any value with 0000 as the last 4 digits.
  
  
  
  • Long term fix from CockroachDB: We should probably change our behaviour from https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/pgwire/pgwirebase/encoding.go#L638 to https://github.com/postgres/postgres/blob/master/src/backend/utils/adt/numeric.c#L10734.