Road to 1.0: comprehensive audit + offer to contribute fixes

[n-rodriguez]

Hi 👋

While evaluating crystal-pg for a Crystal project, I ran a full multi-dimension audit toward a 1.0 tag (correctness, concurrency / fiber-safety, error model, perf/memory/CPU, PostgreSQL protocol completeness, public API / SemVer surface, tests / CI). I want to lead with the honest part: the architecture is genuinely good. A clean two-layer split — the wire protocol in PQ::* cleanly separated from the crystal-db driver in PG::* — correct binary decoders for the large majority of types, SCRAM-SHA-256 + channel binding (rare and well done), logical replication (pgoutput), and a real round-trip spec suite against Postgres 14–18. The bones are solid.

This is meant as a constructive map of what stands between today and a 1.0 that can be promised — and I'm offering to contribute the fixes, starting with the blockers and the most-requested security/recovery gaps.

📄 Full audit (canonical, cited file:line): https://gist.github.com/n-rodriguez/4cbd8a46b0892adc8305156c70273a50

Summary by tier

Tier	Theme	Headline
0	Blockers	PG::Interval#to_spans calls methods that don't exist (won't compile when called); .size instead of .bytesize corrupts the startup/password packet for non-ASCII; a stray pp debug print in replication; CI triggers on main while the default branch is master (push CI never fires); a decode error poisons a pooled connection silently
1	Security	verify-ca/verify-full are accepted but the TLS context is hard-coded VerifyMode::NONE — no cert/hostname verification (silent MITM exposure); an over-permissive .pgpass is warned about but read anyway; PGSSLMODE/PGSSL* env vars are ignored
2	Correctness & concurrency	RowDescription table_oid/column fields swapped + truncated; infinity/-infinity timestamps unhandled; Numeric#to_s fractional truncation is fragile; two array code paths disagree on non-1 lower bounds; the LISTEN/NOTIFY loop shares the socket read path with queries without a lock; @@location_cache is mutated cross-fiber without synchronization; two disjoint exception trees (PQ::PQError is not under PG::Error)
3	Protocol completeness	verify-* unimplemented; NegotiateProtocolVersion unhandled; no CancelRequest; missing types (time/timetz, inet/cidr/macaddr, money, ranges); connect_timeout/options not honored (a dead host hangs forever); replication: no slot creation, proto pinned to v1, ErrorFrame ignored
4	Perf / memory / CPU	decoder resolved by per-value OID hash lookup instead of per-column; no real prepared statements (Parse on every exec — the unnamed statement); Array(Slice?) + per-column copy per row; text param encoding allocates per value
5	API / SemVer	the whole PQ::* namespace is :nodoc: but public in practice (callbacks return PQ::Notification/Notice); PQ::Connection#soc + protocol I/O are public; sslmode is a Symbol; Geo::Path is inconsistent with its sibling records; PG::Error is :nodoc: yet is the root users must rescue; dead pre-Crystal-1.0 monkeypatches
6	Tests / CI / packaging	CI is nix-only with a single Crystal version; pq/frame.cr, pq/query.cr, ext/openssl.cr, pg/record.cr have zero coverage; auth & escape-error specs are disabled by default; depends on crystal-db which is itself pre-1.0

All headline compile / wire-format / security findings were verified by hand against the source.

How this maps to existing PRs and issues

Several long-standing issues corroborate these findings — they're not theoretical:

• Tier 1 (security) is requested by Proper implementation of sslmode=verify-full #240 (verify-full, help wanted), cockroach db #237, Unable to connect to Cockroachdb #231 (CockroachDB users blocked on verify-full + the options param). No open PR addresses it.
• Tier 2 (error model / pool recovery): Connection pool unreliable? #196 (a single outage leaves the pool permanently broken) and Reset connection #42 (reset) are exactly the "decode/IO error doesn't mark the connection lost" finding; Error: can't cast to JSON::Any #263 (can't cast to JSON::Any) and Time seems to drop precision when passed in as an arg using at_end_of_day #252 (Time arg loses precision) are decoder/encoder findings. No open PR.
• Tier 3 (completeness): ranges (Support range types #164) → PRs add support for PostgreSQL range and multirange types #304 / Claude Code suggestions #300 / add support for ranges #213 (3 overlapping, only one should land); pipelining (Support for query pipelining? #155) → draft Add pipeline support #253; SCRAM SASLPrep (scram: SASLPrep for passwords #181, filed here).
• Tier 4 (perf): "prepared statements bypass PARSE" (Are prepared statements supposed to bypass PARSE? #146 / Support PREPARE statements #97) is the prepared = unprepared finding; binary bind args (Use binary encoding for bind arguments #294) → draft Encode query params in binary instead of text #295.

The takeaway: Tier 0 and Tier 1 have no competing PRs but multiple corroborating issues — that's where I'd like to start.

Proposed plan

I'll open one child issue per tier (linked below) so each is independently discussable, and I'd like to start with a PR for Tier 0 (the blockers) — each with a failing spec first, plus the CI branch fix so they can't recur — and then Tier 1 (verify-full, closing #240/#237/#231).

Two questions before I send code:

1. Spec framework for new specs — stdlib spec (what the suite uses today)?
2. For the completeness gaps (Tier 3) and security (Tier 1): prefer to implement them (real TLS verification, missing types, query cancel), or to stop advertising them (reject verify-* and unsupported sslmodes loudly) until they exist? Happy either way.

Thanks for building and maintaining this — it's the reference Postgres driver for Crystal, and I'd be glad to help push it toward 1.0. 🙏

ℹ️ Transparency: this audit was produced with Claude Opus 4.8 [1M] (multi-pass static review), then the headline compile / wire-format / security findings were verified by hand against the source.

[n-rodriguez]

Correction — Tier 0 finding 0.5 retracted.

While preparing fixes I reproduced finding 0.5 ("a decode error silently poisons a pooled connection") against a CI-faithful local cluster, and it does not occur. In every case the pooled connection is resynced and reused cleanly:

• A mid-stream ErrorResponse is drained at the wire layer — handle_async_frames(ErrorResponse) (src/pq/connection.cr:220-221) calls expect_frame Frame::ReadyForQuery before raising PQError.
• A decode/type-mismatch error keeps @end == false, so ResultSet#do_close drains the remaining rows to ReadyForQuery.

So 0.5 is withdrawn (it was the one finding I had not yet hand-verified when filing). The genuinely actionable residue is the error model, not connection state: the generic raise "string" desync errors (statement.cr:27) are untyped — that moves to Tier 2.

The Tier 0 PR (#308) was never affected — it deliberately excluded 0.5. The gist has been updated. Apologies for the noise; better to correct it in public than leave a wrong blocker on the board.

[jgaskins]

It would genuinely be nice to have some of the proposed changes. However, there are some challenges.

The first is that there are no commits in this repo's history written by an LLM. That's not to say it can't or won't ever happen, but accepting vibe-coded contributions comes with a cost. We need to figure out whether we're willing to accept AI-generated code in the repo at all. We've had some internal discussion, but nothing definitive yet.