Message-ID: <gh-pgjdbc-pgjdbc-3062-c4449679172@github.com>
From: "davecramer (@davecramer)" <noreply+davecramer@github.com>
To: "pgjdbc/pgjdbc" <noreply+pgjdbc-pgjdbc@github.com>
Date: Thu, 14 May 2026 10:02:31 +0000
Subject: Re: [pgjdbc/pgjdbc] PR #3062: feat: type cache rework, codec API, and composite-type round-trip
In-Reply-To: <gh-pgjdbc-pgjdbc-3062@github.com>
References: <gh-pgjdbc-pgjdbc-3062@github.com>
List-Id: <gh-pgjdbc-pgjdbc.github.com>
X-GitHub-Author-Login: davecramer
X-GitHub-Comment-Id: 4449679172
X-GitHub-Comment-Type: issue_comment
X-GitHub-Issue: 3062
X-GitHub-Repo: pgjdbc/pgjdbc
X-GitHub-Type: comment
X-GitHub-Url: https://github.com/pgjdbc/pgjdbc/pull/3062#issuecomment-4449679172
Content-Type: text/plain; charset=utf-8

the rest of the review - some stuff that is obvious

5. ThreadLocal CodecDepth — The spec says "safe with virtual threads due to short lifespan and proper cleanup." This
  is partially true — virtual threads do inherit ThreadLocals, and if a codec decode is interrupted mid-recursion
  (e.g., by a timeout), the ThreadLocal won't be cleaned up. The clear() method exists but needs to be called in a
  finally block at every entry point. Worth verifying this is done consistently.
  6. TOCTOU in epoch-based invalidation. Between checking typeCacheEpoch and using the cached PgType, another thread
  could increment the epoch. The volatile on typeCacheEpoch provides visibility but not atomicity of the
  check-then-use. For a JDBC driver (typically single-threaded per connection), this is fine in practice, but the
  spec's ConcurrentHashMap usage suggests multi-threaded access was considered.
  
  Medium Priority
  
  7. PgStruct extends PGobject — This is pragmatic but creates a diamond: PgStruct is both a Struct and a PGobject.
  Code that does if (obj instanceof PGobject) will now match composite columns that previously returned a plain
  PGobject. The ordering in PgPreparedStatement.setObject (Struct before PGobject) is correct, but downstream code
  that pattern-matches on PGobject subtypes may behave differently.
  8. FallbackCodec for unknown types — OID 705 (unknown) gets a special short-circuit to getString() in
  PgResultSet.getObject(int). This is correct for backward compat, but the layering is leaky — the codec layer should
  ideally handle this internally rather than requiring a special case in the ResultSet.
  9. Range types: typelem is zero — The code acknowledges this (pg_range.rngsubtype is not loaded). Range element
  typing is deferred. This means getObject() on a range column returns bounds as unparsed strings. Acceptable for a
  first release but should be tracked.
  10. SET search_path detection relies on the CommandComplete message containing "SET". If a user does SELECT
  set_config('search_path', ...), the cache won't invalidate. This is a known limitation but worth documenting more
  prominently.
  11. Commit structure — The first 10 commits are well-structured and reviewable. Commits 11–26 are fixup commits that
  should be squashed into the appropriate earlier commits before merge (vlsi acknowledges this).
  
  Low Priority
  
  12. map.pg_type.boolean property name uses dots, which is unusual for pgjdbc properties (most use camelCase).
  Consider mapPgTypeBoolean for consistency (which the PGProperty enum name already uses).
  13. @Experimental annotation — Good practice, but the Javadoc should clarify what "experimental" means: will the API
  be removed? Will method signatures change? Will the package be relocated?
  14. No multirange support — Explicitly deferred, which is fine for v1.