pgjdbc/pgjdbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feedFrom: davecramer (@davecramer) <[email protected]>
To: pgjdbc/pgjdbc <[email protected]>
Subject: Re: [pgjdbc/pgjdbc] PR #3306: feat: add support for reading / creating structs
Date: Tue, 27 Jan 2026 14:09:24 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
I asked AI to review this and the result was:
## **Overall Assessment**
This is a significant feature addition that implements proper java.sql.Struct support for PostgreSQL composite types. The implementation is generally well-structured but has some areas that need attention.
## **Strengths**
1. Comprehensive implementation - Covers both text and binary formats for struct encoding/decoding
2. Good test coverage - Multiple test scenarios including nested structs and arrays
3. Proper abstraction - Introduces FieldDecoder interface to handle different field types
4. Binary format support - Implements binary encoding/decoding for better performance
## **Issues & Concerns**
### **Critical Issues**
1. Breaking API Change - Changes PgResultSet.getObject() to return Struct instead of PGobject for composite types. This breaks backward compatibility:
java
// Before: returned PGobject
// After: returns Struct
Object obj = rs.getObject(1); // Existing code will break
2. Thread Safety - The CurrentRow stack-based approach in PgResultSet is complex and potentially error-prone:
java
protected static final class CurrentRow {
private final ArrayDeque<Field[]> fieldsStack = new ArrayDeque<>();
private final ArrayDeque<Tuple> rowStack = new ArrayDeque<>();
This adds significant complexity for nested struct parsing.
3. Missing Null Checks - Several places assume non-null without proper validation:
java
PgStructDescriptor descriptor = castNonNull(connection.getTypeInfo().getPgStructDescriptor(typeName));
### **Design Concerns**
1. Mixing Responsibilities - ArrayDecoding now handles both arrays and structs. Consider separating struct decoding logic.
2. Performance Impact - The CurrentRow locking mechanism could impact performance for regular result sets that don't use structs.
3. Incomplete Implementation - PgStruct.getAttributes(Map) has limited type conversion support.
### **Code Quality Issues**
1. Inconsistent Error Handling:
java
} catch (PSQLException e) {
// Same check as Oid#valueOf
if (oid.isEmpty() || Character.isDigit(oid.charAt(0))) {
throw e;
}
2. Magic Numbers - JSONB version byte handling:
java
bytes.get(); // jsonb has an extra byte 'version'
length -= 1;
3. Regex in Hot Path:
java
private static final Pattern NEEDS_ESCAPE_PATTERN = Pattern.compile("[\\\"()\\s,]");
Consider caching or optimizing for common cases.
## **Recommendations**
1. Maintain Backward Compatibility - Add a connection property to opt-in to new Struct behavior
2. Simplify CurrentRow - Consider alternative approaches that don't require stack management
3. Add More Documentation - Complex parsing logic needs better comments
4. Performance Testing - Benchmark impact on non-struct queries
5. Handle Edge Cases - Add tests for NULL values, empty structs, deeply nested structures
6. Fix Composite Test - The test changes show breaking behavior that needs migration guide
## **Verdict**
Needs Work - This is valuable functionality but requires:
- Backward compatibility strategy
- Simplified implementation of nested struct parsing
- More comprehensive error handling
- Performance validation
The PR is close but needs refinement before merging to avoid breaking existing applications.
Is anyone willing to take this up?
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: github://pgjdbc/pgjdbc
Cc: [email protected], [email protected]
Subject: Re: [pgjdbc/pgjdbc] PR #3306: feat: add support for reading / creating structs
In-Reply-To: <<[email protected]>>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox