feat: optimized binary numeric support

[bokken]

Provides optimized binary numeric support to go directly to a BigDecimal rather than from postgresql binary format to String and then to BigDecimal.

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Does ./gradlew autostyleCheck checkstyleAll pass ?
3. Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

• Does this break existing behaviour? If so please explain.
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your core changes, as applicable?
• Have you successfully run tests with your changes locally?

[bokken]

@vlsi or @davecramer this test is failing. It passes if I comment out PGProperty.PREFER_QUERY_MODE.set(props, "simple"); in the updateProperties method.

[vlsi]

Have you tried to activate logging and verify what SQL the driver sends to the DB? AFAIK queryMode=simple means it inlines the SQL value, so it might result in producing an invalid SQL.

The test code looks valid to me.

[bokken]

@vlsi that was it. I needed to make changes in SimpleParameterList to convert the binary back to String.

If that value has been configured to "simple", should we just disable the binary encoding to begin with? It seems kind of strange to go from "primitive" to binary, back to "primitive" so that we can then go to String.

[codecov-io]

Codecov Report

Merging #1940 into master will increase coverage by 0.40%.
The diff coverage is 92.95%.

@@             Coverage Diff              @@
##             master    #1940      +/-   ##
============================================
+ Coverage     69.39%   69.80%   +0.40%     
- Complexity     4228     4289      +61     
============================================
  Files           197      197              
  Lines         18006    18184     +178     
  Branches       2919     2971      +52     
============================================
+ Hits          12496    12693     +197     
+ Misses         4158     4136      -22     
- Partials       1352     1355       +3     

[vlsi]

@bokken , I see you have quite complicated logic in ByteConverter.
Codecov shows there are uncovered branches.

For instance, the following loop is shown as uncovered: https://codecov.io/gh/pgjdbc/pgjdbc/pull/1940/diff#D7-175

+ | int i = 1;
-- | --
  | 175 | + | for ( ; i < len && d == 0; ++i) {
  | 176 | + | effectiveScale -= 4;
  | 177 | + | idx += 2;
  | 178 | + | d = ByteConverter.int2(bytes, idx);
  | 179 | + | }

+ | if (sign == NUMERIC_NEG) {
-- | --
  | 227 | + | unscaledBI = unscaledBI.negate();
  | 228 | + | }

+ | if (mod != 0) {
-- | --
  | 341 | + | scale += mod;
  | 342 | + | unscaled = unscaled.multiply(tenPower(mod));
  | 343 | + | }

....

Can you please add tests to increase the coverage?

[davecramer]

@bokken I haven't had time to look at this in detail, but it looks quite intricate. I hope we can get some comments in here that would attempt to explain how this works without having to look at the server code to see how everything works.

[bokken]

@davecramer I have attempted to place comments within the public static Number numeric(byte [] bytes, int pos, int numBytes) method to describe how the format works.

I think some of the complexity is due to the very different way BigDecimal represents data internally.

[davecramer]

Ok, let me read these and see if they make sense. thx

[bokken]

@davecramer I have added some more comments explaining the format.

[bokken]

@vlsi @davecramer any ideas why the logical replication status test fails in one of the appveyor executions:

FAILURE   0.1sec, org.postgresql.replication.LogicalReplicationTest > testReplicationRestartFromLastFeedbackPositionParallelTransaction
    java.lang.AssertionError: When we add feedback about applied lsn to replication stream(in this case it's force update status)after restart consume changes via this slot should be started from last success lsn that we send before via force status update, that why we wait consume both transaction without duplicates
    Expected: "BEGIN\ntable public.test_logic_table: INSERT: pk[integer]:1 name[character varying]:'first tx changes'\nCOMMIT\nBEGIN\ntable public.test_logic_table: INSERT: pk[integer]:2 name[character varying]:'second tx changes'\nCOMMIT"
         but: was "BEGIN\ntable public.test_logic_table: INSERT: pk[integer]:1 name[character varying]:'first tx changes'\nCOMMIT\nBEGIN\ntable public.test_logic_table: INSERT: pk[integer]:1 name[character varying]:'first tx changes'\nCOMMIT"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.junit.Assert.assertThat(Assert.java:964)
        at org.postgresql.replication.LogicalReplicationTest.testReplicationRestartFromLastFeedbackPositionParallelTransaction(LogicalReplicationTest.java:834)

It appears this same test occasionally fails in the github actions test execution, but a re-run will pass.

[gmokki]

Have you run any performance tests on this? Is it slower or faster than running in text mode?
For simple value such as 42 or for a long value such as -123456789.123456780.

[bokken]

This is just for decoding/parsing. This is run with 64 bit jdk11 on windows.

Benchmark                                                                               (value)  Mode  Cnt    Score    Error  Units
NumericBinaryBenchmark.binaryConversion                        0.000000000000000000000000000990  avgt    5   23.816 ±  0.250  ns/op
NumericBinaryBenchmark.binaryConversion                                        10.0000000000099  avgt    5   66.667 ±  0.760  ns/op
NumericBinaryBenchmark.binaryConversion                                                   99999  avgt    5   19.846 ±  0.628  ns/op
NumericBinaryBenchmark.binaryConversion                                    -9223372036854775809  avgt    5   61.942 ±  0.749  ns/op
NumericBinaryBenchmark.binaryConversion                               -19223372036854775807.300  avgt    5  113.342 ±  2.306  ns/op
NumericBinaryBenchmark.binaryConversion                                                     -34  avgt    5   16.925 ±  0.431  ns/op
NumericBinaryBenchmark.binaryConversion        20306934976023476.000000000000000000000000000990  avgt    5  304.622 ± 12.355  ns/op
NumericBinaryBenchmark.stringBinaryConversion                  0.000000000000000000000000000990  avgt    5  223.835 ±  3.250  ns/op
NumericBinaryBenchmark.stringBinaryConversion                                  10.0000000000099  avgt    5  129.908 ±  2.353  ns/op
NumericBinaryBenchmark.stringBinaryConversion                                             99999  avgt    5   77.935 ±  0.824  ns/op
NumericBinaryBenchmark.stringBinaryConversion                              -9223372036854775809  avgt    5  222.485 ±  3.688  ns/op
NumericBinaryBenchmark.stringBinaryConversion                         -19223372036854775807.300  avgt    5  257.868 ±  3.759  ns/op
NumericBinaryBenchmark.stringBinaryConversion                                               -34  avgt    5   53.792 ±  1.141  ns/op
NumericBinaryBenchmark.stringBinaryConversion  20306934976023476.000000000000000000000000000990  avgt    5  540.041 ± 31.570  ns/op
NumericBinaryBenchmark.stringParsing                           0.000000000000000000000000000990  avgt    5  108.648 ±  1.440  ns/op
NumericBinaryBenchmark.stringParsing                                           10.0000000000099  avgt    5   73.115 ±  0.670  ns/op
NumericBinaryBenchmark.stringParsing                                                      99999  avgt    5   46.305 ±  0.526  ns/op
NumericBinaryBenchmark.stringParsing                                       -9223372036854775809  avgt    5  148.196 ±  1.099  ns/op
NumericBinaryBenchmark.stringParsing                                  -19223372036854775807.300  avgt    5  177.728 ±  1.075  ns/op
NumericBinaryBenchmark.stringParsing                                                        -34  avgt    5   37.484 ±  0.293  ns/op
NumericBinaryBenchmark.stringParsing           20306934976023476.000000000000000000000000000990  avgt    5  358.943 ± 13.330  ns/op

binaryConversion is this PR.
stringBinaryConversion is the existing binary support in the project.
stringParsing includes converting from byte[] to String before handing off to BigDecimal