JDBC: bad binary-to-string conversion for 1022

[pnmaher]

I'm submitting a bug in the JDBC driver.
When converting a numeric in binary representation, a number like -1022 has the internal zero ignored and -122 is output.
This will affect any number with a zero in the leading short.

• [X ] bug report
• feature request

The relevant class is org.postgresql.util.ByteConverter, method digitToString.
This converts a number from binary representation to text representation.

While looking at the binary representation of numerics, I tested with the value -1022 and was startled by a return value of -122.

Driver Version? 42.2.10
Java Version? 1.8

To Reproduce
Steps to reproduce the behaviour:
The binary representation of -1022 is:
00 01 00 00 40 00 00 01 03 fe
The last short, 0x3fe = 1022.

@Test
final void test1022() {
    byte[] b = new byte[] {0x00, 0x01, 0x00, 0x00, 0x40, 0x00, 0x00, 0x01, 0x03, (byte) 0xfe};
    Number x = org.postgresql.util.ByteConverter.numeric(b) ;
    assertEquals(-1022, x.intValue());
}

@Test
final void test2305() {
    byte[] b = new byte[] {0x00, 0x01, 0x00, 0x00, 0x40, 0x00, 0x00, 0x01, 0x09, (byte) 0x01};
    Number x = org.postgresql.util.ByteConverter.numeric(b) ;
    assertEquals(-2305, x.intValue());
}

Expected behaviour
I expect the correct value to be returned.

** Suggested fix **
Replace ByteConverter.digitToString() with:

private static void digitToString(int idx, short[] digits, CharBuffer buffer, boolean alwaysPutIt) {
    short dig = (idx >= 0 && idx < digits.length) ? digits[idx] : 0;
    boolean putit = alwaysPutIt ;
    for(int p = 1; p < round_powers.length; p++) {
        int pow = round_powers[p];
        short d1 = (short) (dig / pow);
        dig -= d1 * pow;
        
        putit = putit || (d1 > 0);
        if (putit) {
            buffer.put((char) (d1 + '0'));
        }
    }

    buffer.put((char) (dig + '0'));
}

This remembers that the some digit has been added and so does not skip a subsequent zero.

[davecramer]

I don't think we ever send/receive numeric in binary? How did you run into this ?

[bokken]

#1636

…

On Sat, Apr 4, 2020 at 10:47 AM Dave Cramer ***@***.***> wrote: I don't think we ever send/receive numeric in binary? How did you run into this ? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1749 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAW3U3PJJCDMREWQ5WRMABLRK5I7JANCNFSM4L5ICUMA> .

[bokken]

But ad83cb3#diff-1ba924d72e9b18676e312b83bc90c7e7 Should have reverted that, right? Was there a release between the 2? On Sat, Apr 4, 2020 at 11:07 AM Brett Okken <brett.okken.os@gmail.com> wrote:

…

#1636 On Sat, Apr 4, 2020 at 10:47 AM Dave Cramer ***@***.***> wrote: > I don't think we ever send/receive numeric in binary? How did you run > into this ? > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#1749 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAW3U3PJJCDMREWQ5WRMABLRK5I7JANCNFSM4L5ICUMA> > . >

[davecramer]

Good question. It appears that we need more testing for #1636 to make sure we don't overwrite this change. Secondly it appears there is a bug in it

[bokken]

I have spent some time working[1] on "real" support for binary numeric values. The approach in #1636 to convert binary to String and then parse the String does not seem to add any benefit over having the backend just send String representation.

Even after looking at the "real" support for converting the binary representation directly to BigDecimal, I am not convinced there are meaningful gains in either payload size or conversion speed. The postgresql numeric binary representation and the java BigDecimal representation just do not align closely enough to make for a straightforward/fast conversion.

[1] - https://github.com/bokken/pgjdbc/tree/numeric_binary

[davecramer]

@bokken thanks for looking at this. Not surprising