diff --git a/AGENTS.md b/AGENTS.md index 1845f179..04e513ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,7 @@ decision that already has an ADR. | `docs/plans/*.md` | Per-effort plans (performance, bench module, compat suites). | | `docs/jdbc-conformance-matrix.md`, `docs/pooler-compatibility.md` | Behavior matrices. | | `docs/api-surface.md` | The enumerated public API surface (ADR-0020), enforced by `ApiSurfaceManifestTest`. A new public type fails the build until it is listed as Stable or Experimental. | +| `docs/static-analysis.md` | Which analysers the build runs and why, which were rejected, and the scope agreed for the ones not yet adopted. | | `docs/life-of-a-query.md` | Best single orientation doc for the core execution path. | | `docs/reviews/`, `docs/benchmarks/` | Findings from past audits; benchmark reference numbers. | | `compat-suites/` | pgjdbc and Hibernate upstream suites run against our driver, with committed baselines. | @@ -111,21 +112,50 @@ PgBouncer, auth, unix socket) provision their own containers and skip entirely when `pg.it.host` is set. `scripts/run-integration-matrix.sh` sweeps server versions; ADR-0004 defines the supported range (9.1-18; PRs gate on 14-18). -### Build gates (these fail the build at `verify`, not at `test`) - -Run `./mvnw verify` before declaring a change done. The gates are: - -- **Spotless** (`palantirJavaFormat`, 4-space). Fix with `./mvnw spotless:apply` - before committing. Formatting is not checked by `mvn test`, so it is the most - common late surprise. -- **`AsciiSourcePolicyTest`** (in `postgresql-client`, runs in the `test` phase): - every file under any module's `src/`, everything under `docs/`, and the root - `README.md` must be 7-bit ASCII. No em-dashes, curly quotes, or arrows. +### Build gates + +Run `./mvnw verify` before declaring a change done. They do not all fire at the +same phase, which is worth knowing when a build fails early: the compiler gates +fail at `compile` and the source-policy tests at `test`, so a plain `mvn test` +already enforces them. Spotless and JaCoCo wait for `verify`, which is what makes +formatting the most common late surprise. + +**In the compiler** (configured in the root `pom.xml`, so every build gets them): + +- **`-Xlint:all,-this-escape` under `-Werror`**: any javac warning fails the + build. `this-escape` is the sole exclusion. +- **NullAway at ERROR** over `org.postgresql.client.protocol` and + `org.postgresql.client.core`: a nullness violation there fails the compile. + Those prefixes are listed in `NullAway:AnnotatedPackages` in the root POM, so + everything under them is non-null by default; mark the exceptions with + jspecify's `@Nullable`. Error Prone is present only as NullAway's carrier, with + its own checks disabled on purpose -- do not "fix" the `-XepDisableAllChecks` + flag. See `docs/static-analysis.md`. + +**Source-policy tests** (plain unit tests, so they run in the `test` phase): + +- **`AsciiSourcePolicyTest`** (in `postgresql-client`): every file under any + module's `src/`, everything under `docs/`, and the root `README.md` must be + 7-bit ASCII. No em-dashes, curly quotes, or arrows. +- **`NoSynchronizedSourcePolicyTest`** (in `postgresql-client`): the `synchronized` + keyword is banned outright across the four shipped modules, per ADR-0001. Use + `ReentrantLock`. A use with genuinely no I/O beneath it goes in that test's + `ALLOWED` set with its reason, which keeps it a reviewed exception. +- **`ApiSurfaceManifestTest`** (in `postgresql-client`): every public type in an + exported package must be listed in `docs/api-surface.md` as Stable or + Experimental. - **`ModuleLayeringTest`** (in `postgresql-client-jdbc`): `postgresql-client-jdbc` and `postgresql-client-pgjdbc-compat` must not import `org.postgresql.client.protocol.*` directly; they reach it only transitively through core. -- **JaCoCo floor**: 20% line and branch per module bundle. A tripwire, not a target. -- **Enforcer**: Java/Maven minimums, dependency convergence, reactor convergence. + +**Later phases:** + +- **Enforcer** (`validate`): Java/Maven minimums, dependency convergence, reactor + convergence. +- **Spotless** (`verify`; `palantirJavaFormat`, 4-space). Fix with + `./mvnw spotless:apply` before committing. +- **JaCoCo floor** (`verify`): 20% line and branch per module bundle. A tripwire, + not a target. ### Test conventions @@ -217,11 +247,14 @@ commit. See each suite's `README.md`. ## Common pitfalls -- Formatting and ASCII failures only appear at `verify`; run - `./mvnw spotless:apply` before committing and avoid pasting Unicode punctuation. +- Formatting failures only appear at `verify`; run `./mvnw spotless:apply` before + committing. ASCII failures surface earlier, at `test`, so avoid pasting Unicode + punctuation into source, docs, or commit messages. - A new public package without a `module-info.java` export compiles in-module and breaks downstream. - `synchronized` around blocking I/O pins virtual threads: use `ReentrantLock`. + `NoSynchronizedSourcePolicyTest` rejects the keyword in shipped code, so this + fails the build rather than showing up later as a latency mystery. - Do not let pgjdbc's legacy behavior leak into core; it belongs in the JDBC and compat layers. Core changes for compat parity need a real core-level reason. - `-Dtest=...` across the reactor fails modules that lack the class unless you diff --git a/README.md b/README.md index 383e3430..bc16d4c5 100644 --- a/README.md +++ b/README.md @@ -33,13 +33,33 @@ implemented on top of it. The test suite includes integration tests that run against real PostgreSQL (and PgBouncer) via Testcontainers. APIs are still evolving and there are no compatibility guarantees yet. The implementation roadmap (phased, commit-by-commit) lives in -[docs/plans/overall.md](docs/plans/overall.md), which also defines the -"first useful JDBC release" minimum. +[docs/plans/overall.md](docs/plans/overall.md). ## Connecting -The JDBC layer registers itself for the `jdbc:pg:` scheme (ADR-0015), and takes -libpq-style parameters: +The PostgreSQL-native API is the product; nothing in it mentions `java.sql`. +`PgConnections.connect` returns a `PgConnection`, and `execute` returns a pull +cursor over the rows: + +```java +PgConnectionConfig config = PgConnectionConfig.builder() + .host("localhost", 5432) + .database("appdb") + .user("app") + .password(secret) + .sslMode(SslMode.VERIFY_FULL) + .build(); + +try (PgConnection connection = PgConnections.connect(config); + PgResultStream rows = connection.execute("select id from t where name = $1", List.of("widget"))) { + while (rows.next()) { + System.out.println(rows.currentRow().getLong(1)); + } +} +``` + +The JDBC layer sits on top of that and registers itself for the `jdbc:pg:` +scheme (ADR-0015), taking libpq-style parameters: ```java String url = "jdbc:pg://localhost:5432/appdb?user=app&sslmode=verify-full"; @@ -56,9 +76,7 @@ try (Connection connection = DriverManager.getConnection(url, "app", secret); It also answers to `jdbc:postgresql:` for ported applications, though which driver claims that scheme when real pgjdbc is also on the path is a decision of -its own (ADR-0015). The PostgreSQL-native API underneath it is -`org.postgresql.client.core.PgConnections.connect(...)`, which returns a -`PgConnection` and never mentions `java.sql`. +its own (ADR-0015). ## Project layout @@ -85,20 +103,14 @@ modules together. The modules are: place, but some `PGConnection` methods are still stubbed and several functional paths await live-server verification. Treat the "drop-in" goal conservatively until that ring fully lands (see `docs/follow-up.md`, N4.3). -- **`postgresql-client-bench`** - A driver-agnostic, pgbench-style JDBC benchmark that - measures the driver, not the database. The driver under test (pg-java or pgjdbc) - is supplied at runtime, never bundled. Build-only; not published. See - `postgresql-client-bench/README.md`. -- **`postgresql-client-bench-jmh`** - Server-free JMH micro-benchmarks for the codec, - framing and buffer paths, where allocation (bytes/op) is the tracked number. See - `docs/benchmarks/` for the reference runs. Build-only; not published. -- **`postgresql-client-coverage`** - Aggregates per-module JaCoCo coverage into one - combined report. Build-only; not published. -- **`postgresql-client-native-smoke`** - A GraalVM native-image smoke gate: it compiles the - native-image metadata shipped by `postgresql-client-jdbc` and `postgresql-client-pgjdbc-compat` into - a native binary and runs it, catching metadata regressions. Off by default (a - plain build only compiles the probe); the native build runs under the - `native-smoke` profile on a GraalVM JDK. Build-only; not published. +Four more modules are build-only and never published: **`postgresql-client-bench`** +(a driver-agnostic, pgbench-style JDBC benchmark that measures the driver, not the +database; see `postgresql-client-bench/README.md`), +**`postgresql-client-bench-jmh`** (server-free JMH micro-benchmarks where +allocation is the tracked number; reference runs in `docs/benchmarks/`), +**`postgresql-client-coverage`** (aggregated JaCoCo report), and +**`postgresql-client-native-smoke`** (a GraalVM native-image gate that builds and +runs a binary from the shipped metadata, under the `native-smoke` profile). ## Requirements @@ -131,23 +143,9 @@ stays Docker-free. They require a running Docker daemon: mvn verify -Pintegration-tests ``` -By default the integration tests run against `postgres:17`; pass -`-Dpg.it.image=` to test another server image, or use -`scripts/run-integration-matrix.sh` to sweep several PostgreSQL versions. - -To run the shared-server integration tests against an already-running -PostgreSQL instead of Docker, set `-Dpg.it.host`: - -```sh -mvn verify -Pintegration-tests -Dpg.it.host=localhost -``` - -The remaining coordinates default to port `5432` and `postgres` for the user, -password, and database; override them with `-Dpg.it.port`, `-Dpg.it.user`, -`-Dpg.it.password`, and `-Dpg.it.database`. The special-purpose harnesses -(TLS, PgBouncer, auth, unix socket) provision their own containers and are -skipped entirely when `pg.it.host` is set, so an external-server run never -touches Docker. +They default to `postgres:17`. Selecting another image, sweeping the version +matrix, and pointing the suite at an already-running server instead of Docker +are covered in [AGENTS.md](AGENTS.md). ## Contributing diff --git a/docs/adr/ADR-0002-module-boundaries-and-dependency-policy.md b/docs/adr/ADR-0002-module-boundaries-and-dependency-policy.md index eb8cbd2e..233f28a8 100644 --- a/docs/adr/ADR-0002-module-boundaries-and-dependency-policy.md +++ b/docs/adr/ADR-0002-module-boundaries-and-dependency-policy.md @@ -40,3 +40,12 @@ JDBC module. versions. - JDBC ergonomics never dictate core API shape; the JDBC layer is a thin adapter. - Optional integrations cannot pull weight into core. + +## Amendments + +- **Protocol types ride on core's public surface (pre-1.0, N7.3).** Core declares + `requires transitive org.postgresql.client.protocol` and the codec SPI names + `PgWriteBuffer` and `ByteSlice`, so a caller writing a codec compiles against + protocol types. The layering rule above is unaffected -- nothing flows upward -- + but "protocol is an implementation detail of core" is not true of the exported + API. Decide before the API freeze whether to bless that or unweld it. diff --git a/docs/adr/ADR-0003-testing-strategy.md b/docs/adr/ADR-0003-testing-strategy.md index bf921a06..39d7ffe7 100644 --- a/docs/adr/ADR-0003-testing-strategy.md +++ b/docs/adr/ADR-0003-testing-strategy.md @@ -15,8 +15,11 @@ A layered test strategy combining unit tests, ground-truth fixtures, a mock server, and a gated integration matrix. - Unit tests (JUnit 5) in each module; protocol/orchestration tests use in-memory - transports and ground-truth golden fixtures captured from libpq/Wireshark/psql - (not hand-derived from the same reasoning that wrote the encoder). + transports and golden fixtures that are independent of the encoder (not + hand-derived from the same reasoning that wrote it). The shipped fixtures are + spec-derived, built by hand from the message-format documentation; a + capture-based libpq/Wireshark/psql corpus is the stronger ground truth and + remains aspirational (N7.3). - A programmable mock PostgreSQL server (test-only) drives auth edge cases, protocol-version negotiation, protocol violations, and server-closes-mid-query. - Integration tests use Testcontainers plus a PostgreSQL image via the Failsafe diff --git a/docs/adr/ADR-0013-pooler-and-proxy-compatibility.md b/docs/adr/ADR-0013-pooler-and-proxy-compatibility.md index 1aaa768a..e169edbf 100644 --- a/docs/adr/ADR-0013-pooler-and-proxy-compatibility.md +++ b/docs/adr/ADR-0013-pooler-and-proxy-compatibility.md @@ -25,5 +25,8 @@ transaction pooling, with CI coverage where feasible. - The driver works correctly behind transaction poolers when configured for it, instead of silently relying on session state that the pooler does not preserve. - Feature availability under pooling is documented, not discovered in production. +- Pooled logical handles (`ConnectionPoolDataSource`) do not reset session state + between handles. That is pgjdbc parity and a documented contract, not a gap to + fix: an application that changes session state on a pooled handle must undo it. - How rich the pooler-compatible mode should be (which features to actively police versus merely document) remains an open question to revisit. diff --git a/docs/adr/README.md b/docs/adr/README.md index 3ae73ba0..b21d158b 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -2,8 +2,9 @@ This directory records the architectural decisions for pg-java as individual, numbered ADRs. Each ADR captures one decision: its context, the decision itself, -and the consequences. The implementation plan (`docs/plans/overall.md`) carries a -summary of each decision inline; these files are the canonical, citable record. +and the consequences. These files are the canonical, citable record; the +implementation plan (`docs/plans/overall.md`) cites them rather than restating +them. ADRs are append-only in spirit: once accepted, an ADR is superseded by a new one rather than edited away. A decision that survives but whose details have moved on diff --git a/docs/benchmarks/micro-bytes-per-op-001.md b/docs/benchmarks/micro-bytes-per-op-001.md index d2e09545..feea3e94 100644 --- a/docs/benchmarks/micro-bytes-per-op-001.md +++ b/docs/benchmarks/micro-bytes-per-op-001.md @@ -1,5 +1,8 @@ # Micro-benchmark bytes/op reference (run 001) +**Superseded by [run 002](micro-bytes-per-op-002.md)**, which is the tracked reference. Kept as +the first committed capture. + Per-operation heap allocation (`gc.alloc.rate.norm`, **bytes/op**) for the driver's server-free hot paths, from the JMH micro-harness (`pg-java-bench-jmh`, N5.1). This is the tracked allocation KPI for the write-side work (N5.7): unlike throughput, **allocation is counted, not sampled**, so diff --git a/docs/benchmarks/pgjava-vs-pgjdbc-001.md b/docs/benchmarks/pgjava-vs-pgjdbc-001.md index a524bc61..778d4bd4 100644 --- a/docs/benchmarks/pgjava-vs-pgjdbc-001.md +++ b/docs/benchmarks/pgjava-vs-pgjdbc-001.md @@ -1,5 +1,8 @@ # Benchmark: pg-java vs pgjdbc (run 001) +**Superseded by [run 002](pgjava-vs-pgjdbc-002.md)**, which re-ran this baseline after the +compat batch-rewrite pass-through was fixed. Kept as the first data point. + First `pg-java-bench` comparison of pg-java against real pgjdbc. This is the initial data point toward the N5 exit criterion ("a committed, reproducible benchmark report shows parity-or-better on every measured path"). It measures the driver, not the diff --git a/docs/benchmarks/pgjava-vs-pgjdbc-005.md b/docs/benchmarks/pgjava-vs-pgjdbc-005.md index 8569179a..e06d2acf 100644 --- a/docs/benchmarks/pgjava-vs-pgjdbc-005.md +++ b/docs/benchmarks/pgjava-vs-pgjdbc-005.md @@ -1,5 +1,8 @@ # Benchmark: pg-java vs pgjdbc (run 005) -- batch-size sweep, repeated +**Rankings superseded by [run 006](pgjava-vs-pgjdbc-006.md)**, which re-ran them on a fixed +harness. The sweep's shape and method below still stand. + A longer, repeated **batch-size sweep** to firm up run 004's single-shot numbers. `batch-insert` across batch sizes **1, 10, 100, 500, 1000**, with batch-rewrite both **off** and **on**, for all three driver surfaces, **`--repeat 3`** per cell. Run from the new sweep diff --git a/docs/benchmarks/pgjava-vs-pgjdbc-014.md b/docs/benchmarks/pgjava-vs-pgjdbc-014.md index 1362660f..dc5209a7 100644 --- a/docs/benchmarks/pgjava-vs-pgjdbc-014.md +++ b/docs/benchmarks/pgjava-vs-pgjdbc-014.md @@ -1,5 +1,9 @@ # Benchmark: pg-java vs pgjdbc (run 014) -- wide rows, NULL-heavy rows, and a compat-layer finding +**Compat cells superseded by [run 015](pgjava-vs-pgjdbc-015.md)**: they were measured against a +stale jar (the harness bug is documented in 015). The native cells and the two new workloads +stand as measured. + Two workloads the suite never had (`wide-row`, `null-heavy`, added this run) plus `select-rows` and `batch-insert` as controls. The question they answer: everything measured before reads four to nine columns per row, which is where a driver's column-lookup strategy, row reuse and lazy decode barely diff --git a/docs/follow-up.md b/docs/follow-up.md index b89de588..0deca51b 100644 --- a/docs/follow-up.md +++ b/docs/follow-up.md @@ -59,47 +59,6 @@ notifications or the test collected one from a previous method -- our own `NotificationListenerTest` and the LISTEN/NOTIFY ITs cover the driver side and are stable across every run. -## RESOLVED -- ADR-0020 section 6: all six types are interfaces (2026-07-27) - -`PgPipeline`, `PgPipelineBatch`, `PgPipelineStatement`, `PgCopyInStream`, -`PgCopyOutStream` and `PgLargeObject` are interfaces; the implementations are -`Core*` and package-private. `docs/api-surface.md` carries the mapping. - -Nothing outside core changed: every caller named these types only for locals, -fields and one array, and the bench module's reflective lookup resolves an -interface exactly as it did a class. Two contracts were tightened while writing -them down rather than left to be inferred -- neither COPY stream's `close()` -throws, and `PgLargeObject`'s handle is only valid inside the transaction that -opened it, with the pre-9.3 fallback for 64-bit offsets stated. - -This was the one item with a deadline: after a first artifact ships, narrowing a -concrete class to an interface is a breaking change. It is done before N7.1 -publishes anything. - -## RESOLVED -- the build was mis-reporting its own tests (2026-07-27) - -The previous entry here suspected that some unit tests were not running: -`PgPipelineTest` declares 22 `@Test` methods and reported 22 alone, 16 in a full -module build and 0 under the integration profile, with per-class numbers summing -exactly to the module aggregate. - -**They were all running.** Under class-level parallelism surefire and failsafe key -each test event to whichever test set is currently open; concurrent classes -interleave, so testcases are written into another class's report file and the -per-class `tests` attribute is whatever the reporter happened to see. -`PgPipelineTest`'s 22 cases were spread across three files -- 13 in its own, 6 in -`NoSynchronizedSourcePolicyTest.xml`, 3 in `TransientRowCopyOutTest.xml` -- while -its own file claimed 16. - -Confirmed by comparing the complete set of (class, method) pairs across both -modes, not just the totals: 1964 testcases either way, identical sets, nothing -missing. That distinction mattered -- "not running" and "not reported" are very -different problems and the counts alone could not tell them apart. - -Fixed by turning class-level parallelism off (`junit-platform.properties`), which -costs 40s on a 169s build and takes mis-filed reports from 197 of 301 to zero. -The file records the measurements and the flag to trade back. - ## Statement.setMaxRows is enforced client-side only (2026-07-25) Every path caps rows in the client and lets the server produce and transmit the @@ -150,46 +109,6 @@ not to do, preferring to shade. Any of these is a structural decision and wants an ADR, not a silent change. Interacts with N7.1 (release engineering) and the GraalVM metadata in `postgresql-client-native-smoke`. -## RESOLVED -- Hibernate gate: parameterized display names are not stable (2026-07-25) - -Fixed 2026-07-27 in `compat-suites/hibernate/gate.py`: both sides of the diff are -now keyed without the `[N]` index, and keys that collide once it is dropped are -numbered so the comparison is a multiset. The baseline format is unchanged, so -committed baselines still work. Verified against the real run4 report (PASS, no -change, where it previously FAILed on 4 + 4) and against synthetic cases covering -a rotation, a genuine new failure inside and outside the flaky class, a genuine -fix, both directions of the duplicate-text collision, and the known-incompatible -bucket. The original analysis follows. - - -`compat-suites/hibernate` gates by comparing full JUnit display names against -the baseline, and those names embed the parameter index (`[7] LOAD, ...`). In -`LoadAndFetchGraphAssociationNotExplicitlySpecifiedTest` the index-to-argument -mapping is not stable between runs, so the same seven failing cases come back -under permuted indices and the gate reports them as four regressions plus four -newly-passing tests at once. - -Observed on the 2026-07-25 run (after ADR-0022): 13712 -> 13713 passed, -112 -> 111 failed, the failing *name* set identical, and the gate still FAILed. -The signature is unmistakable -- an equal number of "new failures" and "newly -passing" within one class, differing only in the bracketed index. - -Confirmed again on 2026-07-27 (after N3.8), on the cleanest possible evidence: -the run reproduced the baseline exactly -- 16030 total, 13712 passed, 112 -failed, 256 known-incompatible, and an identical known-incompatible set -- yet -the gate still FAILed on four regressions plus four newly-passing tests in that -one class. Dropping the `[N]` index from both name sets makes them identical. - -Fix by normalizing the `[N]` prefix out of the key before diffing, or by holding -that class out of the gated set the way the pgjdbc suite holds its flaky tests -out. Normalizing needs care: display text alone is not unique (the baseline has -`[13] null, null`), so drop the index only when the remaining text is unique -within the class, or compare as a multiset. Do not paper over it by refreshing -the baseline: that bakes in one arbitrary permutation and it flips again on the -next run. -Files: `compat-suites/hibernate/run.sh` (step 7, the gate), and whichever -reporter builds `report.json`'s `failed` list. - ## Hibernate suite: a reused checkout can serve stale bundles (2026-07-27) `run.sh` reuses `target/hibernate-suite/hibernate-orm` across runs. The tests @@ -240,19 +159,6 @@ locally-verifiable change (no Docker, unit tests only). Each is deferred to the milestone -- mostly N3.6 (integration tests against a live PostgreSQL), where the server-observable behaviour can actually be verified. -## RESOLVED -- N3.4 JDBC type cluster (2026-07-28) - -Every part landed: type propagation on `setNull`/`setObject(targetSqlType)` (core -`TypedNull`), `RETURNING` for a data-modifying `WITH` (N3.7), multiple result sets -(N3.8), CALL procedures + INOUT (N3.9), and generated keys by column index (N3.10). - -Two notes worth keeping. The generated-keys narrowing relies on `RETURNING *` yielding -the table's columns in declaration order, so a JDBC "column index in the table" is that -position; the equality does not hold for SQL that carries its own `RETURNING` list. And -pgjdbc does not do this at all -- it throws `0A000` for a non-empty `int[]` -- so a note -in an earlier version of this file calling our behavior "a documented approximation, as -in pgjdbc" was wrong about pgjdbc. - ## N5.7 -- statement-held parameter arrays are unsafe as specified (2026-07-28) The plan's first open N5.7 item reads "statement-held parameter arrays / per-execute churn in @@ -302,28 +208,6 @@ Files: `postgresql-client/.../ParameterEncoder.java` (`resolve` overloads), `CoreConnection.java` (`executeMultiStatementBatch` :937, `encodeDeferred` :316/:327), `CorePipeline.java` (`appendStatement` :155). -## RESOLVED -- the compat layer allocated per JDBC call (2026-07-28) - -Fixed the same day it was found. `PgResultSets.wrap` returned a JDK dynamic proxy, which -allocates an `Object[]` for the arguments of every call and boxes each primitive one -- a cost -per JDBC *call*, and on a result set that is one call per column per row. `CompatResultSet` is -now a written-out class whose delegating methods are generated from `java.sql.ResultSet`. - -Measured proxy vs direct, same box, nothing else changed: `null-heavy` 662 -> 217 bytes/op -(305% -> 100% of pgjdbc), `wide-row` 3,401 -> 1,884 (133% -> 74%), `types` 1,296 -> 1,006, -`select-rows` 512 -> 377. The compat layer now allocates exactly what the native driver does on -every read workload. Details: `docs/benchmarks/pgjava-vs-pgjdbc-015.md`. - -**Still open: the Statement and Connection proxies.** `CompatStatements` and `CompatConnections` -are still `Proxy.newProxyInstance`, so the same per-call cost applies to -`setInt`/`execute`/`prepareStatement`. It is a smaller prize -- those are per-execute and -per-connection, not per column -- and `batch-insert` sitting unchanged at 129% of pgjdbc after the -result-set fix is the visible remainder. The same generate-the-delegates approach applies; -`CompatStatements` needs three (Statement, PreparedStatement, CallableStatement), which is why it -was not done in the same pass. -Files: `postgresql-client-pgjdbc-compat/.../CompatStatements.java` (:44), -`CompatConnections.java` (:35). - ## N3.6 -- JDBC integration suite (expansion) The harness and an initial suite landed (`AbstractJdbcPostgresIT` + `DriverConnectIT`, @@ -339,97 +223,6 @@ The harness and an initial suite landed (`AbstractJdbcPostgresIT` + `DriverConne - The server-executing conformance assertions from N3.5, and the N3.4 items (CALL/INOUT, multiple result sets) once their code lands. -## N2.5 -- mutual-TLS (client cert/key) end-to-end IT -- RESOLVED (N1.18) - -Nothing deferred remains: every part below is marked DONE, closed by N1.18's -`SslMaterialsClientKeyTest` and `TlsNegativeMatrixIT`. Retained for the -container-setup rationale. - -The negative-TLS and `require_auth` refusal parts of N2.5 are covered end-to-end -(`TlsIT` trust/hostname/downgrade failures, `DirectTlsIT` config refusals, `Md5AuthIT` -`require_auth` allow/forbid, and `RequireTlsOverPlaintextIT` for `require` over a server that -declines TLS). Client-cert **support** is implemented (`SslConfig` clientCert/clientKey, -`SslMaterials.keyManagers` :75, `ConnectionStringParser` sslcert/sslkey :329-365) but is not -exercised end-to-end -- no IT sends a client certificate and the TLS container is not -configured for cert auth. A faithful test needs elaborate container setup: generate a client -CA + client cert/key, add `ssl_ca_file`, and rewrite `pg_hba.conf` to -`hostssl ... cert clientcert=verify-full`. Deferred as its own item rather than blocking N2.5. - -- **Cheap interim (no server) -- DONE (N1.18):** `SslMaterialsClientKeyTest` loads a client cert - + PKCS#8 private key (plaintext and PBES2-encrypted, right and wrong passphrases) through - `keyManagers`/`loadPrivateKey`. -- **Full IT -- DONE (N1.18):** `TlsNegativeMatrixIT` runs a dedicated `hostssl ... cert` - container: a valid client cert connects, the encrypted-key variant honors `keyPassword`, - and missing/wrong client certs fail closed. -- New residuals discovered while landing N1.18 (both fixed the next day): - - **DONE:** `PgDatabaseMetaData.getColumns` selected `pg_attribute.attgenerated` (PG 12+) and - `attidentity` (10+) unconditionally, erroring on older supported servers; the selections are - now version-branched and `DatabaseMetaDataCatalogIT` runs ungated on every matrix leg. - - **DONE:** `SslMaterials.decodePem` rejected PEM files with leading textual headers (`#` - comments, openssl "Bag Attributes") that libpq tolerates; it now decodes only the - BEGIN/END envelope, and encrypted-key detection matches the marker line. - -## RESOLVED -- array whitespace: our decoder was already right (2026-07-26) - -The previous entry here claimed our text array decoder kept whitespace -PostgreSQL's `array_in` strips, on the strength of pgjdbc's `jdbc2/ArrayTest` -failing. Checked against a real PG 16 rather than against the test, which is what -that entry said to do: - -| character | trimmed from an unquoted element? | -| --- | --- | -| space, tab, LF, CR, form feed, **vertical tab** | yes | -| NBSP (U+00A0), em quad (U+2001), any non-ASCII space | no | -| anything inside a quoted element | no | -| interior whitespace | no | - -That is exactly the set `ArrayCodecs.Parser.isAsciiWhitespace` already trims, -vertical tab included -- so there was nothing to fix, and the guess in the old -entry that vertical tab was excluded was wrong. - -**pgjdbc diverges here, knowingly.** Its parser -(`ArrayDecoding.buildArrayList`) skips any ASCII whitespace character *anywhere* -in an unquoted token, not just at the ends: the loop's whitespace branch is a -bare `continue`. So for the element ` unquot ` the server yields -`unquot `, which is what we return, and pgjdbc yields -`unquot`, having dropped the interior vertical tab and space as well as -the outer ones. - -That is a deliberate simplification, not an oversight -- `testStringEscaping` -says so in a comment, and the same test asserts *both* values: the server -round-trip yields `un quot ` and the client-side parse of the same -literal yields `unquot`. Its stated justification also checks out -against PG 16: `array_out` quotes an element containing any ASCII whitespace, -interior included (`ARRAY['a b']` prints as `{"a b"}`), so a value the server -produced is always quoted before pgjdbc's stripping could reach it. The -divergence is unreachable for server data and only shows up for a literal the -client wrote by hand, which is exactly what `testDirectFieldString` does. The -misleading part is only that test's own comment, "PostgreSQL drops leading and -trailing whitespace, so does the driver". - -We match the server instead, because it costs nothing to and because a decoder -that agrees with the thing that produced the data is the one worth having. -`testDirectFieldString` and `testStringEscaping` therefore stay red on that one -element, asserting pgjdbc's parse rather than PostgreSQL's, and must not be -"fixed" by matching them. If a future run makes them pass, something has -regressed. - -## RESOLVED -- getArray for a codec-less array type (2026-07-27) - -`getArray` no longer throws "column N is not an array" for an array whose element -type has no codec: the literal is split into text elements, with the delimiter -read from `pg_type.typdelim` rather than assumed to be a comma. Covered by -`ArrayWithoutCodecIT`, including `box[]`, whose semicolon delimiter would -otherwise cut each box in half. - -What is left of that cluster is a *compat* concern, not a core one: the elements -come back as `String`, and pgjdbc gives a `PGobject` carrying the type name. -pgjdbc's `jdbc4/ArrayTest` (`testEvilCasing`, `testCasingComposite`, -`testCreateArrayOfMultiJson`) asserts the latter. Wrapping them belongs in -`PgResultSets`, which already knows how to decide a value's pgjdbc read shape by -OID -- the array case just has to apply that per element. -Files: `postgresql-client-pgjdbc-compat/src/main/java/org/postgresql/PgResultSets.java`. - ## box[] still encodes with a comma (2026-07-27) The array *parser* takes a delimiter now; the *encoder* still writes a comma @@ -552,11 +345,14 @@ push nullability across that boundary. The findings will concentrate in the driv fields and helpers rather than in the API surface, and `ResultSet` getters (which return null for SQL NULL by contract) are the obvious first cluster. -Also still open from N1.9: forbidden-apis (cheap, and catches the default-locale bug class -`ReporterLocaleTest` exists because of), Error Prone's own checks at ERROR severity -(currently all disabled; it is present only as NullAway's carrier), SpotBugs scoped to -protocol/codec and CI-only, and raising the JaCoCo floor (`postgresql-client-jdbc` at ~27% -line is the binding constraint). +The other tools left over from N1.9 are each one commit, so they are plan items rather +than follow-ups: forbidden-apis (N1.23), Error Prone's own checks (N1.24), and SpotBugs +scoped to protocol/codec (N1.25). `docs/static-analysis.md` carries the reasoning and the +scope agreed for each. + +Raising the JaCoCo floor above 20% stays here, because it is a judgement call rather than +a task: `postgresql-client-jdbc` at ~27% line is the binding constraint, so any increase +is really a decision about how much JDBC-layer testing to fund. ## N2.7 -- cancellation-race malicious/adversarial case @@ -641,28 +437,6 @@ because their real behaviour needs a live server. Deferred ITs (JDBC integration considered core change rather than an in-loop edit. `addDataType` rides the getObject->PGobject path above; `cancelQuery` needs the core cancel wired plus a live-server functional test. -## analysis-004 Medium/Low -- prepared statement deallocate no-ops while busy -- RESOLVED (N1.14) - -Landed as **N1.14** (ticked in `docs/plans/overall.md`): `CoreConnection` holds a -`pendingDeallocations` queue that busy-time `Close` requests join, drained at the -next idle boundary. Retained for the detailed rationale below, which explains why -it could not be a small local fix. - -Deferred from the analysis-004 "Also confirmed, Medium/Low" cluster (the other genuinely small -items -- scrollable `getInt`/`getLong` range checks and the compat `Driver` volatile field -- landed). -`CoreConnection.deallocate(...)` early-returns when the connection is `busy` (mid-stream), so a -prepared statement evicted from the cache, or explicitly closed, while a result stream over a -*different* statement is still open never sends its `Close`/`Sync` -- the server-side prepared -statement leaks for the life of the connection. This is not a small, safe, purely-local fix: it -needs a deferred-deallocation queue drained at the next idle point (after the busy stream releases), -plus care not to reorder against an in-flight extended-protocol exchange, and a mock/live-server test -to prove the deferred `Close` actually reaches the wire. Track here rather than fix under the loop. - -The adjacent, already-documented cache items (byte-budget vs entry-count LRU; the eviction -`Close`+`Sync` on the cache-miss path, which runs while the connection is idle, not busy) are noted -in the C12.2 correction in `docs/plans/overall.md` and are intentional/tracked, not part of this -follow-up. - ## Binary result formats -- describe-before-bind for genuinely-unnamed statements Already done at HEAD (pinned by `BindResultFormatTest`): the named/prepared path requests per-column diff --git a/docs/plans/future-compat-suites.md b/docs/plans/future-compat-suites.md new file mode 100644 index 00000000..9bc15c96 --- /dev/null +++ b/docs/plans/future-compat-suites.md @@ -0,0 +1,45 @@ +# Plan: the compat suites we have not built yet + +Two suites past pgjdbc's and Hibernate's are worth running against +`postgresql-client-pgjdbc-compat`, and neither is started. The design is not +restated here: `test-vs-hibernate.md` is the template, and its shared primitive +-- publish our compat jars into a local Maven repo as +`org.postgresql:postgresql:` (the GAV shadow), then run the upstream +project's own PostgreSQL tests unmodified against a baselined report -- carries +over to both. Write the full plan when one is picked up; until then this records +what each buys and what makes it awkward. + +## jOOQ + +Buys the two surfaces other suites barely reach. jOOQ's code generation +reflects a live schema through `DatabaseMetaData` and direct `pg_catalog` +queries, so it stresses our metadata and catalog behavior harder than anything +we run, and it runs before any test does. Its generated-code integration tests +then hammer `ResultSet`, the full type system (arrays, enums, ranges, JSON, +custom types) and bind handling. PostgreSQL is supported by the open-source +edition, so this path is reachable. + +Awkward part: the run is two-phase. Codegen is a hard gate that must succeed +before the runtime tests execute at all, so a failure there has to be reported +distinctly from a test failure rather than as a wall of errors. + +## Spring + +Buys the issues that only appear behind an abstraction: pool integration, +transaction and savepoint semantics, `DatabaseMetaData`-driven dialect +selection, generated keys through repositories, datasource health checks. Scope +it to the DB-touching projects -- Spring Data JDBC, Spring Data JPA (which also +exercises us through Hibernate, with Spring's repository and transaction layer +on top), and Spring Boot's datasource/Testcontainers autoconfiguration tests. +Running Spring Framework whole is mostly not database work. + +Awkward part: mixed build tools. Spring Data is Maven, Spring Boot is Gradle, so +the substitution primitive has to be applied twice (a `settings.xml` plus a +forced version property for Maven, an `--init-script` dependency substitution +for Gradle) and the results merged into one report. + +## Neither has the pgjdbc harness's hardest problem + +Both projects' tests are written against their own APIs, not against +`org.postgresql` internals, so there is no compile-time exclusion problem and no +test patching. Each is a pure runtime driver swap. diff --git a/docs/plans/jdbc-bench-module.md b/docs/plans/jdbc-bench-module.md index 121c18b0..6149f48d 100644 --- a/docs/plans/jdbc-bench-module.md +++ b/docs/plans/jdbc-bench-module.md @@ -2,10 +2,8 @@ Status: **built and in use.** The module ships, `scripts/benchmark.sh` drives it, and its results are the `docs/benchmarks/pgjava-vs-pgjdbc-*.md` series. Kept as the design -record -- what the workloads are shaped to measure and why -- not as a live checklist: -the boxes below were never ticked as the work landed, and the running status is in -[`overall.md`](overall.md). Read an unticked box here as "see overall.md", not as -"not done". +record -- what the workloads are shaped to measure and why -- not as a live checklist. +The running status is in [`overall.md`](overall.md). Bring a standalone `jdbc-bench` tool (a pre-existing harness that lived outside @@ -67,7 +65,9 @@ The external tool is mature and well-factored; we copy most of it. Key pieces: `bench_ingest` (write); reset between workloads (truncate writer / `VACUUM ANALYZE` reader) outside the timed loop so results are order-independent. - **CLI** (`cli/BenchCommand.java`, picocli): the full option surface (see the tool README). -- **Matrix scripts** (`scripts/run-pgjdbc.sh`, `run-pg-matrix.sh`, `pg-docker.sh`). +- **Matrix scripts** driving the tool from outside it; they landed here as + `scripts/benchmark.sh`, `run-pgjava-vs-pgjdbc.sh`, `run-batch-sweep.sh` and + `pg-docker.sh`. ### Validated compatibility (why this works against us) @@ -116,16 +116,16 @@ workload set is optional; see Phase B3, kept out of the agnostic core.) ### Phase B0 -- Module scaffolding and dependency governance (the hard part) -- [ ] Create `postgresql-client-bench/` with a `pom.xml` inheriting the parent; `packaging=jar`, +- Create `postgresql-client-bench/` with a `pom.xml` inheriting the parent; `packaging=jar`, `finalName=postgresql-client-bench`. -- [ ] Add `postgresql-client-bench` to the parent `` (after `postgresql-client-pgjdbc-compat`, +- Add `postgresql-client-bench` to the parent `` (after `postgresql-client-pgjdbc-compat`, before or after `postgresql-client-coverage`). -- [ ] **Dependencies** (all must pass the enforcer `dependencyConvergence`, +- **Dependencies** (all must pass the enforcer `dependencyConvergence`, `banDuplicatePomDependencyVersions`, `reactorModuleConvergence`): - - [ ] `info.picocli:picocli` -- add a `` property + a + - `info.picocli:picocli` -- add a `` property + a `dependencyManagement` entry in the parent pom. - - [ ] `org.hdrhistogram:HdrHistogram` -- add version property + managed entry. - - [ ] `org.testcontainers:testcontainers` (core only) at **compile/runtime** scope so it + - `org.hdrhistogram:HdrHistogram` -- add version property + managed entry. + - `org.testcontainers:testcontainers` (core only) at **compile/runtime** scope so it shades in for `--pg`. Already aligned by the existing `testcontainers-bom` import in parent `dependencyManagement`. RISK: the project currently uses Testcontainers only at *test* scope; a compile-scope pull drags docker-java into the bench's runtime @@ -133,114 +133,114 @@ workload set is optional; see Phase B3, kept out of the agnostic core.) `-pl` in the loop's normal build; this is a one-off diagnostic). If convergence fails, pin the offending transitive (jackson, docker-java, slf4j) in `dependencyManagement`. - - [ ] Decide the slf4j binding for the bench runtime (docker-java/testcontainers log via + - Decide the slf4j binding for the bench runtime (docker-java/testcontainers log via slf4j). Use `slf4j-simple` (already managed, test-scoped today) at runtime scope in the bench, or `slf4j-nop`, to avoid a "no binding" warning. Keep it out of the published driver artifacts (it is bench-only). -- [ ] **License gate**: add a `postgresql-client-bench` section to `docs/dependencies.md` recording +- **License gate**: add a `postgresql-client-bench` section to `docs/dependencies.md` recording that picocli (Apache-2.0), HdrHistogram (BSD-2-Clause/CC0), Testcontainers + docker-java (MIT/Apache-2.0) are permissive. Note the bench jar is **not a published artifact**, so this is for completeness, not redistribution of the driver. -- [ ] **maven-shade-plugin** in the bench module only: main class +- **maven-shade-plugin** in the bench module only: main class `org.postgresql.client.bench.Main`, `ServicesResourceTransformer` (merge testcontainers/docker-java `META-INF/services`), exclude `module-info.class` and `META-INF/*.SF/DSA/RSA`. Bind to `package`. Confirm **no driver** is ever shaded. -- [ ] **Coverage gate**: the inherited JaCoCo `BUNDLE` check (LINE+BRANCH >= 0.20) applies +- **Coverage gate**: the inherited JaCoCo `BUNDLE` check (LINE+BRANCH >= 0.20) applies to this module too. Plan to clear it honestly with Phase B4 unit tests; if the reflective/CLI-glue/Testcontainers code drags it under 0.20, add a module-local jacoco `` excluding `**/ManagedPostgres*`, `**/cli/**`, `**/Main*` (glue that needs a live server), keeping the pure logic (metrics, durations, reporters, registry, script parser) covered. -- [ ] **Verify the gates**: `mvn spotless:apply` (palantir 4-space reformat of all copied +- **Verify the gates**: `mvn spotless:apply` (palantir 4-space reformat of all copied sources); ASCII sweep (Phase B1); `mvn -o test` stays green with the new module. ### Phase B1 -- Port the driver-agnostic core (repackage `io.bench` -> `org.postgresql.client.bench`) -- [ ] `db/ConnectionFactory.java` -- verbatim logic (URLClassLoader + ServiceLoader driver +- `db/ConnectionFactory.java` -- verbatim logic (URLClassLoader + ServiceLoader driver loading; classpath fallback). -- [ ] `db/PgApi.java` -- verbatim (reflective `PGConnection` / CopyManager / getNotifications). -- [ ] `core/`: `BenchmarkContext`, `Runner`, `WorkerLoop`, `ThreadState`, `RunResult`. -- [ ] `metrics/`: `Allocation` (ThreadMXBean), `LatencyRecorder` (HdrHistogram), `Stats`. -- [ ] `output/`: `Reporter`, `TableReporter`, `JsonReporter`, `CsvReporter` (+ locale-safe +- `db/PgApi.java` -- verbatim (reflective `PGConnection` / CopyManager / getNotifications). +- `core/`: `BenchmarkContext`, `Runner`, `WorkerLoop`, `ThreadState`, `RunResult`. +- `metrics/`: `Allocation` (ThreadMXBean), `LatencyRecorder` (HdrHistogram), `Stats`. +- `output/`: `Reporter`, `TableReporter`, `JsonReporter`, `CsvReporter` (+ locale-safe formatting, JSON escaping -- already hardened upstream in T1.3). -- [ ] `workload/`: `Workload` SPI, `WorkloadRegistry`, `WorkloadSupport`. -- [ ] `db/`: `SchemaManager` (UNLOGGED tables + reset/vacuum), `DataLoader`, `ManagedPostgres`. -- [ ] `cli/`: `BenchCommand` (picocli), `Durations`; `Main`. -- [ ] **ASCII sweep**: replace any non-ASCII in copied Java sources (em-dashes, arrows, +- `workload/`: `Workload` SPI, `WorkloadRegistry`, `WorkloadSupport`. +- `db/`: `SchemaManager` (UNLOGGED tables + reset/vacuum), `DataLoader`, `ManagedPostgres`. +- `cli/`: `BenchCommand` (picocli), `Durations`; `Main`. +- **ASCII sweep**: replace any non-ASCII in copied Java sources (em-dashes, arrows, non-breaking spaces) with ASCII so it matches the project's ASCII-only policy. (Docs may keep prose but the plan/commit rule here is ASCII everywhere -- keep it ASCII.) -- [ ] **Spotless**: `mvn spotless:apply`; fix any palantir reflow that breaks long strings. +- **Spotless**: `mvn spotless:apply`; fix any palantir reflow that breaks long strings. ### Phase B2 -- Port the workloads -- [ ] Generic JDBC (`workload/impl/`): `ConnectWorkload`, `Select1Workload`, +- Generic JDBC (`workload/impl/`): `ConnectWorkload`, `Select1Workload`, `SelectRowsWorkload`, `SelectByIdWorkload`, `PreparedWorkload`, `InsertWorkload`, `UpdateWorkload`, `ReturningWorkload`, `BatchInsertWorkload`, `TypesWorkload`, `StreamWorkload`, `MetadataWorkload`, `TxWorkload`, `FatTxWorkload`, `MixedWorkload`, `ScriptWorkload` (custom `--script` SQL with `:id/:int/:long/:double/:text` params). -- [ ] PG-specific via `PgApi` (work against pg-java-compat + pgjdbc): `CopyWorkload`, +- PG-specific via `PgApi` (work against pg-java-compat + pgjdbc): `CopyWorkload`, `CopyOutWorkload`, `ListenNotifyWorkload`. -- [ ] Register all in `WorkloadRegistry`; keep `--workload all` and comma-lists. -- [ ] Sanity-run against a live PG (Testcontainers or local) for each driver surface; +- Register all in `WorkloadRegistry`; keep `--workload all` and comma-lists. +- Sanity-run against a live PG (Testcontainers or local) for each driver surface; confirm the coverage matrix in section 0 holds (native driver cleanly *skips* the PG-specific ones with a clear "not a PostgreSQL driver connection" error, flagged by the exit-code contract, rather than crashing). ### Phase B3 -- pg-java-specific enhancements (beyond a straight port) -- [ ] **Virtual-thread worker mode** `--virtual-threads` (HIGH VALUE): run workers on +- **Virtual-thread worker mode** `--virtual-threads` (HIGH VALUE): run workers on `Thread.ofVirtual()` and support a high-concurrency scenario (e.g. `--threads 10000`). This is exactly N5.1's "high-concurrency virtual-thread scenario" and pg-java's headline advantage (non-pinning, N1.7). Upstream uses only platform threads. Sub-tasks: thread factory switch in `Runner`; relax/rework the "one connection per thread" model for many-vthread runs (a bounded connection pool shared across vthreads, or accept N connections = N vthreads for the scenario); document the measurement. -- [ ] **`--label `** run tag recorded in the report env. NEEDED because +- **`--label `** run tag recorded in the report env. NEEDED because pg-java-compat returns the pgjdbc-identical driver name "PostgreSQL JDBC Driver" (N4.2), so `driver-name` alone cannot distinguish pg-java-compat from real pgjdbc in a matrix. Let the user stamp `pg-java-native` / `pg-java-compat` / `pgjdbc-42.7.4`. -- [ ] **pg-java classpath helper** `scripts/pg-java-classpath.sh` (or a Maven assembly): - pg-java native needs its full runtime jar set on the bench classpath - (protocol + core + jdbc + scram tree + slf4j-api + a binding). Provide a script that - resolves/echoes that classpath (or builds a shaded `pg-java-all` benchmarking jar) so - `--driver-jar` / `-cp` runs are one command. -- [ ] **Comparison runner** `scripts/run-pgjava-vs-pgjdbc.sh`: builds pg-java, fetches a +- **pg-java classpath helper**: pg-java native needs its full runtime jar set on the + bench classpath (protocol + core + jdbc + scram tree + slf4j-api + a binding). + Shipped inside `scripts/run-pgjava-vs-pgjdbc.sh`, which materializes each module's + runtime classpath with `dependency:build-classpath` and passes it as `--driver-jar`, + rather than as a standalone script. +- **Comparison runner** `scripts/run-pgjava-vs-pgjdbc.sh`: builds pg-java, fetches a pinned pgjdbc, and runs the suite for `pg-java-native`, `pg-java-compat`, and `pgjdbc` against one shared server, emitting one labelled result file per cell plus a combined manifest -- the raw material for the N5 comparison report. -- [ ] **ADR-0015 note in the run docs**: when both `org.postgresql.Driver`s (compat and +- **ADR-0015 note in the run docs**: when both `org.postgresql.Driver`s (compat and real pgjdbc) are present, `DriverManager` arbitration is undefined; always benchmark with `--driver-jar` scoped to a single driver and/or `--driver-class` to disambiguate. -- [ ] (Optional, deferred) **Native-API workload set**: a separate profile/module that adds +- (Optional, deferred) **Native-API workload set**: a separate profile/module that adds `postgresql-client` as a dep and registers native COPY / LISTEN / pull-stream workloads via the `Workload` SPI, to showcase the native surface. Kept OUT of the agnostic core so the default bench has no driver dependency. ### Phase B4 -- Tests, CI, and the committed report -- [ ] Port/extend the pure-logic unit tests (clear the coverage gate honestly): +- Port/extend the pure-logic unit tests (clear the coverage gate honestly): `ReporterLocaleTest` (locale-safe formatting, JSON escaping), plus new tests for `Durations.parseNanos`, `Stats` percentile/throughput math, `WorkloadRegistry.resolve` (names, `all`, unknown), `ScriptWorkload.parse`, and `BenchCommand.validate` argument matrix (mutually-exclusive `--url`/`--pg`, ranges). -- [ ] Optional IT under the `integration-tests` profile: a smoke `--pg 16 --workload +- Optional IT under the `integration-tests` profile: a smoke `--pg 16 --workload select-1,insert,copy --count 50` run asserting exit 0 and non-empty results, so the bench itself is covered by CI when Docker is available. Keep it out of plain `mvn test`. -- [ ] CI wiring (rides N1.6): a manual/scheduled job that runs +- CI wiring (rides N1.6): a manual/scheduled job that runs `run-pgjava-vs-pgjdbc.sh` and uploads the manifest as the N5 benchmark report. -- [ ] Commit an initial reproducible comparison report (README-referenced) to satisfy the +- Commit an initial reproducible comparison report (README-referenced) to satisfy the N5 exit criterion once pg-java is functionally complete enough to run the suite. ### Phase B5 -- Docs and plan integration -- [ ] `postgresql-client-bench/README.md`: build, the three run modes (classpath / `--driver-jar` / +- `postgresql-client-bench/README.md`: build, the three run modes (classpath / `--driver-jar` / `--pg`), the driver-surface matrix, `--label` guidance, and the pg-java-vs-pgjdbc comparison recipe. -- [ ] Tie into the overall plan: reference this module from **N5.1** (macro half + +- Tie into the overall plan: reference this module from **N5.1** (macro half + virtual-thread scenario) and **N5.6** (differential harness), and make it the vehicle for the **N5 exit** "committed reproducible benchmark report". Add a milestone item (e.g. **N5.8 `postgresql-client-bench` module**) or fold explicitly under N5.1. -- [ ] Update `docs/dependencies.md` (done in B0) and note the module in the top-level +- Update `docs/dependencies.md` (done in B0) and note the module in the top-level `README.md` module list (as build-only, not published). ## 3. How we will run it diff --git a/docs/plans/overall.md b/docs/plans/overall.md index 1a9da4ed..d944a83d 100644 --- a/docs/plans/overall.md +++ b/docs/plans/overall.md @@ -75,337 +75,23 @@ full-featured, PostgreSQL-specific driver with a JDBC layer on top. ## Architectural decisions (ADRs) -Full texts live in `docs/adr/`. Summaries below; still-live caveats are noted -inline. - -### ADR-0001: I/O and concurrency model - -**Decision: codecs-only sans-I/O + a pluggable blocking transport, no Netty, -virtual-thread friendly. No async/reactive API.** - -- The **codecs** (framing, message encode/decode) are sans-I/O and pure in - `postgresql-client-protocol`. The **orchestration** is ordinary blocking code in - `postgresql-client` on a `Transport`; it is not a pure state machine, though it - explicitly tracks protocol state. -- **Virtual-thread friendly, not virtual-thread powered.** In normal use the - caller's thread blocks in driver calls. On Java 21+ that thread may be virtual; - the driver must not pin it. Discipline (required throughout): never hold a - `synchronized` monitor across a blocking I/O call; use `ReentrantLock`. Avoid - thread-locals for per-call state on hot paths. `VirtualThreadPinningIT` - asserts non-pinning via JFR. -- **Scaling honesty.** Virtual threads remove client-side *thread* cost; they do - not remove per-connection socket/buffer memory, and they do nothing about - server-side connection limits and per-session cost, which are the real ceiling - on database connection counts. -- **No async/reactive API, no Netty / NIO event loop.** A non-blocking API would - be a second code path for no real benefit here. - -### ADR-0002: Module boundaries and dependency policy - -- `postgresql-client-protocol`: pure wire codecs + framing. No orchestration, no I/O, no - JDBC, zero runtime dependencies. -- `postgresql-client`: transport, orchestration, authentication, type system, - executor, public native API, connection lifecycle (no built-in pool). Runtime - deps limited to SCRAM (ongres) and the SLF4J API. -- `postgresql-client-jdbc`: `java.sql.*` implemented on `postgresql-client`, including the SQL - rewrite/parse layer. -- Optional side modules (not core): JSON adapters, OpenTelemetry/Micrometer - metrics-and-tracing bindings. -- Logging via the SLF4J API only (no bundled binding). The protocol module stays - log-free. -- Caveat (pre-1.0, tracked under N7.3): core `requires transitive` protocol and - the codec SPI exposes `PgWriteBuffer`/`ByteSlice`, so protocol types ride on - the public surface. Decide whether to bless or unweld before the API freeze. - -### ADR-0003: Testing strategy - -- Unit tests (JUnit 5) in each module; protocol/orchestration tests use in-memory - transports and golden fixtures. The fixtures are currently **spec-derived** - (encoder-independent, hand-built from the message-format spec); a - capture-based libpq/Wireshark corpus remains aspirational (N7.3). -- A **programmable mock PostgreSQL server** (test-only) drives auth edge cases, - protocol-version negotiation, protocol violations, and server-closes-mid-query. -- Integration tests use Testcontainers + a PostgreSQL image via the Failsafe - plugin in a dedicated profile (a normal `mvn test` does not require Docker). -- **Negative TLS matrix** is first-class (coverage gaps tracked in N1.18). -- **Differential testing** vs pgjdbc runs through the `compat-suites/` harnesses - (pgjdbc's own suite, Hibernate's suite; see N4.7). -- A **PostgreSQL version matrix** in CI: 18/17/16/15/14 plus a PgBouncer - transaction-pooling job (N1.6). -- Decoders get fuzz/property tests; a load test asserts no virtual-thread pinning. - -### ADR-0004: Compatibility contract (versions, protocol, defaults) - -- **Supported servers:** the **maintained matrix** -- exercised on every CI build - -- is majors 18/17/16/15/14, plus optional dev/beta. Beyond that the driver is - **capability-gated down to PostgreSQL 9.1**: version-dependent behavior checks - a capability set and the IT harnesses skip features an older server lacks via - `assumeServerAtLeast`, so the suite runs green against any 9.1-18 image driven - through `scripts/run-integration-matrix.sh` (verified on demand; not in the - default CI matrix). 9.0 and 8.4 are deferred -- no official Docker images ship - for them. -- **Protocol version policy (default 3.0):** `protocolVersion=3.0` is the - default (libpq parity; some middleware rejects unknown minors rather than - negotiate down). `latest` opts into requesting 3.2 with - `NegotiateProtocolVersion` fallback; `3.2` targets direct PG18+. The decoder - always supports variable-length `BackendKeyData` (4..256 bytes in 3.2; the - lower bound is enforced at decode, the upper bound deliberately left open so - opaque middleware keys round-trip). Cancellation sends the key form matching - the **negotiated** protocol; unknown keys are preserved as raw bytes. -- **Compatibility levels are explicit choices, not accidents:** where libpq and - pgjdbc differ (notably the meaning of `ssl=true`, ADR-0009), we pick the safer - behavior and document the divergence. -- **Capability gating.** Version-dependent features (SCRAM PG10+, multirange and - numeric `Infinity` PG14+, `MERGE` tag PG15+, `sslnegotiation=direct` PG17+, - protocol 3.2 PG18+) check a capability set derived from `server_version` and - `ParameterStatus`. Built-in type OIDs are global; user-defined type OIDs are - **per-database** and never cached globally. -- **Out-of-scope auth:** GSSAPI/Kerberos, SSPI, and OAUTHBEARER. The protocol - still decodes their request messages so an unsupported mechanism fails with a - clear "unsupported authentication mechanism" error, not a protocol violation. - -### ADR-0005: Pull-first streaming result model - -**Decision: the lowest-level query primitive is a pull-based cursor; callbacks -and aggregates are adapters on top.** - -``` -PgResultStream rs = connection.execute(sql, params); -while (rs.next()) { - Row row = rs.currentRow(); // transient: valid until the next next() -} -rs.close(); // or auto-closed when fully drained -``` - -- `next()` advances by reading exactly enough off the wire to produce the next - row; the full result set is never buffered in core. The `Row` is transient - (may reuse backing buffers) and is valid only until the following `next()`. -- **Connection-busy contract:** while a result stream is open the connection is - busy; the application must drain or `close()` it before issuing the next - query. `close()` drains/discards remaining rows and restores the connection - to a clean, reusable state. Note: a mid-stream server error surfaces on - `next()` when the consumer is still reading; a trailing error captured while - `close()` drains is thrown by `close()` itself after the connection is - released -- errors are never silently lost (analysis-004 F1). -- **Adapters built on the cursor:** `forEach(RowConsumer)`, `map`, `collect`, - exactly-one / at-most-one, `count`. -- **JDBC materialization (2026-07-09/10 amendment).** The JDBC layer's default - result set (fetchSize=0, forward-only) **materializes** the whole result at - execute time as raw row bytes with per-getter lazy decode, matching pgjdbc's - default behavior (interleaved statements on one connection, results survive - the statement). `fetchSize > 0` outside autocommit streams through a portal, - matching pgjdbc's cursor-mode rule; transaction boundaries close open - streaming cursors. Recorded in ADR-0005's Amendments section (N1.20, 2026-07-16). -- **Scope boundary (large rows vs large result sets).** A single enormous value - is still materialized by its codec; the message cap (ADR-0008) bounds it. - LOB-style single-value streaming is an open question (N8.3). - -### ADR-0006: Public native API design - -- Design the surface as a deliberate unit: the connection/client type, - configuration entry point and builder, the pull `PgResultStream`/`Row`, result - summary, and the exception tree, defined as interfaces first. -- Names read naturally for PostgreSQL users and do not mirror JDBC. Blocking - signatures; no async anticipation (ADR-0001). -- Caveat (pre-1.0, tracked under N7.3): the concrete `CoreConnection` class has - become the de facto public API (the `PgConnection` interface is a small - vestige). Promote a real interface or bless the class before the API freeze. - -### ADR-0007: Exception and warning model - -- **Unchecked is a deliberate product choice**: `PgException` (unchecked) root - carrying full PostgreSQL error fields; subtypes derived from the SQLSTATE - class (connection, integrity-constraint, syntax, transaction-rollback, - insufficient-resources, etc.) so callers branch without string matching. - Connection/config/TLS/auth failures get dedicated subtypes. -- Notices and `SQLWarning`-equivalents are a separate non-fatal channel delivered - via the event SPI; they never throw. -- The JDBC layer maps `PgException` to the appropriate `SQLException` subtype; - a pluggable factory seam lets the compat layer substitute `PSQLException` - (installed per-connect, not JVM-global). - -### ADR-0008: Buffer and memory management - -- Per-connection read/write buffers are heap `byte[]`/`ByteBuffer` (not direct); - sizing and growth are bounded and configurable, and both buffers shrink on - idle after a large message. -- **Two-tier bounding:** the **message-length cap is the pre-read allocation - guard** -- the length prefix is checked before any buffer grows (default - 1 GiB; `UNLIMITED` is an explicit opt-in). The **optional per-value cap is a - fail-fast policy check** applied as each column is exposed; values are - zero-copy slices of the already-capped frame, so no second pre-read guard is - needed. Per-value cap defaults to unlimited. - -### ADR-0009: TLS and transport security - -- **`sslmode`** spectrum: disable, allow, prefer, require, verify-ca, - verify-full (with the prefer/allow reconnect dance). -- **`ssl=true` semantics:** validate certificate AND hostname (pgjdbc behavior), - not libpq's non-validating default for that spelling. -- **Direct TLS:** `sslnegotiation=direct` (PG17+), permitted only with - `sslmode=require` or stronger. -- **Trust material:** system trust store or local CA file; client cert/key for - mutual TLS; CRL support; SNI on by default; configurable TLS min/max version; - a hook for a custom `SSLContext`/`SSLSocketFactory`. -- **Channel binding:** `channelBinding` disable/prefer/require; `require` - refuses non-PLUS SCRAM (downgrade protection) and requires TLS, using - `tls-server-end-point`. -- **`require_auth`-style policy:** require or reject specific authentication - methods (e.g. forbid cleartext/MD5). -- **Shipped default posture (decided; 2026-07-28 amendment):** - `sslmode=prefer` with a non-validating trust manager and silent plaintext - fallback; `require` encrypts but does NOT authenticate the server. WARNs fire - when a session ends up unauthenticated. The credential is gated instead of - the transport: cleartext/MD5 auth over an unencrypted channel is refused - unless `allowUnencryptedPasswordAuth=true` (the compat layer sets it to keep - pgjdbc's posture); an opt-in strict `secure` profile is N2.13. - -### ADR-0010: Secret-handling boundary - -- Core APIs accept clearable secrets (`char[]`/`byte[]`/`Supplier`), zero - driver-owned copies after use, and scrub outbound auth buffers after flush. -- The JDBC boundary, where `String` passwords arrive via `Properties`/URL, - converts to a clearable form as early as possible; password-bearing URLs are - redacted in logs and `toString`. -- We make no promise that the Java heap never transiently holds a password; we - minimize lifetime and exposure. - -### ADR-0011: Type-system strategy - -- **Force safe session defaults by default:** `client_encoding=UTF8`, - `DateStyle=ISO`, `IntervalStyle=iso_8601` (opt-out available); verify via - `ParameterStatus`, re-assert with a post-auth `SET` if needed; refuse - `standard_conforming_strings=off` at connect (the SQL lexer assumes standard - strings). -- **Conservative binary policy:** binary format only for stable built-in types - (in practice this now covers most built-in scalars, numeric, and arrays of - binary-capable elements); text fallback for complex/user-defined types. - timetz/interval/timestamptz deliberately stay text. -- **typmod handling**; **unknown parameter typing** (untyped nulls, `setString` - policy); domain/enum/composite arrays. -- **Type-cache invalidation:** same-connection DROP/ALTER TYPE/DOMAIN and any - DISCARD clear the cache; `reset()` and `CoreConnection.invalidateTypes()` - cover DDL the driver cannot observe (another connection altering a type in - place). Caveat: still open is fail-loudly charset decoding (N1.19) -- - undecodable server bytes currently become U+FFFD. - -### ADR-0012: Prepared statement and query-mode policy - -- **Query mode** knob (preferQueryMode-style): simple, extended, - extended-for-prepared-only, or unnamed-only. SIMPLE/UNNAMED_ONLY deliberately - disable the statement cache for pooler compatibility. -- **prepareThreshold:** promote to a named server statement after N executions - (default 5, configurable, 0 disables); unnamed-statement fast path. -- **Cache:** per-connection LRU keyed by **SQL + search_path + declared - parameter-type OIDs**; the same OID vector is declared in the promoted - `Parse`, so typing is identical below and above the threshold. The budget is - an **entry-count LRU** (`preparedStatementCacheQueries`, default 256), a - deliberate simplification of the ADR's byte-budget wording. -- **Failure semantics:** transparent re-prepare on 0A000 (skipped inside an - already-failed transaction so the original error is not masked); app-issued - `DEALLOCATE ALL` is detected and invalidates the cache; a busy-time - deallocation is queued and flushed at the next idle boundary. - -### ADR-0013: Pooler and proxy compatibility - -- Real deployments include PgBouncer (transaction pooling), RDS-Proxy-style - systems, and protocol-aware middleware; these break assumptions about - server-side session state. -- A **pooler-compatible mode** exists (unnamed-only / simple-query modes, - disable the named-statement cache); PgBouncer transaction-pooling ITs - exercise it. Note: pooled logical handles do not reset session state between - handles (pgjdbc-parity contract; documented, not "fixed"). - -### ADR-0014: Batch execution via array-parameter rewrite (Accepted, landed) - -- Collapse a qualifying INSERT batch into **one** execution whose parameters are - PostgreSQL arrays (multi-arg `unnest`), opt-in via `rewriteBatchUsingArrays`, - gated on server support (9.4+). Non-qualifying batches fall back unchanged. - -### ADR-0015: JDBC URL scheme ownership and driver arbitration (Accepted, landed) - -- The native driver owns the canonical `jdbc:pg:` scheme and claims the legacy - `jdbc:postgresql:` scheme only via per-connect `claimPostgresqlScheme` - arbitration (`auto` yields to a registered `org.postgresql.Driver` with a - one-shot WARN; `never`/`always` override). `PgDriver` gained - `register()`/`deregister()` and an auto-register opt-out; the compat shim - fails fast when a real pgjdbc is co-present. - -### ADR-0016: Batch execution via multi-row VALUES rewrite (Accepted, landed) - -- A pgjdbc-`reWriteBatchedInserts`-style rewrite that folds a batched INSERT - into multi-row `VALUES` lists (complementing ADR-0014's array rewrite; the - compat driver maps pgjdbc's property name onto it). See - `docs/adr/ADR-0016-batch-multi-values-rewrite.md`. - -### ADR-0017: JSON value type and binding (Accepted, NOT yet implemented) - -- A dependency-free `PgJson` marker type + text-only codec so json/jsonb values - are distinguishable from `varchar` and bind with the correct OID without a - SQL cast; opt-in, defaults unchanged; `Row.getJson(col)` convenience; a - Jackson/Gson binding stays out-of-core. Implementation tracked under N6.6. - -### ADR-0018: Composite type mapping to Java records (Accepted, NOT yet implemented) - -- Opt-in mapping of composites to Java `record`s and enums to Java `enum`s - (`registerComposite("schema.type", MyRecord.class)`), layered on the existing - type cache and per-field codecs; defaults unchanged. Implementation tracked - under N6.3. - -### ADR-0019: General pipelining API (Accepted, landed) - -- `PgPipeline`: queue extended-protocol statements without waiting, explicit - `sync()` error boundaries, results read strictly in queue order; blocking and - virtual-thread friendly, no futures. Results are buffered per handle (not - streamed) to avoid the pipelining write-write deadlock; fetch-size/portal - suspension, COPY, and multi-statement SQL are out of scope in this version. - -### ADR-0020: Core public API surface and stability tiers (Accepted, landed) - -- "Exported" and "supported" became different things: an explicit, tiered - (Stable/Experimental/Internal) allowlist in `docs/api-surface.md`, enforced by - JPMS internal packages, qualified exports, and `ApiSurfaceManifestTest`. - Behavioral surface is interface-backed, implementations package-private. - Landed as the pre-1.0 API lockdown (N7.3); the binary-compatibility gate the - ADR also asks for waits on a first release to diff against (N7.1). See the - ADR's Implementation status for the two corrections building it produced. - -### ADR-0021: Single-Sync execution of the multi-row VALUES batch chunks (Accepted, landed) - -- The compat multi-values rewrite buffers a whole row-free batch and flushes it - at one `Sync`, resolving each chunk while idle but encoding values - write-through into the outbound buffer. Strictly all-or-nothing; recovered the - allocation the materialized version cost (run 013). - -### ADR-0022: Package root, module names, and Maven coordinates (Accepted, landed) - -- Everything ships under `org.postgresql.client`, one segment per module, with - groupId `org.postgresql` and `postgresql-client*` artifacts. `org.postgresql.core` - and `.jdbc` are unavailable because real pgjdbc exports them and a split package - is a JPMS resolution error regardless of namespace ownership. Core sits under - `.core`; promoting the public API to the root is deferred to N7.3. +`docs/adr/` is the canonical record; [`docs/adr/README.md`](../adr/README.md) is +the index, with each decision's status and whether the code implements it yet. +This plan does not restate them -- the summaries that used to sit here drifted +from the ADRs they summarized. Items below cite an ADR by number where they +implement or constrain one. ## Milestones -- **M0 Protocol skeleton:** frame codec, core messages, fuzzing. (Phases 1-3) -- **M1 Earliest end-to-end:** trust auth + simple `SELECT 1` + text row via the - pull stream, over plaintext. (through Phase 6) -- **M2 Secure connect:** TLS, SCRAM, channel binding, the auth matrix. (Phases - 7-8) -- **M3 Usable core:** common codecs, the full pull-stream API, transactions, - cancellation. (Phases 9, 10, 13, 14) -- **M4 Parameterized queries:** extended protocol, parameter typing, portals, - fetch size, prepared statements + cache. (Phases 11-12) -- **M5 JDBC walking skeleton:** Driver, Connection, Statement, the SQL rewrite - layer, PreparedStatement, forward-only ResultSet. (early Phase 23) -- **M6 Production JDBC:** timeouts, metadata basics, generated keys, batching, - pooler compatibility. (Phases 15-16, mid Phase 23) -- **M7 Advanced PostgreSQL:** COPY, LISTEN/NOTIFY, arrays/ranges/json, large - objects. (Phases 17-21, late Phase 23) -- **M8 Release hardening:** benchmarks, docs, CI matrix, reproducible releases. - (Phase 24) - -The milestones above (M0-M8) describe the build-out to a usable, hardened driver. +M0-M8 sequenced the build-out to a usable, hardened driver: protocol skeleton, +first end-to-end query, secure connect, usable core, parameterized queries, the +JDBC walking skeleton, production JDBC, advanced PostgreSQL features, release +hardening. All of it is behind us, along with the "first useful JDBC release" +cut line those milestones aimed at -- the driver ships COPY, LISTEN/NOTIFY, +large objects, multi-host, XA, scrollable/updatable result sets, PgBouncer +modes and refcursor support on top of that minimum. The phase items below keep +the detail; git history keeps the sequencing. + The path from there to **best-in-class** (faster, smarter, easier, cleaner than pgjdbc) is sequenced as N1-N8, detailed in "Post-MVP milestones" below: @@ -424,40 +110,13 @@ pgjdbc) is sequenced as N1-N8, detailed in "Post-MVP milestones" below: - **N7 1.0 release:** reproducible signed Maven Central artifacts + complete docs. - **N8 Advanced/optional (post-1.0):** replication, advanced JDBC, LOB streaming. -## First useful JDBC release (the MVP cut line) - -The ruthless minimum for a first artifact people can actually use: - -- connect over TLS + SCRAM (and trust/cleartext/MD5) -- simple + extended query protocols -- the pull result stream -- common scalar + temporal codecs -- transactions (autocommit + explicit) -- cancellation and timeouts -- prepared statements with prepareThreshold + cache -- JDBC `Driver`, `Connection`, `Statement`, `PreparedStatement`, the SQL rewrite - layer, forward-only `ResultSet`, basic `DatabaseMetaData`, generated keys -- a simple `DataSource` -- batching -- protocol 3.0 default (3.2 opt-in) - -Status (2026-07-15): every bullet is implemented, and the driver is well past -the cut line (COPY, LISTEN/NOTIFY, large objects, multi-host, XA, -scrollable/updatable result sets, PgBouncer modes, refcursor support). The two -verification gaps this section used to carry -- no functional `DataSource` -test, and the hand-written `DatabaseMetaData` catalog queries never run -against a server -- were closed by N1.18 (2026-07-13). - --- -## Current state and priority queue (2026-07-25) +## Current state and priority queue -Snapshot as of ~916 commits (status figures refreshed 2026-07-15 per -finding-2026-07-15 F7). The eleventh-round audit (2026-07-10) was followed the -same day by a remediation wave of ~28 commits that closed nearly all of its -findings; since then N1.18/N1.21/N1.22 landed along with benchmark runs -007-011 and several pgjdbc-baseline refreshes. Where this section and an older -phase annotation disagree, this section is newer. +Refreshed as work lands rather than dated once; each figure below carries the +date it was measured. Where this section and an older phase annotation +disagree, this section is newer. **Where we are:** @@ -531,25 +190,17 @@ phase annotation disagree, this section is newer. integration counts were identical across the move (1192 and 652), and both compat suites were re-run against it. -**Priority queue (work top to bottom):** +**Priority queue (work top to bottom; closed entries drop off, the N-item +checkboxes below are the record):** -1. **P0 -- N1.22: re-baseline.** DONE (benchmark runs 007-011 committed, - baseline refreshed repeatedly, plan figures updated 2026-07-15). -2. **P1 -- correctness residuals.** DONE: N1.19 (fail-loudly charset decoding) - and the N1.21 low-severity sweep both landed. -3. **P1 -- N1.18: verify what we ship.** DONE 2026-07-13 (DatabaseMetaData - catalog ITs, DataSource functional test, targetServerType selection tests, - XA/pooled DataSource tests, the TLS negative matrix incl. client-cert). -4. **P2 -- pgjdbc drop-in (N4).** N4.7 cluster burn-down (batch failure +1. **P2 -- pgjdbc drop-in (N4).** N4.7 cluster burn-down (batch failure semantics next, then quoting, dates, metadata, PGobject); N4.3 remainder (addDataType, getObject->PGobject); N1.20 doc/metadata honesty. -5. **P2 -- JDBC breadth residuals (N3).** DONE 2026-07-28 (N3.9 CALL + INOUT, - N3.10 generated-keys-by-index, N3.11 secondary metadata methods). -6. **P3 -- performance (N5).** JMH micro-harness (N5.1), write-side allocation - (N5.7), batch statement reuse (N5.9), then quantification (N5.5) and the - differential harness (N5.6). -7. **P3 -- pre-1.0 API lockdown (N7.3).** -8. **P4 -- N6 observability/resilience/ergonomics, then N7 release +2. **P3 -- performance (N5).** Write-side allocation residuals (N5.7), the rest + of the measurement item (N5.1), then the differential harness (N5.6); N5.5 + (binary quantification) and N5.9 (batch statement reuse) are closed. +3. **P3 -- pre-1.0 API lockdown (N7.3).** +4. **P4 -- N6 observability/resilience/ergonomics, then N7 release engineering, then N8.** --- @@ -855,7 +506,8 @@ phase annotation disagree, this section is newer. - [x] **C24.4** Coverage gates (JaCoCo per-module check, 0.20 floor -- a regression tripwire, not an assurance gate) plus `-Xlint:all` under `-Werror` and NullAway at ERROR over the protocol and core packages; the jdbc/compat - modules remain, see `docs/follow-up.md` N1.9. (no tests) + modules remain (`docs/follow-up.md` N1.9), as do the tools in N1.23-N1.25. + (no tests) - [ ] **C24.5** Expanded fuzzing. (unit) - [ ] **C24.6** Javadoc, user guide, docs site (= N7.2). (no tests) - [x] **C24.7** PostgreSQL version matrix + PgBouncer job in CI (= N1.6). (it) @@ -908,8 +560,9 @@ Convert every claim into something fixed or verified by automation. - [x] **N1.7** Virtual-thread non-pinning load test (C24.2). - [x] **N1.8** ASCII-only policy gated by `AsciiSourcePolicyTest`. - [x] **N1.9** JaCoCo coverage gate, `-Xlint:all` under `-Werror`, and NullAway - at ERROR over `org.postgresql.client.protocol` and `org.postgresql.client.core` - (jdbc/compat and the remaining tools deferred: `docs/follow-up.md` N1.9). + at ERROR over `org.postgresql.client.protocol` and `org.postgresql.client.core`. + The residuals are broken out: NullAway over jdbc/compat and the coverage floor + in `docs/follow-up.md` N1.9, the remaining tools as N1.23-N1.25. - [x] **N1.10** README/status reconciliation. - [x] **N1.11** Multi-statement trailing error no longer swallowed (`drainToReadyForQuery` captures and rethrows). @@ -1002,6 +655,25 @@ Convert every claim into something fixed or verified by automation. pgjdbc-suite baseline refreshed repeatedly (gated baseline 6198/841/54 as of 2026-07-15), and the figures quoted in this plan updated (2026-07-15, finding-2026-07-15 F7). +- [ ] **N1.23** forbidden-apis over the shipped modules: ban default-locale and + default-charset methods (`String.format` without a `Locale`, `new String(byte[])` + without a charset) plus `System.out`/`System.err`. This is the bug class + `ReporterLocaleTest` exists because of. Cheapest of the remaining tools + (`docs/static-analysis.md`). (unit) +- [ ] **N1.24** Error Prone's own checks at ERROR severity, replacing + `-XepDisableAllChecks`. The default ERROR tier only (`ReferenceEquality`, + `MissingOverride`, `EqualsHashCode`, format-string checks); the WARNING tier + stays off deliberately. Expect it to need `--add-exports` flags and to be the + thing that breaks on a JDK bump. (no tests) +- [ ] **N1.25** SpotBugs + FindSecBugs, scoped to `postgresql-client-protocol` and + the codec packages, CI-only, with `EI_EXPOSE_REP*` excluded wholesale. The value + is the bounds arithmetic on attacker-controlled lengths in `FrameReader` and + `PgReadBuffer`; the cost is a separate verify-phase bytecode pass, hence the + scoping. (no tests) + +Extending NullAway to the jdbc and compat modules is the fourth residual and is +not one commit: see `docs/follow-up.md` N1.9, which records why a scripted pass +does not converge. Raising the JaCoCo floor is tracked there too. Exit: green CI on the full PG matrix + PgBouncer (done, N1.6); non-pinning test passing; zero known High/Medium correctness findings; the shipped-but-untested @@ -1017,8 +689,7 @@ surfaces closed (N1.18); coverage and ASCII gates enforced. - [x] **N2.4** Secret zeroing finished (`Secret.toUtf8Bytes` intermediates, PKCS#8 passphrase copies); `byte[]`-record identity semantics documented. - [x] **N2.5** Negative-TLS + `require_auth` refusal matrix end-to-end - (mutual-TLS end-to-end deferred: `docs/follow-up.md` N2.5 section, also - N1.18). + (mutual-TLS end-to-end closed by N1.18). - [x] **N2.6** Encoder/decoder hardening: 16-bit count validation; strict trailing-byte policy for fixed-shape messages, lenient for variable-shape. - [x] **N2.7** Malicious-server matrix (framing/size-limit/downgrade unit @@ -1377,6 +1048,10 @@ in CI; at least two "pgjdbc can't do this cleanly" capabilities documented. - [ ] **N7.1** Reproducible builds, signing, Maven Central (C24.8); semver + changelog. The reproducible `project.build.outputTimestamp` is set; GPG signing, Central deployment wiring, and the semver/changelog policy remain. + Wire the revapi (or japicmp) binary-compatibility gate as part of this, not + after it: it diffs against the last released artifact, so the first release is + the moment it becomes possible, and it is what makes the Stable tier mean + anything across versions (ADR-0020 section 4, `docs/static-analysis.md`). - [ ] **N7.2** Javadoc, user guide, docs site (C24.6); migration-from-pgjdbc guide. - [ ] **N7.3** Pre-1.0 API lockdown; finalize `module-info` surfaces; expanded @@ -1384,7 +1059,8 @@ in CI; at least two "pgjdbc can't do this cleanly" capabilities documented. is enumerated and enforced, the behavioral types are interfaces, the protocol-type seam is decided and pinned, and the transient-row contract has a runtime check. What remains under this item is expanded fuzzing (C24.5) and the - binary-compatibility gate, which needs a first published artifact (N7.1). + revapi/japicmp binary-compatibility gate, which needs a first published artifact + and is tracked on N7.1. - [x] Promote a real interface over `CoreConnection` (or bless the class and split it internally) -- the concrete class is the de facto public API. Done 2026-07-26 (ADR-0020): `PgConnection` carries the native API, `PgPreparedStatement` @@ -1451,18 +1127,15 @@ and a stable public API. ## Cross-cutting concerns (apply throughout) +The guiding principles above are not repeated here. These are the constraints +every item has to hold up under that they do not cover: + - **Encoding:** force UTF8 by default; honor runtime `client_encoding` changes; - map to a Java `Charset` (fail-loudly work tracked in N1.19). + map to a Java `Charset` (ADR-0011). - **Async messages are central:** the executor accepts `NoticeResponse`/ `ParameterStatus`/`NotificationResponse` at any valid protocol point. -- **Secret handling (ADR-0010):** clearable at our boundaries; redact URLs; - never log secrets; no heap guarantee. -- **Untrusted server input:** pre-read message cap (default 1 GiB, ADR-0008); - decoders never crash/over-allocate. - **Thread-safety:** a connection is single-threaded per logical use; no built-in pool. `ReentrantLock`, never `synchronized` across I/O. -- **Streaming, pull-first (ADR-0005):** core never buffers a whole result set; - the JDBC default materializes by deliberate choice. - **Capability gating (ADR-0004):** version-dependent features check capabilities. - **Pooler awareness (ADR-0013):** features that rely on session state document @@ -1593,16 +1266,18 @@ still actionable is a numbered item above. The durable non-task knowledge: ## References (decisions lean on these) -- PostgreSQL protocol: overview, message flow, and message formats (current - docs, v18 / protocol 3.2, incl. variable-length cancel keys). -- PostgreSQL SASL authentication (SCRAM, channel binding, OAUTHBEARER). -- PostgreSQL libpq connection docs (`sslmode`, `sslnegotiation=direct`, - `require_auth`, multi-host/Unix-socket URIs, `.pgpass`, service files). -- pgJDBC docs (connection properties, `ssl=true` semantics, prepared-statement - cache and query-mode knobs). -- Oracle Java 21 virtual threads (pinning and `ReentrantLock` guidance). - -## Suggested commit message convention - -`: ` where area is one of `protocol`, `core`, `jdbc`, -`build`, `docs`, `test`. Plaintext ASCII, no AI attribution trailers. +- PostgreSQL protocol: [overview](https://www.postgresql.org/docs/current/protocol-overview.html), + [message flow](https://www.postgresql.org/docs/current/protocol-flow.html), + [message formats](https://www.postgresql.org/docs/current/protocol-message-formats.html) + (current docs, v18 / protocol 3.2, incl. variable-length cancel keys). +- [PostgreSQL SASL authentication](https://www.postgresql.org/docs/current/sasl-authentication.html) + (SCRAM, channel binding, OAUTHBEARER). +- [libpq connection parameters](https://www.postgresql.org/docs/current/libpq-connect.html) + (`sslmode`, `sslnegotiation=direct`, `require_auth`, multi-host/Unix-socket + URIs, `.pgpass`, service files). +- [pgJDBC connection properties](https://jdbc.postgresql.org/documentation/use/) + (`ssl=true` semantics, prepared-statement cache and query-mode knobs). +- [JEP 444: Virtual Threads](https://openjdk.org/jeps/444) (pinning and + `ReentrantLock` guidance). + +Commit conventions live in [`AGENTS.md`](../../AGENTS.md), which is the contract. diff --git a/docs/plans/performance.md b/docs/plans/performance.md index eab6f2e5..68f67e82 100644 --- a/docs/plans/performance.md +++ b/docs/plans/performance.md @@ -3,9 +3,7 @@ Status: **largely executed, and superseded as a tracker.** The pipelining, write-through encode, and binary-decode work landed under N5.x; the numbers that justified each step are in `docs/benchmarks/`. Kept as the design record for why each item was chosen and -what it was measured against. Its checkboxes were not maintained as the work landed -- -[`overall.md`](overall.md) is the live list -- so an unticked box here means "see -overall.md", not "not done". +what it was measured against; [`overall.md`](overall.md) is the live list. Turns the benchmark findings (`docs/benchmarks/pgjava-vs-pgjdbc-00{1,2}.md`) into a concrete, @@ -45,15 +43,15 @@ milestones N5.1 (measure), N5.2 (pipelining), N5.5 (binary), N5.7 (allocation). Do this before optimizing -- without it we cannot attribute a 3% macro delta, and bytes/op gives an immediate deterministic score for the allocation work. -- [ ] JMH micro-benchmark module (or profile) with benches for the hottest per-call paths: +- JMH micro-benchmark module (or profile) with benches for the hottest per-call paths: decode int/long/text/numeric/timestamp per value; parameter encode/bind per param; the Parse/Bind/Execute exchange against a loopback or mock server; row materialization. -- [ ] async-profiler runbook (alloc + cpu) checked into `docs/benchmarks/` so anyone can +- async-profiler runbook (alloc + cpu) checked into `docs/benchmarks/` so anyone can reproduce a flame graph for a workload. -- [ ] A tracked results file that records bytes/op and ops/sec per workload per change, so the +- A tracked results file that records bytes/op and ops/sec per workload per change, so the trend is visible (the macro `postgresql-client-bench` already emits both; wire it into a committed report per milestone -- run 003, 004, ...). -- [ ] (Nice) an EpsilonGC / fixed-heap bench mode to isolate allocation-driven differences and +- (Nice) an EpsilonGC / fixed-heap bench mode to isolate allocation-driven differences and report GC time. ## 3. Pillar A -- Pipelining (N5.2): the batch gap @@ -98,14 +96,14 @@ declined INSERT still pays N round-trips. ### Sub-steps (each: commit + mock/IT test + bench) -- [ ] A1. Core: a send that buffers without flushing (or a multi-message batch send), so many +- A1. Core: a send that buffers without flushing (or a multi-message batch send), so many Bind/Execute can be queued before one flush. (`WireChannel.send` flushes per call today.) -- [ ] A2. Core: `executeBatchPipelined` with a small fixed window (correctness first); mock-server +- A2. Core: `executeBatchPipelined` with a small fixed window (correctness first); mock-server test pinning the message sequence and the error boundary. -- [ ] A3. Core: byte-budget window sizing; a deadlock stress IT (large batch x wide rows). -- [ ] A4. JDBC: wire `executeBatch`; preserve `BatchUpdateException`, generated keys, and the +- A3. Core: byte-budget window sizing; a deadlock stress IT (large batch x wide rows). +- A4. JDBC: wire `executeBatch`; preserve `BatchUpdateException`, generated keys, and the array-rewrite path. -- [ ] A5. Bench: batch-insert (no rewrite), batch-update, batch-delete -> confirm ~10x; commit +- A5. Bench: batch-insert (no rewrite), batch-update, batch-delete -> confirm ~10x; commit the report; tick N5.2. ### Risks @@ -125,38 +123,38 @@ landed without being ticked, and B1 and B4 are partly done. ### Quick wins (low risk, broad, deterministic) -- do first -- [x] B5. DONE. `PgPreparedStatement.boundParameters()` used to allocate `new ArrayList<>(Arrays.asList( +- B5. DONE. `PgPreparedStatement.boundParameters()` used to allocate `new ArrayList<>(Arrays.asList( parameters))` per execute -- two objects just to hand params to core, which re-reads them into arrays. Pass the `Object[]` (or a `List` view). -- [x] B6. DONE via a `primaryOid()` accessor. Was `AbstractCodec.oids().clone()` per bound +- B6. DONE via a `primaryOid()` accessor. Was `AbstractCodec.oids().clone()` per bound parameter, called from `ParameterEncoder.encodeWithCodec` -- a fresh `int[]` clone just to read `oids[0]`. Add a `firstOid()` accessor or cache the clone. (This is P10.) -- [ ] B4. PARTIAL: the common fixed tags are interned, but `tag.split(" ")` survives for +- B4. PARTIAL: the common fixed tags are interned, but `tag.split(" ")` survives for count-bearing tags (`CommandComplete.java:118`). Was: tag parsed ~4x per write via `SimpleResultSummary.of` (`SimpleResultSummary.java:10-13` -> `CommandComplete.java:47` `tag.split(" ")` + boxing). Parse the tag once. Inflates every single-row INSERT/UPDATE/DELETE. (This is P11.) -- [ ] B3. Per-execute `int[]` rebuilds that are cacheable on `CorePreparedStatement` (metadata +- B3. Per-execute `int[]` rebuilds that are cacheable on `CorePreparedStatement` (metadata is fixed once described): `columnTypeOids` (`CoreConnection.java:419`), the result-format array `ResultFormatPolicy.forColumns` (`ResultFormatPolicy.java:38`), and memoize the `cacheKey` StringBuilder+String rebuilt every execute (`CoreConnection.java:290`, P9). -- [ ] B2. Numeric boxing: `SimpleRow.getInt/getLong` return `Integer`/`Long` +- B2. Numeric boxing: `SimpleRow.getInt/getLong` return `Integer`/`Long` (`SimpleRow.java:110,127`) and `PgResultSet` immediately unboxes. Add a primitive core accessor (`getIntOr(col, sentinel)` + `wasNull`, or an int-returning path) -- removes one box per numeric column on the select-by-id hot path. ### Structural (bigger, do after the harness + quick wins) -- [ ] B1. PARTIAL: `FrameReader` now holds a `reusableFrame`, but `new Frame()` remains on two +- B1. PARTIAL: `FrameReader` now holds a `reusableFrame`, but `new Frame()` remains on two paths (`FrameReader.java:151,157`). Was: each `DataRow` costs ~5 allocations -- `new Frame` (`FrameReader.java:184`), `new PgReadBuffer` (`Frame.java:43`), two `int[]` (offsets + lengths) and the `DataRow` record (`DataRow.java:25-26,40`). `SimpleRow` and the framing buffer are already reused per connection; extend the same reuse to a mutable frame carrier + a reusable read cursor + stream-owned offset/length arrays refilled per row. Touches every row of every result set -- the largest structural win. -- [ ] B7. `getObject` text path allocates ByteSlice + String + `trim()` String + box per value +- B7. `getObject` text path allocates ByteSlice + String + `trim()` String + box per value (`SimpleRow.java:147-153`, `AbstractCodec.java:64`, `ScalarCodecs.java:38`). Parse straight from bytes where the codec allows; largely subsumed by Pillar C (binary decode). -- [x] B8. DONE. Temporal getters no longer capture a `() -> row.getString(col)` lambda per call; +- B8. DONE. Temporal getters no longer capture a `() -> row.getString(col)` lambda per call; they use a non-capturing path, as the int/string getters already did. ### Method @@ -171,10 +169,10 @@ Numeric and arrays still decode via text (String + parse), which is both CPU (pa allocations per value through `getObject` (Pillar B7). Stable scalars and fixed-width temporals already decode binary and are requested binary on the named path. -- [ ] C1. Numeric binary decode (base-10000 digit groups) -> removes the String+trim+parse chain +- C1. Numeric binary decode (base-10000 digit groups) -> removes the String+trim+parse chain for `numeric`/`decimal`. -- [ ] C2. Array binary decode -> removes text array parsing. -- [ ] Keep text by design where documented (timestamptz/timetz/interval -- session-tz / composite +- C2. Array binary decode -> removes text array parsing. +- Keep text by design where documented (timestamptz/timetz/interval -- session-tz / composite forms). Measure via the `types` workload (bytes/op + ops/sec) and a JMH decode micro. ## 6. Anything else? (secondary levers) diff --git a/docs/plans/test-vs-jooq.md b/docs/plans/test-vs-jooq.md deleted file mode 100644 index 7c96d928..00000000 --- a/docs/plans/test-vs-jooq.md +++ /dev/null @@ -1,131 +0,0 @@ -# Plan: run jOOQ's integration test suite against our compat driver - -Status: **not started.** A forward plan on the pattern of `test-vs-pgjdbc.md` -and `test-vs-hibernate.md` (both executed; see `compat-suites/`). - -## Goal - -Run jOOQ's own PostgreSQL integration tests -- unmodified, pulled from an -arbitrary jOOQ git ref -- against our `postgresql-client-pgjdbc-compat` driver, and -produce a single consolidated report of what passed, failed, and was skipped. -No jOOQ source is copied into this repo, and no source in the jOOQ checkout is -patched. - -Inputs: - -- a jOOQ git ref (branch, tag, or commit hash) -- our final compat jars (`postgresql-client`, `postgresql-client-jdbc`, `postgresql-client-pgjdbc-compat`) -- a target PostgreSQL server version - -Output: one categorized, baselined report, with a nonzero exit on new failures -so it can gate CI. - -## Why this is worth doing - -jOOQ exercises two driver surfaces that other suites barely touch: - -1. Code generation. jOOQ generates code by reflecting the live schema through - `DatabaseMetaData` and direct `pg_catalog` queries. This is an unusually - thorough stress test of our metadata and system-catalog behavior, and it runs - before any test does. -2. Runtime. The generated-code integration tests hammer `ResultSet`, the full - type system (arrays, enums, ranges, JSON, custom types), and bind-variable - handling. - -PostgreSQL is a jOOQ open-source-supported database, so the open-source edition -covers this path. - -## Key difference from the pgjdbc harness - -jOOQ's tests are written against jOOQ's API and its generated code, not against -`org.postgresql` internals, so there is no compile-time exclusion problem -- this -is a pure runtime driver swap. The distinctive wrinkle here is instead the -two-phase shape: code generation is a gate that must succeed before the runtime -tests can run at all. - -## Approach - -An external, ephemeral harness reusing the shared GAV-shadow driver-substitution -primitive, jOOQ's own schema DDL, and a two-phase run. - -``` -inputs: (jooq ref, compat jars, target PG version) - 1. clone jOOQ @ ref - 2. publish compat jars to a local Maven repo as - org.postgresql:postgresql: (GAV shadow) - 3. start a Postgres for the target version, load jOOQ's postgres test schema - DDL (shipped in the checkout) - 4. phase A -- code generation: run jOOQ codegen against the live schema using - our driver; treat failure here as a hard gate and report it distinctly - 5. phase B -- runtime tests: run the PostgreSQL integration test modules using - our driver - 6. aggregate JUnit results (plus the codegen gate result) -> one categorized, - baselined report -``` - -### Build tool and driver substitution - -jOOQ builds with Maven. Use the shared GAV-shadow primitive: publish our compat -jars to a local Maven repo under `org.postgresql:postgresql:` -and pin jOOQ's PostgreSQL profile to resolve that version through Maven -dependency management. No checkout edits: drive it via `-s settings.xml` -pointing at the local repo plus a forced version property, so the change lives -outside the tree. - -### Schema provisioning: reuse jOOQ's DDL - -jOOQ ships the integration-test schema as per-database DDL scripts in the -checkout (the PostgreSQL variant of its test schema). Load those into the target -server rather than reconstructing them, so the schema stays correct across refs. -This DDL is deliberately broad (many types, constraints, routines) precisely so -codegen and the runtime tests get full coverage. - -### The codegen gate - -If phase A fails, no runtime tests can run, so a raw "0 passed" would be -misleading. Report the codegen result as its own first-class outcome: either -"codegen: ok" (proceed to phase B) or "codegen: failed" with the error, and in -the failed case surface it as the top-line result rather than an empty test -report. - -### Consolidated report - -Post-process JUnit XML into buckets, prefixed by the codegen gate: - -- codegen -- ok / failed (hard gate; failure is the headline) -- passed -- failed -- real compat gaps in types, results, or binds -- skipped (assumption) -- server or feature not present -- known-incompatible (deny-list) -- curated tests that assume pgjdbc-specific - behavior we do not reproduce, each with a reason - -As with the Hibernate harness there is no "excluded (internal)" bucket. Stamp -the report with the jOOQ ref, PG version, and compat jar version, and add a -baseline diff with a nonzero exit on new failures. - -## Tasks - -- [ ] J1. Harness skeleton: script taking (jooq ref, compat jars, PG version); - shallow-clone jOOQ at the ref; validate inputs. -- [ ] J2. GAV-shadow publisher: publish compat jars to a local Maven repo as - `org.postgresql:postgresql:`. (Shared primitive with Hibernate/Spring - harnesses; reuse if already built.) -- [ ] J3. Maven settings/override: `settings.xml` plus forced version property - that pins `org.postgresql:postgresql` to the local repo without editing the - checkout. -- [ ] J4. Schema provisioning: start a Postgres for the target version and load - jOOQ's PostgreSQL test-schema DDL from the checkout. -- [ ] J5. Phase A codegen run: run jOOQ code generation against the live schema - with our driver; capture success/failure and errors as a gate result. -- [ ] J6. Phase B runtime run: execute the PostgreSQL integration test modules - with our driver; capture JUnit XML. -- [ ] J7. Deny-list support: curated known-incompatible tests with reasons, - applied during aggregation. -- [ ] J8. Report aggregator: codegen gate + passed / failed / skipped / - known-incompatible, stamped with ref + PG version + compat version. -- [ ] J9. Baseline + CI gate: store a baseline, diff, exit nonzero on new - failures. -- [ ] J10. Ephemeral container packaging: run the whole flow in a throwaway - container; inputs are just the ref and the jars. -- [ ] J11. Documentation: usage, the two-phase model, reading the report, - updating the baseline and deny-list. diff --git a/docs/plans/test-vs-spring.md b/docs/plans/test-vs-spring.md deleted file mode 100644 index c8238b34..00000000 --- a/docs/plans/test-vs-spring.md +++ /dev/null @@ -1,141 +0,0 @@ -# Plan: run Spring's integration test suites against our compat driver - -Status: **not started.** A forward plan on the pattern of `test-vs-pgjdbc.md` -and `test-vs-hibernate.md` (both executed; see `compat-suites/`). - -## Goal - -Run Spring's own PostgreSQL integration tests -- unmodified, pulled from -arbitrary upstream git refs -- against our `postgresql-client-pgjdbc-compat` driver, and -produce a single consolidated report of what passed, failed, and was skipped. -No Spring source is copied into this repo, and no source in the Spring checkouts -is patched. - -Inputs: - -- upstream git refs for the targeted Spring projects (see scope below) -- our final compat jars (`postgresql-client`, `postgresql-client-jdbc`, `postgresql-client-pgjdbc-compat`) -- a target PostgreSQL server version - -Output: one categorized, baselined report (aggregated across the targeted -projects), with a nonzero exit on new failures so it can gate CI. - -## Scope: which "Spring" - -"Spring" is several projects with different build tools. Running all of Spring -Framework is mostly not database work and would be low signal. Target the -DB-touching modules with the highest application relevance: - -- Spring Data JDBC -- direct CRUD, dialect, and metadata against PG (Maven). -- Spring Data JPA -- exercises the driver through Hibernate underneath, adding - Spring's repository and transaction layer on top (Maven). -- Spring Boot datasource / Testcontainers autoconfiguration tests -- connection - pool setup, health checks, and datasource init against a real PG (Gradle). - -Spring Framework's `spring-jdbc` module tests are an optional add-on if we want -lower-level `JdbcTemplate` coverage, but the three above give the best -signal-to-effort ratio and are the default scope. - -## Why this is worth doing - -This layer catches issues that only appear behind an abstraction: pool -integration, transaction and savepoint semantics, `DatabaseMetaData`-driven -dialect selection, generated keys through repositories, and datasource health -checks. These are exactly the surfaces real Spring applications depend on. - -## Key difference from the pgjdbc harness - -Spring's tests are written against Spring's API, not against `org.postgresql` -internals, so there is no compile-time exclusion problem -- this is a pure -runtime driver swap. The distinctive wrinkle here is the mixed build tools: -Spring Data modules are Maven, Spring Boot is Gradle, so the harness must apply -the shared driver-substitution primitive through both. - -## Approach - -An external, ephemeral harness reusing the shared GAV-shadow driver-substitution -primitive, applied per project through its native build tool, with results -merged into one report. - -``` -inputs: (per-project refs, compat jars, target PG version) - 1. clone each targeted Spring project @ its ref - 2. publish compat jars to a local Maven repo as - org.postgresql:postgresql: (GAV shadow) -- shared by all projects - 3. start a Postgres for the target version (plus Testcontainers wiring for - Boot's container-based tests) - 4. run each project's PostgreSQL-profile tests with our driver: - - Maven projects (Spring Data JDBC/JPA): -s settings.xml + forced version - - Gradle projects (Spring Boot): --init-script dependency substitution - 5. aggregate JUnit results across projects -> one categorized, baselined report -``` - -### Driver substitution: one primitive, two build tools - -The GAV-shadow primitive is the same as the Hibernate and jOOQ harnesses: -publish our compat jars to a local Maven repo under -`org.postgresql:postgresql:`. Apply it per build tool: - -- Maven (Spring Data JDBC/JPA): a `settings.xml` pointing at the local repo plus - a forced version property, so `org.postgresql:postgresql` resolves to our jars - without editing the checkout. -- Gradle (Spring Boot): an `--init-script` using - `resolutionStrategy.dependencySubstitution` to redirect the module to the same - local repo. - -Presenting our driver under the real coordinates and a plausible version matters -here too, because Spring Boot's datasource autoconfiguration and Hibernate's -dialect detection both inspect the driver and its reported version. - -### Database provisioning - -A working PG with a test db/user is enough for Spring Data JDBC/JPA. Spring -Boot's Testcontainers-based tests manage their own container lifecycle; the -harness supplies the target image tag so Boot spins up the right PG version. -Feature-absent tests assumption-skip and land in the report as "skipped." - -### Consolidated report - -Each targeted project emits JUnit XML; merge them into one report tagged by -project, with buckets: - -- passed -- failed -- real compat gaps behind the Spring abstractions -- skipped (assumption) -- server or feature not present -- known-incompatible (deny-list) -- curated per-project tests that assume - pgjdbc-specific behavior we do not reproduce, each with a reason - -As with the Hibernate and jOOQ harnesses there is no "excluded (internal)" -bucket. Stamp the report with each project's ref, the PG version, and the compat -jar version, and add a baseline diff with a nonzero exit on new failures. - -## Tasks - -- [ ] S1. Harness skeleton: script taking (per-project refs, compat jars, PG - version); shallow-clone each targeted Spring project at its ref; validate - inputs. -- [ ] S2. GAV-shadow publisher: publish compat jars to a local Maven repo as - `org.postgresql:postgresql:`. (Shared primitive with Hibernate/jOOQ - harnesses; reuse if already built.) -- [ ] S3. Maven override: `settings.xml` + forced version pinning - `org.postgresql:postgresql` to the local repo for Spring Data JDBC/JPA. -- [ ] S4. Gradle override: `--init-script` dependency substitution pinning the - same module for Spring Boot. -- [ ] S5. Database provisioning: start a Postgres for the target version; wire - Testcontainers image tag for Boot's container-based tests. -- [ ] S6. Per-project test runs: execute the PostgreSQL-profile tests for Spring - Data JDBC, Spring Data JPA, and Spring Boot datasource/autoconfig; capture - JUnit XML per project. -- [ ] S7. Deny-list support: curated per-project known-incompatible tests with - reasons, applied during aggregation. -- [ ] S8. Report aggregator: merge per-project results into passed / failed / - skipped / known-incompatible, tagged by project, stamped with refs + PG - version + compat version. -- [ ] S9. Baseline + CI gate: store a baseline, diff, exit nonzero on new - failures. -- [ ] S10. Ephemeral container packaging: run the whole flow in a throwaway - container; inputs are just the refs and the jars. -- [ ] S11. Optional scope add-on: Spring Framework `spring-jdbc` `JdbcTemplate` - tests, behind a flag. -- [ ] S12. Documentation: scope rationale, usage, reading the merged report, - updating baselines and deny-lists. diff --git a/docs/static-analysis.md b/docs/static-analysis.md index 5026a05d..35576af1 100644 --- a/docs/static-analysis.md +++ b/docs/static-analysis.md @@ -1,212 +1,102 @@ -# Static analysis: what to add, what to skip - -Decision-support for N1.9. The build had no static analysis at all. `-Xlint:all` is now -gated with `-Werror`, and NullAway runs at ERROR over the protocol and core packages; this -is the case for and against everything else. - -The constraint stated up front, because it decides most of these: **the build must -stay fast and must not become a place where correct code has to argue with a tool.** -Current timings on the dev box, `-T 1`, warm local repo: - -| build | wall clock | -| --- | --: | -| `clean compile` | 23 s | -| `clean verify` (unit tests + all gates) | 64 s before, 49 s after adding Error Prone/NullAway (within run-to-run noise) | - -A tool that adds a few seconds is free. One that adds a minute to every `verify` will -get skipped with `-D...skip=true` within a week, which is worse than not having it. - -## Where we already are - -Five gates exist and none of them is a general-purpose analyser: - -| gate | catches | cost | -| --- | --- | --- | -| Spotless (palantir-java-format) | formatting | ~2 s | -| `AsciiSourcePolicyTest` | non-ASCII in source and docs | ~2 s | -| `ModuleLayeringTest` | jdbc/compat importing protocol directly | negligible | -| `NoSynchronizedSourcePolicyTest` | `synchronized` anywhere in shipped code (ADR-0001) | negligible | -| `ApiSurfaceManifestTest` | undeclared public types on the exported surface (ADR-0020) | negligible | -| JaCoCo | 20% line/branch floor | included above | - -They are narrow by design: each encodes one project-specific invariant that no -off-the-shelf tool knows about. What is missing is the generic stuff -- null -handling, resource leaks, API misuse -- which is what the tools below are for. - -## `-Xlint:all` -- **adopted and gated** - -Free (it is javac), zero build-time cost, no dependency. Now `-Werror` with -`-Xlint:all,-this-escape`; the backlog below is cleared. - -It paid for itself on the first clean build by finding a real API leak: -`HostAndPort` had been moved to a non-exported package while still appearing in -`PgConnectionConfig`'s and `SocketTransport`'s public signatures, so callers could -invoke those methods but not name the type. `ApiSurfaceManifestTest` could not see -that -- it checks which *types* are exported, not whether exported *signatures* are -reachable. Complementary gates, both worth having. - -### The remaining 59 warnings, triaged - -| category | count | verdict | -| --- | --: | --- | -| exported class exposes an implicit public constructor | 18 | **Fix.** A default constructor on a utility class is unintended public API. One `private X() {}` each. | -| missing `serialVersionUID` | 15 | **Fix.** These are the exception tree. Driver exceptions cross process boundaries (app servers, remoting); without an explicit ID, deserialization breaks on any recompile. | -| `this-escape` | 10 | **Suppress the category.** Java 21's noisiest lint. Ours are constructors calling overridable methods on `final` classes, which cannot actually be subclassed. Real-bug rate here is zero and the fixes would be contortions. | -| raw types | 5 | **Fix.** `Codec`, `PgRange`, `PgMultirange` used raw in the encoder and range codecs; genuine generics sloppiness in code that does unchecked casts anyway. | -| compat signature leaks (`requires transitive`) | 5 | **Fix by declaring intent.** `PSQLException` exposes `PgException`, `PGInterval` exposes `PgInterval`. Either mark the compat module's dependency transitive or stop exposing those types. | -| `auto-closeable resource ignored` | 2 | **Suppress locally.** The `try (X ignored = ...)` idiom is deliberate -- the resource is being closed, not used. | -| deprecation | 2 | **Keep.** One is `Thread.getId()` in the bench; one is `setUnicodeStream`, a deprecated `java.sql` method we are *required* to implement. | -| `module not found: org.postgresql.client.jdbc` | 1 | **Keep.** Core's qualified export names a module compiled later in the reactor. Benign and unavoidable. | -| redundant cast | 1 | **Fix.** Trivial. | - -**Done (2026-07-26).** All of the above are fixed and `-Werror` is on, so the backlog -cannot silently regrow. Two turned out to be judgement calls I got wrong on the first -pass and corrected: `ProtocolEncoder` and `CodecRegistry` looked like stateless helpers -but are constructed by callers, so they keep public constructors. - -## Candidates, ranked by value per unit of friction - -### 1. NullAway -- **adopted; core and protocol are clean** - -Error Prone plugin; needs Error Prone as the carrier. Landed 2026-07-26. - -**Where it stands.** ERROR severity over `org.postgresql.client.protocol` and all of -`org.postgresql.client.core` (the package prefix covers `codec`, `auth`, `transport` and -`internal`). `postgresql-client-jdbc` and the compat layer are not yet annotated and are -unmeasured; `docs/follow-up.md` carries that item, including why the next pass should not -be scripted. - -The core package started at 146 findings, measured with `-Xmaxwarns` raised (javac caps at -100 by default, which is what made earlier surveys read as exactly "100"). A scripted -attempt was backed out: deriving fixes from the messages cascades outward faster than it -closes (the dominant category grew from 34 to 78 across four rounds), and it produces -annotations that are simply wrong -- `codecContext()` was marked `@Nullable` when it can -never return null. On a Stable type that is a published contract that is false. - -Reading one file (or one closely-related group) per commit converged instead, 146 to 0 -across eight commits, with the count never rebounding after a commit landed. - -**What the passes actually found.** Overwhelmingly, nullability the code already depended -on and only a comment recorded -- `CodecRegistry`'s lookups, `AuthExchange.host()` being -absent on a Unix socket, `StartupResult.backendKeyData` being null when the server sends -none (which is precisely why `backendProcessId()` can return 0), every optional TLS setting -on `SslConfig`, and both bounds of an unbounded `PgRange`. Turning those into declarations -is the value: they were invariants a reader had to take on trust. A second, smaller group -were invariants that wanted an explicit assertion rather than an annotation -- the -prepared-statement cache when caching is disabled, the transient row on a result that -cannot yield one, `sessionState` before the handshake resolves it. - -It also found four things worth fixing rather than annotating. `ArrayCodecs.register` and -`RangeCodecs.registerRange` passed a possibly-null lookup straight into a codec -constructor, correct only because of registration order in `Codecs.defaultRegistry()` -- -now an explicit `requireNonNull` that names the OID. `PgServiceFile` would have thrown an -NPE on a `pg_service.conf` whose first non-comment line was a bare key before any -`[section]` header. `TypeCache` registered a domain codec over a possibly-null base type. -And the `lo_*` fastpath decoders folded an absent result into a length-check message that -named the wrong problem. - -**Cost.** Negligible: `clean verify` measured 64 s before Error Prone/NullAway and 49 s -after, i.e. within run-to-run noise. No separate build phase. - -### 2. Error Prone -- **adopt, as NullAway's carrier and on its own merits** - -**Pro.** Runs inside javac, so no separate phase -- the cheapest way to get real -analysis. Its default ERROR checks are near-zero false positive by policy -(`ReferenceEquality`, `MissingOverride`, `EqualsHashCode`, format-string checks). -Notably relevant here: it catches `==` on boxed types and mistaken `equals` across -types, both easy to hit in codec and metadata code. - -**Con.** Historically painful on new JDKs -- it needs `--add-exports` javac flags -and lags major JDK releases, which matters because we now build on 21 *and* 25. That -is the real cost, not the analysis. Budget for it breaking on a JDK bump. - -**Verdict: adopt at ERROR severity only.** Do not enable the WARNING tier; that is -where the opinionated checks live and where arguments with the tool start. - -### 3. SpotBugs (+ FindSecBugs) -- **adopt for the protocol and codec modules only** - -**Pro.** Bytecode analysis, so it sees things source-level tools miss: unclosed -streams on exception paths, ignored return values, integer overflow in size -arithmetic. That last one is directly on point -- `FrameReader` and `PgReadBuffer` -do bounds arithmetic on attacker-controlled lengths, and we have already had one -overflow bug there (`analysis-013 F14`). FindSecBugs adds crypto/TLS checks relevant -to the SCRAM and TLS code. - -**Con.** The slowest option: a separate `verify`-phase pass over all bytecode, -plausibly +20-40 s on this tree, which is a ~50% increase on `clean verify`. Also -the noisiest -- `EI_EXPOSE_REP` (returning a mutable array) will fire constantly on -code that returns `byte[]` deliberately for performance, and suppressing that class -of finding is most of the setup work. - -**Verdict: adopt narrowly.** Scope it to `postgresql-client-protocol` and the codec -packages, where the value is concentrated and the module is small, and exclude -`EI_EXPOSE_REP*` wholesale. Run it in CI only, not in the local default build. - -### 4. forbidden-apis -- **adopt, cheap and well-targeted** - -**Pro.** Nearly free (a bytecode scan, ~2 s) and enforces things this project -already cares about by convention: default-locale/charset methods (`String.format` -without a `Locale`, `new String(byte[])` without a charset) are exactly the bugs a -driver hits when a user's locale is Turkish. `ReporterLocaleTest` exists because of -this class of bug. It can also ban `System.out`, and JDK-version-inappropriate APIs. - -**Con.** Signature-list maintenance; occasional legitimate use needs a suppression. - -**Verdict: adopt.** Best value-to-cost ratio after `-Xlint`. - -### 5. Checkstyle -- **skip** - -**Pro.** Ubiquitous, configurable. - -**Con.** It overwhelmingly checks style, and Spotless with palantir-java-format -already decides style mechanically and unarguably. Adding Checkstyle means -maintaining a second opinion about formatting that can disagree with the first. Its -non-style checks are a weak subset of Error Prone's. - -**Verdict: skip.** Pure friction here. - -### 6. PMD -- **skip** - -**Pro.** Broad rule set; the CPD copy-paste detector is genuinely useful. - -**Con.** High false-positive rate, rule sets need heavy curation, and it overlaps -Error Prone and SpotBugs without beating either. The main unique value (CPD) does -not justify the ruleset maintenance. - -**Verdict: skip.** Reconsider CPD alone if duplication becomes a concern. - -### 7. revapi / japicmp -- **defer to first release, then mandatory** - -**Pro.** ADR-0020 section 4 asks for exactly this: fail the build on an unapproved -breaking change to the Stable surface. - -**Con.** Diffs against the last *released* artifact, and nothing is published, so it -currently has nothing to compare. `ApiSurfaceManifestTest` is the pre-1.0 stand-in -and covers type membership, though not member-level compatibility. - -**Verdict: defer, but wire it the moment N7.1 publishes a first artifact.** This is -the gate that makes the Stable tier mean anything across versions. - -## Recommended sequence - -1. DONE. Clean the ~40 real `-Xlint` warnings, then `-Werror` with `-this-escape`. *No new - dependencies, no build-time cost.* -2. **forbidden-apis.** Cheap, catches a bug class this project has already been bitten - by. Still open. -3. DONE. **Error Prone at ERROR severity**, then **NullAway** once `@NullMarked` is applied. - Do these together -- Error Prone alone is worth less than Error Prone plus NullAway. -4. **SpotBugs + FindSecBugs, scoped to protocol/codec, CI-only.** Still open. -5. **revapi** at first release. Still open. - -Steps 1-2 should leave `clean verify` under ~70 s. Step 3 adds compile overhead but -no new phase. Step 4 is the only one that meaningfully slows the build, which is why -it is scoped and CI-only. - -## What to keep resisting - -Anything that produces a "score", anything requiring a server (SonarQube), and any -tool whose default configuration must be argued down before the build is green. The -existing gates work because each one encodes a decision this project actually made. -A tool that imports someone else's decisions wholesale will be disabled the first -time it blocks a correct change, and a disabled gate is worse than none -- it looks -like coverage that is not there. +# Static analysis: which tools we run, and why not the others + +The decision record for the driver's static-analysis tooling. It says what is in the +build, what was considered and rejected, and what is scoped but not yet adopted. It is +not a tracker: open work lives in `docs/plans/overall.md` and `docs/follow-up.md`. + +The constraint that decides most of these, stated up front: **the build must stay fast +and must not become a place where correct code has to argue with a tool.** A tool that +adds a few seconds is free. One that adds a minute to every `verify` will get skipped +with `-D...skip=true` within a week, which is worse than not having it. Error Prone and +NullAway were measured against that bar when they landed and came in within run-to-run +noise on `clean verify`. + +## In the build today + +Six gates encode project-specific invariants that no off-the-shelf tool knows about: + +| gate | catches | +| --- | --- | +| Spotless (palantir-java-format) | formatting | +| `AsciiSourcePolicyTest` | non-ASCII in source and docs | +| `ModuleLayeringTest` | jdbc/compat importing protocol directly | +| `NoSynchronizedSourcePolicyTest` | `synchronized` anywhere in shipped code (ADR-0001) | +| `ApiSurfaceManifestTest` | undeclared public types on the exported surface (ADR-0020) | +| JaCoCo | 20% line/branch floor, a regression tripwire rather than an assurance gate | + +Two general-purpose analysers run alongside them, both inside javac (no separate build +phase), configured in the root `pom.xml`: + +- **`-Xlint:all,-this-escape` under `-Werror`.** Free, since it is javac. `this-escape` + is the one exclusion: Java 21's noisiest lint, and ours are constructors of `final` + classes calling overridable methods, where the hazard it warns about cannot occur. + `-Werror` keeps the rest at zero rather than letting a backlog regrow. It paid for + itself on the first clean build by catching an API leak `ApiSurfaceManifestTest` + structurally cannot see: `HostAndPort` had moved to a non-exported package while still + appearing in public signatures, so callers could invoke those methods but not name the + type. The manifest test checks which *types* are exported, not whether exported + *signatures* are reachable. Complementary gates, both worth having. +- **NullAway at ERROR** over `org.postgresql.client.protocol` and all of + `org.postgresql.client.core` (the prefix covers `codec`, `auth`, `transport`, + `internal`). Error Prone is present only as NullAway's carrier, with every one of its + own checks off via `-XepDisableAllChecks`. + +## Rejected + +- **Checkstyle.** It overwhelmingly checks style, and Spotless with palantir-java-format + already decides style mechanically and unarguably. Adding Checkstyle means maintaining + a second opinion about formatting that can disagree with the first. Its non-style + checks are a weak subset of Error Prone's. Pure friction here. +- **PMD.** High false-positive rate, rule sets need heavy curation, and it overlaps Error + Prone and SpotBugs without beating either. Its one genuinely unique feature is the CPD + copy-paste detector, which does not justify the ruleset maintenance. Reconsider CPD + alone if duplication becomes a concern. + +Also worth continuing to resist, as a class: anything that produces a "score", anything +requiring a server (SonarQube), and any tool whose default configuration must be argued +down before the build is green. The existing gates work because each one encodes a +decision this project actually made. A tool that imports someone else's decisions +wholesale will be disabled the first time it blocks a correct change, and a disabled gate +is worse than none -- it looks like coverage that is not there. + +## Accepted but not yet adopted + +These are decided in principle, including their scope. The scope is the point: adopting +any of them unscoped would trip the constraint above. The first three are plan items +N1.23, N1.24, and N1.25 respectively. + +- **forbidden-apis.** Nearly free (a bytecode scan) and enforces what this project + already cares about by convention: default-locale and default-charset methods + (`String.format` without a `Locale`, `new String(byte[])` without a charset) are + exactly the bugs a driver hits when a user's locale is Turkish, which is why + `ReporterLocaleTest` exists. It can also ban `System.out` and JDK-version-inappropriate + APIs. Cost is signature-list maintenance plus the occasional suppression. Best + value-to-cost ratio of what is left. +- **Error Prone's own checks, at ERROR severity only.** Its default ERROR tier is + near-zero false positive by policy (`ReferenceEquality`, `MissingOverride`, + `EqualsHashCode`, format-string checks); relevant here because `==` on boxed types and + mistaken `equals` across types are both easy to hit in codec and metadata code. Do not + enable the WARNING tier, which is where the opinionated checks live and where arguments + with the tool start. The real cost is not the analysis: Error Prone historically needs + `--add-exports` javac flags and lags major JDK releases, which matters because we build + on 21 *and* 25. Budget for it breaking on a JDK bump. +- **SpotBugs + FindSecBugs, scoped to `postgresql-client-protocol` and the codec + packages, CI-only, excluding `EI_EXPOSE_REP*` wholesale.** Bytecode analysis sees what + source-level tools miss: unclosed streams on exception paths, ignored return values, + integer overflow in size arithmetic. That last one is directly on point, since + `FrameReader` and `PgReadBuffer` do bounds arithmetic on attacker-controlled lengths + and have already had one overflow bug there; FindSecBugs adds crypto/TLS checks + relevant to the SCRAM and TLS code. It is also the slowest option (a separate + `verify`-phase pass over all bytecode) and the noisiest: `EI_EXPOSE_REP` fires + constantly on code that returns `byte[]` deliberately for performance, and suppressing + that class of finding is most of the setup work. Hence narrow, and out of the local + default build. +- **revapi (or japicmp), at first release.** ADR-0020 section 4 asks for exactly this: + fail the build on an unapproved breaking change to the Stable surface. It diffs against + the last *released* artifact, and nothing is published yet, so it currently has nothing + to compare; `ApiSurfaceManifestTest` is the pre-1.0 stand-in and covers type membership + but not member-level compatibility. This is the gate that makes the Stable tier mean + anything across versions, so it wants wiring the moment a first artifact exists. + +Extending NullAway to `postgresql-client-jdbc` and the compat layer is also still open, +and carries a specific warning about how not to do it: see `docs/follow-up.md`, N1.9.