Repository navigation
test(#14): same-slot drain-pin probe (diagnostic) - #19
Conversation
Adds tests/same-slot-probe.sh (+ same-slot-probe.mjs and pgvpd-pool-size1.conf): a single-connection-pool probe that measures how long an abandoned in-flight query pins a pooled slot before the next client can reuse it — the drain-pin introduced by #11's correct-but-pinned checkin. Measured on 1.0.3: an abandoned SELECT pg_sleep(3) pins the slot ~2.95s (the query's remaining runtime); pg_sleep(8) pins ~5.05s then the slot is discarded and recreated (capped by the 5s reset timeout); every reused slot returns a correct result. The pin only bites under pool saturation — with headroom the next checkout gets a different slot. It is a measurement, not wired into run.sh, so CI stays green; set THRESHOLD_MS to make it a red-then-green pass/fail pin for the #14 fix (cancel the abandoned query via client_connection_check_interval so the slot returns immediately). Test/diagnostic only; no library change.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4b1833a01
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ( cd tests/drizzle && PGVPD_HOST="$PG_HOST" PGVPD_PORT="$PGVPD_PORT" \ | ||
| SLEEP_S="${SLEEP_S:-3}" TRIALS="${TRIALS:-5}" THRESHOLD_MS="${THRESHOLD_MS:-}" \ | ||
| node same-slot-probe.mjs ) |
There was a problem hiding this comment.
On a clean checkout, tests/drizzle/node_modules is absent and gitignored, so this direct node invocation fails with ERR_MODULE_NOT_FOUND for pg before the documented probe can run. The existing Drizzle runners install dependencies first; this standalone wrapper should likewise run npm ci/npm install when needed or document and validate that prerequisite.
Useful? React with 👍 / 👎.
| PGPASSWORD=testpass psql -h "$PG_HOST" -p "$PG_PORT" -U postgres -d pgvpd_test \ | ||
| -f tests/fixtures.sql -v ON_ERROR_STOP=1 >/dev/null |
There was a problem hiding this comment.
When the compose service is already running, this unconditionally reloads tests/fixtures.sql into the retained database, but its INSERT INTO tenants has no uniqueness constraint or conflict handling. A second probe run therefore duplicates each tenant's rows, and the probe's n === 2 correctness check fails even though pgvpd is correct; reset the relevant tables or recreate the test database before loading fixtures.
Useful? React with 👍 / 👎.
|
|
||
| // Correctness is always required. Latency only fails when a threshold is set. | ||
| const correctnessFail = !correct; | ||
| const latencyFail = THRESHOLD_MS !== null && abandonMed > THRESHOLD_MS; |
There was a problem hiding this comment.
When clean checkout latency is significant, comparing THRESHOLD_MS to abandonMed can report RED even after abandoned-query cancellation removes all incremental delay. The probe already defines the pin as abandonMed - cleanMed, and the README describes the threshold as applying to that pin, so the verdict and exit condition should compare against the computed delta rather than total checkout latency.
Useful? React with 👍 / 👎.
| -f tests/fixtures.sql -v ON_ERROR_STOP=1 >/dev/null | ||
|
|
||
| lsof -tiTCP:$PGVPD_PORT -sTCP:LISTEN 2>/dev/null | xargs kill 2>/dev/null || true | ||
| "$BIN" --config tests/pgvpd-pool-size1.conf > "$PGVPD_LOG" 2>&1 & |
There was a problem hiding this comment.
If the advertised PGVPD_TEST_PG_PORT override differs from 15432, fixtures are loaded through that selected port but pgvpd is still started with pgvpd-pool-size1.conf, whose upstream port is hard-coded to 15432. The probe then measures a different database or fails despite the selected database being available; pass --upstream-port "$PG_PORT" or generate a matching configuration.
Useful? React with 👍 / 👎.
| if [ ! -x "$BIN" ]; then | ||
| echo "Building pgvpd (release)…" | ||
| cargo build --release | ||
| BIN=./target/release/pgvpd |
There was a problem hiding this comment.
When a caller supplies a mistyped or non-executable PGVPD_BIN, this branch silently builds and substitutes the current checkout's release binary. The command then produces plausible measurements attributed to the requested build while actually testing another binary; only the unset default should trigger a build, while an invalid explicit override should fail immediately.
Useful? React with 👍 / 👎.
1 participant
Adds the reproduction for #14 that measures the drain-pin introduced by #11's correct-but-pinned checkin. Diagnostic/test-only, not wired into
run.sh(CI stays green)../tests/same-slot-probe.shruns pgvpd on a single-connection pool so the next checkout must reuse the abandoned slot, then times it.Measured on 1.0.3:
SELECT pg_sleep(3)SELECT pg_sleep(8)Results are correct in every case — the pin is the cost of correctness, not a poison. The pin only bites under pool saturation; with headroom the waiter gets a different slot (which is why the size-20 consumer suite saw it at 0.9%).
THRESHOLD_MSturns the probe into a red-then-green pass/fail pin for the #14 fix: RED while the slot is pinned, green once the backend cancels the abandoned query (client_connection_check_interval) and the slot returns immediately. NexusPlus plans to wire it into their contract suite as a same-slot lane.Refs #14.