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 #3062: feat: type cache rework, codec API, and composite-type round-trip
Date: Thu, 14 May 2026 10:02:31 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
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.
view thread (32+ messages) latest in thread
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 #3062: feat: type cache rework, codec API, and composite-type round-trip
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