crystal-pg — Road to 1.0: comprehensive audit (correctness, concurrency, error model, perf, protocol completeness, API/SemVer, tests) — produced with Claude Opus 4.8 [1M], headline findings hand-verified

AUDIT-1.0.md

crystal-pg — Road to 1.0: comprehensive audit

Multi-dimension audit of crystal-pg (pg shard, v0.30.0, crystal-db ~> 0.14.0) toward a 1.0 tag. Dimensions: correctness, concurrency / fiber-safety, error model, perf/memory/CPU, PostgreSQL protocol completeness, public API / SemVer surface, test coverage + unaudited files.

Headline findings (compile / wire-format / security) were verified by hand against the source. Citations are file:line.

ℹ️ Transparency: produced with Claude Opus 4.8 [1M] (multi-pass static review, 6 parallel agents), then the blockers were hand-verified.

The good news first

The architecture is sound and the layering is the right one: a low-level wire layer (PQ::*) cleanly separated from the crystal-db driver (PG::*), correct binary decoders for the large majority of types, SCRAM-SHA-256 + channel binding support (rare and well done), logical replication (pgoutput), and a real round-trip spec suite against Postgres 14–18. The bones are good. What follows is the map of what stands between today and a 1.0 that can be promised.

Summary by tier

Tier	Theme	Headline
0	Blockers	4 spots that pass crystal build but break when the path is exercised: Interval#to_spans → undefined methods; .size instead of .bytesize (non-ASCII startup/password); a pp debug print in prod; CI on main ≠ master (never fires). (A 5th finding — decode error poisoning a pooled connection — was retracted after a reproduction showed it does not occur; see 0.5.)
1	Security	verify-ca/verify-full accepted but VerifyMode::NONE (no cert/hostname verification); an over-permissive .pgpass is read anyway; PGSSLMODE/PGSSL* env vars ignored
2	Correctness & concurrency	RowDescription table_oid/column fields swapped + truncated; infinity/-infinity timestamps unhandled; Numeric#to_s truncation fragile; two array paths disagree on lower bounds; the LISTEN/NOTIFY loop shares the socket with no lock; @@location_cache not fiber-safe; error model has two disjoint trees
3	Protocol completeness	verify-* unimplemented; NegotiateProtocolVersion unhandled; no CancelRequest; missing types (time/timetz, inet/cidr/macaddr, money, ranges); replication: no slot creation, proto pinned to v1, ErrorFrame ignored
4	Perf / memory / CPU	decoder resolved by per-value OID hash instead of per-column; no real prepared statements (Parse on every exec); Array(Slice?) + per-column copy per row; to_f64 allocates a reverse+enum
5	API / SemVer	the whole PQ::* is :nodoc: but public in practice; PQ::Connection#soc + protocol I/O public; sslmode is a Symbol; Geo::Path inconsistent with its record siblings; PG::Error :nodoc: yet root of a hierarchy users must rescue; dead pre-Crystal-1.0 monkeypatches
6	Tests / CI / packaging	CI nix-only, single Crystal; frame.cr/query.cr/ext/openssl.cr/record.cr zero coverage; auth & escape-error specs disabled; depends on crystal-db, itself pre-1.0

---

Tier 0 — Blockers (compiles, but breaks when the path is exercised)

0.1 PG::Interval#to_spans calls methods that don't exist

src/pg/interval.cr:35-40 — to_spans returns {to_time_span, to_time_month_span}, but those methods don't exist; the real ones are to_span / to_month_span (lines 13, 31). Won't compile the moment any caller touches to_spans (no spec covers it, hence the silence). Fix: {to_span, to_month_span} — note to_span requires approx_months when months != 0, so reconsider the signature. Add a spec.

0.2 .size instead of .bytesize in the startup and password packets

src/pq/connection.cr:151 (acc + arg.size + 1) and :486 (password.size + 4 + 1) use the character count, not bytes. For any non-ASCII user/database/application_name/password, the length prefix undercounts → corrupted packet. Lines 497/505 (query.bytesize) are correct — hence the inconsistency. Fix: arg.bytesize / password.bytesize.

0.3 A debug pp left in production

src/pg/replication/error_frame.cr:11 — pp size: size writes to stderr on every replication error frame. Release-blocking (noise/professionalism). Fix: delete the line. (See also copy_data.cr:23, an IO::Memory allocated but never used.)

0.4 CI does not run on the default branch

.github/workflows/ci.yml:3-4 — trigger push: branches: [main], but the repo's default branch is master. Push CI never fires; only PRs / workflow_dispatch run. Fix: master (or rename the branch).

0.5 (RETRACTED) Decode / mid-stream errors do NOT poison a pooled connection

Retracted after reproduction. The original claim was that a decode error in move_next/read (src/pg/result_set.cr) leaves a desynced wire buffer on a pooled connection. A reproduction (single-connection pool; mid-stream SQL error via 1/(10-i), type-mismatch decode error, and query_each mid-stream error) showed the connection is resynced and reused cleanly in every case:

• a server ErrorResponse mid-stream is handled at the wire layer — handle_async_frames(ErrorResponse) (src/pq/connection.cr:220-221) does expect_frame Frame::ReadyForQuery before raising PQError, so the ReadyForQuery is consumed;
• a decode error keeps @end == false, so do_close (result_set.cr:189-213) drains the remaining rows to ReadyForQuery.

The rescue ex that sets @end = true (result_set.cr:62-65) is a defensive cleanup, but errors reaching it originate from an already-drained ErrorResponse, so it does not strand a ReadyForQuery. No fix required for connection safety. (The genuinely actionable residue is the error model, not connection state — see Tier 2: the generic raise "string" desync errors are untyped.)

---

Tier 1 — Security

1.1 verify-ca / verify-full accepted but no verification (MITM)

src/pq/connection.cr:52 forces ctx.verify_mode = OpenSSL::SSL::VerifyMode::NONE with the comment "currently emulating sslmode require not verify_ca or verify_full", while conninfo.cr:168-171 accepts verify-ca/verify-full. A user asking for verify-full gets an unauthenticated TLS channel with no error — direct MITM exposure. The README (l178) even warns you're "not safe unless fully verifying". Requested by #240, #237, #231 (Azure / CockroachDB users blocked). Fix: implement verification (VerifyMode::PEER + ca_certificates + hostname check) for those modes, or raise loudly if requested. The #1 security item for 1.0.

1.2 Over-permissive .pgpass read anyway

src/pq/pgpass.cr:14-18 — the permission check warns but reads the password anyway. libpq ignores a group/other-readable .pgpass. The spec (pgpass_spec.cr:55) only asserts the warning. Fix: return nil (skip) when perms are too open, like libpq. The 0o7177 mask is also slightly off (libpq = & 0o077 != 0).

1.3 PGSSLMODE / PGSSLCERT / PGSSLKEY / PGSSLROOTCERT env vars ignored

src/pq/conninfo.cr — none of these are read; an operator setting PGSSLMODE=require silently gets :prefer. PGCONNECT_TIMEOUT, PGOPTIONS also unhandled. Fix: fall back default_sslmode to ENV["PGSSLMODE"]?, etc.

1.4 Channel binding is untested

src/ext/openssl.cr (scram_signature) — the tls-server-end-point derivation reads correctly but has zero tests. The cert hash algorithm choice is exactly what needs a test vector before 1.0.

---

Tier 2 — Correctness & concurrency

Correctness

• RowDescription fields swapped + truncated — src/pq/frame.cr:205-207, field.cr:5-7. Reads col_oid as i32 then table_oid as i16. The protocol order is table_oid(i32), col_attr(i16), type_oid(i32)…: the bytes land correctly by coincidence (4+2), but the semantics are swapped and table_oid (an i32 OID) is stored in an i16 → truncated. Fix: read table_oid(i32) then col_number(i16), retype Field.
• infinity / -infinity unhandled — src/pg/decoder.cr:420-430. Postgres sends Int64::MAX/MIN (and Int32::MAX/MIN for date); the code does JAN_1_2K + micros → corrupt/overflowing Time. Fix: detect the sentinels.
• Numeric#to_s fractional truncation fragile — src/pg/numeric.cr:120-136. Mixes dscale and str.size (the significant length, not the 4-wide NBASE group width) to compute a slice index → wrong rendering when the last NBASE digit has leading zeros. Fix: build each group as a 4-char zero-padded string, concatenate, take exactly dscale.
• Two array paths, two lower-bound behaviors — src/pg/decoders/array_decoder.cr:40-51 (pads synthetic nils indistinguishable from real NULLs) vs :86-93 (ignores lbound). Inconsistent and both debatable for lbound≠1. Fix: unify, preserve lbound metadata.
• CharDecoder byte-vs-char — src/pg/decoder.cr:108-114. "char" (oid 18) is one byte, not a Unicode Char; [0] on byte 128–255 → replacement char, bytesize==0 → IndexError. Fix: build the Char from the byte.
• escape dead fast-path — src/pg/escape_helper.cr:72. num_backslashes == num_quotes == 0 parses as (Bool) == 0 → always false: the escaping loop always runs (not an injection hole — the loop is correct — but a latent bug + perf cliff). Fix: == 0 && == 0.
• Prepared = unprepared — src/pg/connection.cr:26-32, statement.cr, pq/connection.cr:506. Both build the same Statement; the Parse is always unnamed → nothing is prepared/reused server-side. Corroborated by #146 / #97. Document or implement (see 4.2).
• Polygon/array: count not bounded — src/pg/decoder.cr:302-303. Array.new(count) from a malformed frame → OOM. Fix: validate count against bytesize.
• Minor robustness — version indexes vers[0]/[1] unguarded (connection.cr:106, IndexError with no dot); process_ssl_message (connection.cr:71-86); negative ncols unguarded (connection.cr:160).

Concurrency / fiber-safety

• LISTEN/NOTIFY loop shares the socket with no lock — src/pq/connection.cr:183-192. The @mutex only guards close; no send_*/read_* is synchronized. A query on the same connection while the async loop reads → two fibers in read on one socket → framing corruption. Safe only because ListenConnection is dedicated — document as a 1.0 invariant and flag Connection#listen as not-for-general-use.
• @@location_cache not fiber-safe — src/pg/connection.cr:68. Class-level Hash mutated by time_zone across fibers/connections without a lock; clear_time_zone_cache compounds it. Fix: Mutex or fiber-safe memoization.
• Replication: keepalive vs reconnect — src/pg/replication.cr. The read loop reads socket without the write_mutex while the keepalive fiber writes; reconnect reassigns @conn under the mutex while an in-flight keepalive captured the old socket. last_wal_byte_flushed/applied and @closed are plain property/Bool read/written cross-fiber (OK in practice on 64-bit, formal race). Fix: capture @conn once under the lock in send_keepalive!; Atomic(Int64) / Atomic::Flag.
• @soc reassigned (negotiate_ssl) read everywhere unguarded — connection.cr:38-62, TOCTOU with close.

Error model

• Two disjoint exception trees — src/pq/error.cr (PQ::PQError, PQ::ConnectionError < ::Exception) vs src/pg/error.cr (PG::RuntimeError < PG::Error < ::Exception). A server-side SQL error surfaces as PQ::PQError, so rescue PG::Error won't catch it. No common rescuable root. Fix: unify under one documented PG:: tree.
• PG::Error is :nodoc: while it's the root users must rescue — contradictory and SemVer-hostile.
• Generic raise "string" on protocol desync — statement.cr:27 ("expected RowDescription or NoData, got …") — raises a bare Exception, indistinguishable from any other and not under a PG/DB error root. This is an error-model/typing gap (not a connection-poisoning bug — see retracted 0.5). Fix: raise a typed DB::Error/PG::RuntimeError.
• Replication: frame loop swallows decode errors — src/pq/connection.cr:204-206 (rescue e; Log.error) → tight spin on a desynced stream, run_loop never sees the error → no reconnect. Fix: propagate/break.
• Replication: ErrorFrame ignored — src/pg/replication.cr:182-183 no-op → server errors (slot conflict, missing publication) invisible. Fix: raise/surface.
• Swallowed exceptions — pg.cr:53-56, connection.cr:114-117 (empty rescue on close, conventional but swallows everything).

---

Tier 3 — PostgreSQL protocol completeness

Implemented (well): simple query, extended (Parse/Bind/Describe/Execute/Sync), COPY in/out (+ both for replication), auth OK/cleartext/MD5/SCRAM-SHA-256(-PLUS), SSL negotiation, async NotificationResponse/NoticeResponse/ParameterStatus, BackendKeyData parsed.

Missing:

• verify-ca/verify-full unimplemented (see 1.1).
• NegotiateProtocolVersion ('v') unhandled → connect would raise on a future 3.x protocol server. connection.cr:271-279.
• No CancelRequest — BackendKeyData (pid/secret) is parsed then discarded (connection.cr:271-279); no query cancellation (the standard async-safe mechanism). Fix: store pid/secret + a cancel on a 2nd socket.
• sslmode allow/prefer conflated — negotiate_ssl always tries SSL first; no non-SSL retry; sslnegotiation=direct (PG17) absent.
• Multi-host / failover (host=h1,h2), connect_timeout (→ TCPSocket.new with no timeout, a dead host hangs forever — a real availability bug), options unsupported (corroborated by #237/#231). conninfo.cr.
• libpq key=value quoting (password = 'a b', \ escapes) unsupported; double parsing of URI query params (conninfo.cr:80 vs 84-88).
• pgpass: \:/\\ escaping unhandled (pgpass.cr:22); socket-vs-localhost host match divergent.
• Statement/portal Close, PortalSuspended (row-limited cursors), pipelining (#155), multiple result sets not exposed.
• GSSAPI/SSPI/Kerberos auth: enum values present but raise unsupported. SCRAM SASLPrep missing (#181).

Missing types (PGValue / decoders): time/timetz (1083/1266) → fall through to bytea; inet/cidr/macaddr/macaddr8; money; bit/varbit; range types (#164); oidvector/int2vector. The unknown-OID fallback = silent ByteaDecoder (decoder.cr:504) hides gaps — prefer an explicit decoder that surfaces the OID. On the encoding side (param.cr), Time is formatted as text RFC3339 9-digit (Postgres truncates to µs) and there's no Time/Geo/Interval specialization inside arrays.

Replication: no CREATE_REPLICATION_SLOT (slot must be pre-created out of band); proto_version '1' pinned (no streaming/two-phase/column-list/row-filter, transaction_id fields commented out); binary 'true' hard-coded and inconsistent with the text tuple API; the doc references PG.listen_replication while the method is PG.connect_replication (replication.cr:12 vs pg.cr:34).

---

Tier 4 — Perf / memory / CPU

• H1 — decoder resolved by per-value OID hash — src/pg/result_set.cr:173-179, decoder.cr:506. Decoders.from_oid(oid(index)) = Hash lookup + not_nil! for every column of every row, though the mapping is fixed for the result set. Fix: precompute decoders : Array(Decoder) per column when @fields is received. Biggest easy win.
• H2 — read_data_row allocates Array(Slice?) + a Slice copy per column/row — src/pq/connection.cr:159-172. The crystal-db streaming path avoids it, but this buffering path stays heavy. Fix: reuse a buffer / read straight into the decoder.
• H3 — no real prepared statements — statement.cr:10-32. Parse/Bind/Describe/Execute/Sync on every exec, SQL re-parsed server-side + args re-encoded; no named statement. Corroborated by #146/#97. Fix: cache named statements keyed by SQL (major win for prepared-heavy workloads).
• H4 — args.map { Param.encode } allocates an Array + String+to_slice per value (Time via Format.format). Consider binary encoding (Time/Int/Float) + a per-connection scratch buffer. Corroborated by #294 (binary bind args, ~44% fewer bytes/exec).
• H7/to_f64 — NumericDecoder allocates Array(Int16) per value (decoder.cr:495); to_f64 (numeric.cr:65) allocates a reverse + enumerator; to_big_d/r a BigInt per digit. version re-parses on each call (connection.cr:106).
• MED/LOW — send_bind_message two passes + small writes (OK, buffered); rows_affected split (frame.cr:234); decode_array dim_info[1..-1] slice per level. SSL throughput is also reported as a concern (#303).

---

Tier 5 — Public API / SemVer / 1.0

• PQ::* boundary to decide — all PQ is :nodoc: but public in practice: PQ::Notification/Notice/Field/ConnInfo/PQError come back through public callbacks. :nodoc: only hides docs. Decide what's public (at least Notice/Notification/Field/ConnInfo/errors) and mark the rest unstable, before freezing the wire layer at 1.0.
• PQ::Connection#soc + protocol I/O public — connection.cr:16. getter soc + dozens of send_*/read_*/write_* are public and let callers corrupt state. Fix: protected/private.
• sslmode is a Symbol — conninfo.cr:27. :"verify-full" accepted but unverified (see 1.1). Fix: enum + enforce/reject.
• Global mutable decoder registry — decoder.cr:504-514, @@decoders without synchronization; register_decoder is the documented extension mechanism but Decoders is :nodoc:. Stabilize + document (registration at load-time only). PR #250 (citext) reworks exactly this.
• Inconsistent value types — Geo::Path is a hand-written struct with no ==/hash/to_s among record siblings (geo.cr:8-18); Numeric exposes a mutable digits : Array(Int16) and has no ==/hash (1.10 vs 1.100 unequal); Interval getters lack explicit types. Align before 1.0.
• Exception hierarchy (see Tier 2) — unify, reconsider the PQError name (double prefix).
• Ad-hoc returns — version → non-evolvable NamedTuple; to_spans → Tuple. Consider structs.
• SemVer-fragile monkeypatches — src/ext/openssl.cr, IO::Sized#read_remaining= (result_set.cr:1-8, dead code: the comment says to remove it, crystal >= 1.0 is required), DB::ResultSet#read reopens. Raising the Crystal floor (>= 1.1) and deleting the dead backport branches shrinks the fragile surface.
• Nits — "invalid paramater" (conninfo.cr:69), "prepared statment" (connection.cr:506), VERSION duplicated in shard.yml/version.cr (manual sync).

---

Tier 6 — Tests / CI / packaging

Coverage table (source → tested):

File	Tested	Note
pg/driver,connection,decoder,decoders/array,geo,interval,numeric	yes	real round-trip specs
pg_ext/big_decimal,big_rational	yes	via numeric_spec
pq/conninfo,pgpass,param	yes	no DB needed
pg/escape_helper	partial	2 error cases commented out
pg/replication/*	partial	gated on wal_level=logical; proto ≥2 untested
pg/copy_result,result_set,statement,error	partial	COPY write / direct parse-bind uncovered
pq/connection (auth)	partial	specs disabled by default, clientcert pending
ext/openssl.cr	no	channel binding — 0 test
pq/frame.cr	no	wire parsing — 0 direct test
pq/query.cr	no	0 direct test
pg/record.cr	no	macro/Reader never invoked
pq/field,notice	no/partial	indirect only

CI / packaging:

• CI exists (.github/workflows/ci.yml) but is nix-only (nix flake check). Postgres matrix 18/17/16/15/14 (good), but a single Crystal while shard.yml promises >= 1.0, < 2.0. Add a Crystal matrix (min 1.0 + latest).
• CI on main ≠ master (see 0.4) → push CI is silent.
• CONTRIBUTING.md:12 references a vanished .circleci/config.yml (stale docs).
• Depends on crystal-db ~> 0.14.0, itself pre-1.0 → a transitive blocker for a true 1.0.
• Security-sensitive specs to re-enable: escape error-paths, auth (md5/SCRAM/cleartext/SSL cert). No concurrency tests (a pooled driver with a Mutex), no large-data / disconnect-mid-row tests despite recent disconnection-handling commits.

Unaudited / zero-coverage hot-path files: pq/frame.cr, pq/query.cr, ext/openssl.cr, pg/record.cr.

---

Field map: open PRs & issues (mapped by tier)

Surveyed from will/crystal-pg (10 open PRs + 28 open issues). Many issues corroborate the findings above — these are not theoretical; users have been blocked for years.

Open PRs → tier

PR	Subject	Tier	Status / note
#304	range + multirange (PG::Range(T))	3	open, non-draft, recent — candidate to close the ranges gap
#300	network types + ranges + cursor + Numeric#==	3 + 5	open; overlaps #304/#213; extract the non-range parts
#295	binary params	4	draft; addresses H4; author has doubts (int4→int8 autocast); fixes #294
#253	pipelining	3	draft; closes #155
#250	citext + extension API + per-connection decoders	5 + 3	draft; addresses the "global mutable registry" finding
#213	ranges (incomplete)	3	superseded by #304
#132	bigdecimal	3/5	2018, likely obsolete (pg_ext/big_decimal.cr exists)
#121	async notifications break RETURNING	2	2017; real symptom of the shared async socket
#44	"Use MemoryIO"	4	2016, almost certainly obsolete

3 PRs overlap on ranges (#304/#300/#213): only one should land. Tier 0 and Tier 1 have no PR — a zero-conflict contribution space.

Open issues → tier (✓ = corroborates a finding)

Tier	Issues	Corroboration
1 — Security	#240 (verify-full, help wanted), #237, #231 (CockroachDB: verify-full + options param)	✓ 1.1 — users blocked (Azure, CockroachLabs)
2 — Correctness / error / recovery	#196 (pool never recycles broken connections — IO/disconnect, partly addressed by recent disconnection-handling commits; needs its own repro), #42 (reset, help wanted), #287 (Supabase/Supavisor), #263 (can't cast to JSON::Any), #252 (Time loses precision), #208 (message nil), #202 (query_all drops the first row), #150 (Array(Int) without DB type)	✓ JSON decoder; ✓ Time encoding; ✓ array dispatch (NB: decode-error connection poisoning was retracted — see 0.5)
3 — Completeness	#164 (ranges), #155 (pipelining), #281 (text-format results), #181 (SCRAM SASLPrep, filed by will), #237/#231 (options), #115 (composite type), #126 (BigDecimal)	✓ missing types/protocol
4 — Perf	#303 (sslmode=disable boosts throughput), #146 / #97 ("prepared statements bypass PARSE"), #294 (binary bind args)	✓ H3 (prepared = unprepared); ✓ H4; TLS overhead
6 — Tests / docs	#128 (lowercase SQL keywords in specs), #109 (hosted docs)	—

Likely obsolete / config: #200 (RAISE EXCEPTION → on_notice), #192 (cleartext → documented), #111 (to_json), #144, #64 (old crystal-db mapping).

Strategic reads

1. #196 (broken pool on server restart): an IO/disconnect-recovery concern (distinct from the retracted 0.5). Recent commits ("Handle disconnections from the server") likely address much of it; it needs its own reproduction before any fix.
2. Tier 1 + Tier 2: high value × no competing PR × demand proven by long-standing issues — the best entry point for contribution.
3. For the umbrella issue, each tier can cite the issues it closes — a strong argument for the maintainer (who filed #181 themselves).

---

Proposed plan (à la mcp.cr#4)

One parent (umbrella) issue + one child issue per tier (independently discussable), and a first PR on Tier 0 (the 5 blockers), each with a failing spec first, plus the CI fix so they can't recur.

Two questions before sending code:

1. Spec framework for new specs — stdlib spec (what the suite uses)?
2. For the completeness gaps (Tier 3) and security (Tier 1): implement (real TLS verification, missing types, cancel) or stop advertising (reject verify-*, stop accepting what isn't handled) until they exist? Happy either way.