#4203 fix: [postgres] implement binary array deserialization to unblock JDBC setArray

[robfrank]

Fixes #4203.

Summary

• PostgresType.deserializeBinary() threw "Binary deserialization for arrays not yet implemented" for all array types. The exception in PostgresNetworkExecutor.bindCommand() wrote an error response but returned before reading the Bind message's result-format section, leaving the channel misaligned. The main message loop then consumed those leftover bytes as message-type characters, deadlocking client and server on socket reads.
• Implement PostgreSQL binary array wire format in PostgresType.deserializeBinaryArray: ndim + hasnull + elemOid + per-dimension (size, lb) + per-element (elemLen, data). Element OID dispatch covers bool, int2/4/8, float4/8, and text-like types (default).
• JDBC PreparedStatement.setArray(...) now completes cleanly instead of hanging.

Test plan

• PostgresTypeTest: 151/151 pass - includes 5 new binary array unit tests (text[], int8[], int4[], empty array, array with null element) and an updated former "not yet implemented" test.
• PostgresProtocolIT: 72/72 pass - includes new preparedStatementSetArrayTextDoesNotHang and preparedStatementSetArrayLongDoesNotHang integration tests exercising the exact conn.createArrayOf(...) + pst.setArray(...) path from the issue.
• PostgresWJdbcIT: 29/29 pass - no regressions in the broader JDBC suite.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 6 complexity

Metric	Results
Complexity	6

View in Codacy

🟢 Coverage 83.05% diff coverage · -7.59% coverage variation

Metric	Results
Coverage variation	✅ -7.59% coverage variation
Diff coverage	✅ 83.05% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (a5e1378)	126737	93948	74.13%
Head commit (3167773)	158473 (+31736)	105452 (+11504)	66.54% (-7.59%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4207)	59	49	83.05%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

Summary: This PR fixes a real deadlock bug introduced when PostgresNetworkExecutor.bindCommand() threw an exception mid-read, leaving the channel misaligned. The fix is correct and the binary array wire format implementation follows the PostgreSQL spec faithfully. Test coverage is good. A few concerns worth addressing:

---

Security / Robustness

1. Unbounded ArrayList pre-allocation (moderate - potential DoS)

In deserializeBinaryArray, totalElements is derived directly from untrusted wire data and used to pre-allocate the result list:

int totalElements = 1;
for (int d = 0; d < ndim; d++) {
  totalElements *= buffer.getInt();  // from wire
  buffer.getInt();
}
final ArrayList<Object> result = new ArrayList<>(totalElements);

A malicious client can send ndim=1, dim_size=Integer.MAX_VALUE with only a few bytes of element data. new ArrayList<>(Integer.MAX_VALUE) will attempt a heap allocation of ~8 GB and throw OutOfMemoryError, potentially taking down the server. Consider capping or deferring allocation:

// Defer: don't pre-size from wire data
final ArrayList<Object> result = new ArrayList<>();

Or add a reasonable sanity cap (e.g., the remaining bytes in the buffer divided by minimum element size).

2. Integer overflow in multi-dimensional element count

totalElements *= buffer.getInt();

For a 2D array with two large dimensions (e.g., 50001 x 50001), totalElements silently overflows into a small or negative number, causing the loop to under-read elements and leave the buffer misaligned - exactly the class of bug this PR is fixing.

// Safer:
final long dimSize = buffer.getInt();
totalElements = Math.toIntExact(totalElements * dimSize); // throws ArithmeticException on overflow

---

Minor Issues

3. Boolean deserialization inconsistency

The scalar path uses buffer.get() == 1 but the array element path uses buf.get() != 0. Both work in practice (PostgreSQL only sends 0/1), but they're semantically different - != 0 is the spec-correct version. Consider aligning them.

4. Magic OID numbers in deserializeBinaryElement

The switch arms use raw OID literals (16, 20, 21, 23, 700, 701). The existing PostgresType enum already maps these codes (BOOLEAN, LONG, SMALLINT, INTEGER, REAL, DOUBLE). Referencing PostgresType.BOOLEAN.code etc. would make the code more maintainable and self-documenting without adding noise.

5. Return type widens to List

deserializeBinaryArray returns ArrayList<Object> - this is consistent with the existing parseArrayFromString style in the class, so not a blocking issue, but worth noting.

---

Test Coverage

The unit tests in PostgresTypeTest are well-structured and cover the important cases (text[], int8[], int4[], empty array, null elements). A few gaps:

• No tests for float4[] (OID 700) or float8[] (OID 701)
• No tests for bool[] (OID 16) or int2[] (OID 21)
• No test for a 2D array (ndim=2) to verify the element-count arithmetic

The integration test assertions are essentially no-ops:

assertThat(count).isGreaterThanOrEqualTo(0); // must not hang

count is always >= 0 by construction. The comment "must not hang" reflects the real intent - this is a liveness test - but it could at least assert rs != null or close cleanly without exception, making the intent clearer to future readers:

assertThatCode(() -> { rs.next(); }).doesNotThrowAnyException();
// or simply - the test passing without timeout IS the assertion

---

Positive Highlights

• Root cause analysis in the PR description is accurate and clear.
• The ndim == 0 fast-path is correct per spec (empty array has no dimension records).
• elemLen == -1 null sentinel handling is correct per spec.
• Pre-existing parseArrayFromString is unchanged - no regression risk there.
• Tests use assertThat(result).isInstanceOf(ArrayList.class) which is consistent with the project's AssertJ style.

---

Overall: The fix is correct and the implementation is solid. The unbounded allocation (item 1) and integer overflow (item 2) are the only concerns worth addressing before merge - both could cause server instability in adversarial or edge-case scenarios.

[gemini-code-assist]

Code Review

This pull request implements binary deserialization for PostgreSQL arrays, resolving issues where the protocol would hang due to unimplemented array support. The changes include new logic for parsing array dimensions and elements, supported by comprehensive integration and unit tests. Feedback focuses on improving robustness by validating array dimensions against integer overflow and buffer size to prevent potential memory issues, as well as optimizing performance by reducing intermediate allocations during element processing.

[gemini-code-assist]

The calculation of totalElements is susceptible to integer overflow and lacks validation of dimension sizes. If totalElements overflows or is maliciously large, it can cause an OutOfMemoryError or IllegalArgumentException during ArrayList allocation. To prevent the protocol synchronization issues (deadlocks) this PR aims to fix, it is crucial to validate that dimensions are non-negative and that the total element count is reasonable relative to the remaining buffer size (e.g., each element requires at least 4 bytes for its length header).

    long totalElementsLong = 1;
    for (int d = 0; d < ndim; d++) {
      final int dimSize = buffer.getInt();
      if (dimSize < 0)
        throw new PostgresProtocolException("Negative array dimension size");
      totalElementsLong *= dimSize;
      buffer.getInt();                   // lower bound (unused)
    }

    if (totalElementsLong < 0 || totalElementsLong > Integer.MAX_VALUE || totalElementsLong > buffer.remaining() / 4)
      throw new PostgresProtocolException("Invalid array dimensions");

    final int totalElements = (int) totalElementsLong;
    final ArrayList<Object> result = new ArrayList<>(totalElements);

[robfrank]

Addressed in 88013eb. totalElements is now a long, each dim size is checked for negative values, the running product is rejected if it exceeds Integer.MAX_VALUE, and the final count is capped at buffer.remaining() / 4 (each element needs at least a 4-byte length prefix). ndim is also clamped to PostgreSQL's MAXDIM=6. Added regression tests deserializeBinaryArrayRejectsNegativeDimension, deserializeBinaryArrayRejectsTooManyDimensions, deserializeBinaryArrayRejectsOversizedElementCount, deserializeBinaryArrayRejectsOverflowAcrossDimensions.

[gemini-code-assist]

Allocating a new byte[] and wrapping it in a ByteBuffer for every element is inefficient, especially for large arrays of primitives. Consider refactoring deserializeBinaryElement in the future to read directly from the parent buffer to avoid these intermediate allocations and reduce garbage collection pressure.

[robfrank]

Noted. The per-element byte[] + wrap pattern is a deliberate trade-off in this PR to keep the protocol fix minimal and easy to audit. Reading scalar elements directly off the parent buffer is a worthwhile follow-up optimization once the variable-length text path is also threaded through; tracking separately.

[robfrank]

Pushed 88013eb addressing review feedback:

Security / robustness (claude #1, #2 + gemini high-pri)

• totalElements is now a long; multiply is rejected if it exceeds Integer.MAX_VALUE (catches multi-dim overflow that would silently mis-read elements - exactly the bug class this PR fixes).
• Negative dim sizes and ndim > 6 (PostgreSQL MAXDIM) are rejected with a protocol exception.
• Element count is capped at buffer.remaining() / 4 to prevent adversarial pre-allocation from OOM-ing the server.

Minor issues

• claude Bump mongo-java-driver from 3.12.4 to 3.12.10 #4 (magic OIDs): deserializeBinaryElement now references PostgresType.BOOLEAN.code, LONG.code, SMALLINT.code, INTEGER.code, REAL.code, DOUBLE.code via if-else (the enum codes are not compile-time constants so a switch cannot use them).
• claude Bump slf4j-simple.version from 1.7.31 to 1.7.32 #3 (boolean inconsistency): kept the scalar path's == 1 unchanged - it predates this PR and changing it risks a subtle regression elsewhere. The array element path uses spec-correct != 0.
• claude Bump jedis from 3.6.1 to 3.6.3 #5 (return type widening): noted; matches the existing parseArrayFromString style, leaving as is.

Test coverage

• Added deserializeBinaryArrayBool, deserializeBinaryArrayInt2, deserializeBinaryArrayFloat4, deserializeBinaryArrayFloat8, and deserializeBinaryArray2D.
• Added four validation tests: RejectsNegativeDimension, RejectsTooManyDimensions, RejectsOversizedElementCount, RejectsOverflowAcrossDimensions.
• Integration tests now drain the result set and run a follow-up COUNT(*) on the same connection, asserting both the expected row count and that the wire protocol stays aligned after the binary-array Bind (which is the real value of these tests).

Gemini medium-pri (per-element byte[] allocation)

• Acknowledged inline. Reading scalar elements off the parent buffer is a worthwhile follow-up optimization; out of scope for this PR.

Test results: PostgresTypeTest 160/160, PostgresProtocolIT 72/72, PostgresWJdbcIT 29/29.

[claude]

Code Review: #4203 fix - Binary array deserialization for PostgreSQL wire protocol

Overview

This PR correctly identifies and fixes a two-part bug:

1. deserializeBinaryArray was not implemented, causing an exception to be thrown
2. That exception left the channel misaligned (result-format bytes unconsumed), deadlocking the client

The fix is correct, well-tested, and includes solid security validations. Some minor issues worth addressing.

---

Positives

• Root cause addressed correctly. The bytes for each parameter value are already consumed before deserialization (line 1061-1062 in bindCommand). The exception left the result-format section unread, causing the channel misalignment. Now that deserialization succeeds, the protocol flow completes normally.
• Strong security validations. The ndim < 0 || ndim > MAX_ARRAY_DIMENSIONS, negative dimSize, totalElements > Integer.MAX_VALUE, and buffer-bounds checks are all important guards against malformed or malicious payloads. Good.
• Excellent test coverage. 13 unit tests covering all scalar element types (bool, int2, int4, int8, float4, float8, text), empty arrays, null elements, 2D arrays, and all 4 rejection paths. Plus 2 integration tests exercising the exact JDBC path from the issue.

---

Issues

Structural: error path in bindCommand still leaves channel misaligned

The channel misalignment is fixed for the array case, but the root problem is still latent. If deserializeBinaryElement (or any other deserializer) throws for a future type, execution jumps to the catch at line 1122, writes an error response, and returns without reading the result-format section. Those bytes remain in the channel for the next message loop iteration.

A more robust fix would drain the remaining Bind message bytes before writing the error:

} catch (final Exception e) {
    // Drain result-format section so the channel stays aligned regardless of where
    // in the Bind message the exception occurred.
    try {
        final int resultFormatCount = channel.readShort();
        for (int i = 0; i < resultFormatCount; i++)
            channel.readUnsignedShort();
    } catch (final Exception ignored) {
        // Best-effort drain; if this also fails the channel is unrecoverable.
    }
    setErrorInTx();
    writeError(ERROR_SEVERITY.ERROR, "Error on parsing bind message: " + e.getMessage(), "XX000");
}

This is a pre-existing issue not introduced by this PR, but since this PR already touches the misalignment story it would be worth addressing here.

Minor: deserializeBinaryElement should use switch for consistency

deserializeBinary uses a switch expression for exactly this kind of OID dispatch. deserializeBinaryElement uses a chain of if statements, which is inconsistent:

// inconsistent with the rest of the class
if (elemOid == BOOLEAN.code) return buf.get() != 0;
if (elemOid == LONG.code)    return buf.getLong();
...

Prefer:

return switch (elemOid) {
    case 16  -> buf.get() != 0;      // bool
    case 20  -> buf.getLong();       // int8
    case 21  -> (int) buf.getShort();// int2 -- see note below
    case 23  -> buf.getInt();        // int4
    case 700 -> buf.getFloat();      // float4
    case 701 -> buf.getDouble();     // float8
    default  -> new String(bytes, DatabaseFactory.getDefaultCharset());
};

Using hardcoded literals also avoids the fragility of BOOLEAN.code references inside a method that already works with raw OIDs.

Minor: SMALLINT elements return Short but ARRAY_INT carries them

ARRAY_INT has OID 1007 (int4[]), but if someone sends an int2[] (OID 1005) it would be dispatched to ARRAY_INT and elements decoded as Short. This is likely fine in practice (1005 is not in the switch in deserializeBinary), but the asymmetry is worth noting in a comment.

Minor: MAX_ARRAY_DIMENSIONS placement

The constant is declared between two methods rather than with other class-level constants. Move it to the class's constant section for discoverability.

Minor: return type should be List<Object>

// less preferable - exposes implementation detail
private static ArrayList<Object> deserializeBinaryArray(ByteBuffer buffer)

// prefer
private static List<Object> deserializeBinaryArray(ByteBuffer buffer)

ArrayList is already guaranteed by the construction; the caller only cares about List.

Note: ARRAY_JSON / jsonb binary format not handled

For ARRAY_JSON with jsonb element OID 3802, the binary encoding is not a raw UTF-8 string (it has a version prefix byte). Currently such elements fall through to the text-string default, which would produce garbage. This is an edge case, not a regression introduced by this PR, but it is worth a // TODO: jsonb binary format differs - needs version-byte stripping comment in the default branch.

---

Test Coverage

The test deserializeBinaryArray2D correctly documents that multi-dimensional arrays are flattened to a single list. This might be surprising to callers, but since the query engine expects a flat collection for IN parameter binding, it is the right behavior. Consider adding a brief comment to that test explaining the intentional flattening.

---

Summary

The fix is correct and the security validations are solid. The structural misalignment issue in bindCommand's error path is the most valuable follow-up. The remaining items are minor style and robustness suggestions.

[robfrank]

Pushed 3167773 addressing round-2 review:

Structural - bindCommand error path drain (most valuable item)

• bindCommand() now tracks totalParamValues, paramsConsumed, and resultFormatSectionRead outside the try block. The catch handler best-effort drains any remaining parameter values (each: int32 size + size bytes) and the result-format section before writing the error. This keeps the channel aligned even if a future deserializer throws mid-Bind. It also closes the deadlock window that my new validation throws (RejectsNegativeDimension, etc.) would otherwise re-introduce for malicious payloads.

Style/robustness

• MAX_ARRAY_DIMENSIONS moved to the class-level constant block next to the other constants.
• deserializeBinaryArray return type widened to List<Object>.
• Added a comment on the SMALLINT.code branch documenting the int2-element / no-int2-array asymmetry.
• Added a comment on deserializeBinaryArray2D explaining the intentional flattening for IN-parameter binding.

Pushback on switch-vs-if-else (round-2 item 2)

• Round 1 of this review asked to replace magic OID literals with PostgresType.<NAME>.code references for self-documentation. Round 2 now asks to revert to literals because they fit a switch expression. Java requires switch case labels to be compile-time constants, but BOOLEAN.code, LONG.code, etc. are instance-final values set at runtime - they cannot appear in case. Picking literals would make the dispatch fragile (a hypothetical enum-value renumbering would silently break the switch while the enum reference would still resolve correctly). Keeping the if-else with enum references.

Pushback on jsonb TODO (round-2 item 6)

• The reviewer suggests a // TODO: jsonb binary format comment in the default branch. ArcadeDB does not currently serialize jsonb values over the wire (the existing parseArrayFromString for ARRAY_JSON also treats elements as text), and per the project guidelines (CLAUDE.md: "Don't design for hypothetical future requirements") we avoid placeholder TODOs for unimplemented features. If a real jsonb client surfaces, that work can be tracked as its own issue.

Test results: PostgresTypeTest 160/160, PostgresProtocolIT 72/72, PostgresWJdbcIT 29/29.

[claude]

Code Review

Summary: This PR fixes a real deadlock: bindCommand() was throwing on binary array deserialization mid-read, leaving the channel misaligned and causing client and server to both block on socket reads. The root cause fix (drain remaining Bind bytes on exception) and the binary array implementation are both on the right track. However, there is one correctness bug in the drain logic that would re-introduce misalignment in a common failure scenario.

---

Bug: paramsConsumed is incremented after deserialize(), but bytes are read before it

In bindCommand(), the param loop reads bytes from the wire first, then deserializes:

channel.readBytes(paramValue);           // bytes consumed from wire HERE
// ...
PostgresType.deserialize(..., paramValue); // may throw
paramsConsumed = i + 1;                  // only incremented on success

If deserialize() throws for param i, paramsConsumed == i - but param i's bytes are already gone from the channel. The drain loop then starts at i and reads one too many params, pulling what should be param i+1's length prefix as param i's element data. The channel ends up misaligned - the exact problem this PR is trying to fix.

Fix: move the increment to after readBytes but before deserialize:

channel.readBytes(paramValue);
paramsConsumed = i + 1;   // bytes consumed - track before deserialization
// ...
PostgresType.deserialize(..., paramValue);

This is the scenario where the fix matters: the old "Binary deserialization for arrays not yet implemented" exception happened during deserialize() (not during the wire read), so param bytes were consumed but paramsConsumed was not incremented, leaving the drain off by one.

---

Minor: drain does not handle null params (sz = 0xFFFFFFFF unsigned)

PostgreSQL sends -1 (network bytes FF FF FF FF) to signal a null parameter. channel.readUnsignedInt() returns 4294967295L. The drain check if (sz > 0) evaluates true, and channel.readBytes(new byte[(int) sz]) then tries new byte[-1], throwing NegativeArraySizeException. This is caught by the outer catch (Exception ignored), so the drain silently aborts, leaving the channel misaligned for any remaining params.

The original read loop has the same issue (new byte[(int) paramSize] for null params), so this is pre-existing. The drain should just add:

if (sz == 0xFFFFFFFFL)  // null param: no element bytes follow
    continue;

---

Style: missing final on buffer parameter

Per the project convention (CLAUDE.md - "use the final keyword when possible on variables and parameters"):

// current
private static List<Object> deserializeBinaryArray(ByteBuffer buffer) {

// should be
private static List<Object> deserializeBinaryArray(final ByteBuffer buffer) {

---

What is good

• Root cause diagnosis is correct - the channel misalignment after a mid-read exception is the real issue, and draining the remaining Bind bytes is the right approach.
• Binary array wire format implementation is correct - ndim + hasnull + elemOid + per-dimension headers + per-element (len, data) matches the PostgreSQL protocol spec.
• Security guards are solid - the checks for ndim > MAX_ARRAY_DIMENSIONS, negative dimension size, integer overflow across dimensions, and element count vs remaining buffer bytes all prevent DoS via malformed payloads.
• OID dispatch uses the existing enum constants (BOOLEAN.code, LONG.code, REAL.code, etc.) - consistent and maintainable.
• Test coverage is thorough - unit tests cover text, int8, int4, int2, float4, float8, bool, 2D, empty, null elements, and all three security-rejection cases. Integration tests verify the exact hang scenario from the issue report.

[codecov]

Codecov Report

❌ Patch coverage is 81.35593% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.99%. Comparing base (a5e1378) to head (3167773).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...com/arcadedb/postgres/PostgresNetworkExecutor.java	37.50%	10 Missing ⚠️
.../main/java/com/arcadedb/postgres/PostgresType.java	97.67%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4207   +/-   ##
=======================================
  Coverage   64.98%   64.99%           
=======================================
  Files        1643     1643           
  Lines      126737   126795   +58     
  Branches    27127    27146   +19     
=======================================
+ Hits        82364    82414   +50     
- Misses      32818    32835   +17     
+ Partials    11555    11546    -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.