Message-ID: <gh-pgjdbc-pgjdbc-3306-c3805434734@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "pgjdbc/pgjdbc" <noreply+pgjdbc-pgjdbc@github.com>
Date: Tue, 27 Jan 2026 14:09:24 +0000
Subject: Re: [pgjdbc/pgjdbc] PR #3306: feat: add support for reading / creating structs
In-Reply-To: <gh-pgjdbc-pgjdbc-3306@github.com>
References: <gh-pgjdbc-pgjdbc-3306@github.com>
List-Id: <gh-pgjdbc-pgjdbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 3805434734
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 3306
X-GitHub-Repo: pgjdbc/pgjdbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/pgjdbc/pgjdbc/pull/3306#issuecomment-3805434734
Content-Type: text/plain; charset=utf-8

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?