Repository navigation
Integration branch: record all our upstream PRs as merged; CONTRIBUTING - #4
Merged
Merged
Conversation
`binary_copy::write_basic` hangs indefinitely when run on an otherwise idle
connection. It passes only when a busy test suite happens to generate the
concurrent activity that unsticks the loop, which is why the suite appeared
healthy.
The deadlock
process_req's CopyIn arm blocked once the copy channel drained:
Ok(None) => {
nonblock_write(stream, write_buf)?;
copy_in_msg = rcv.recv(); // blocks
}
That parks the only coroutine that reads the socket. Meanwhile copy_in()
queues its initial buffer and waits on responses.next() for BindComplete — a
message only that parked coroutine could decode. Two halves of one connection
waiting on each other.
The fix
Copy data is now drained without blocking. The receiver is held as
`pending_copy_in` state on the connection loop and polled each pass, so reads
keep being serviced while a copy is in flight.
Two consequences of no longer sitting inside the copy arm had to be made
explicit, because the blocking recv had been providing them as side effects:
- CopyInSink wakes the connection after queuing data. Copy data travels on
its own channel rather than the request queue, so nothing else would tell
the loop there is more to write and it would stay parked in wait_io.
- process_req defers other queued requests while a copy is in flight. The
connection is in COPY mode and the server accepts only
CopyData/CopyDone/CopyFail; writing a queued request mid-copy injects it
into the stream and the server rejects the transfer with "unexpected
message type 0x43 during COPY from stdin".
WaitIoWaker is not Clone, so Connection now holds it in an Arc to share with
the sink. process_req no longer writes, so its unused stream parameter is
dropped.
Tests
Four regression tests, each bounded by a 20s deadline so a deadlock fails
legibly instead of hanging CI:
- a COPY IN completes on an idle connection, with no concurrent activity —
the case the existing test could not catch
- the connection stays usable for ordinary queries afterwards
- an empty copy completes, the boundary case where the channel is empty from
the outset
- abandoning a copy without finish() aborts it and leaves the connection
answering
Verified against PostgreSQL 16. binary_copy goes from hanging to 10/10 passing
in 0.08s, and write_basic alone from a timeout to passing in 0.02s.
Notably the suite now completes at all: on the base commit it aborts after 38
tests, and with this change all 81 run. The remaining failures are
environmental — auth-mode tests needing pg_hba entries, and citext/hstore tests
needing those extensions — and the citext/hstore ones are newly visible only
because execution now reaches them.
There is currently no CI, so nothing checks a change before it lands.
Workflow
test — starts PostgreSQL, runs `cargo test --all` against 14 and 16
lint — `cargo fmt --check` and `cargo clippy --all-targets`
Test fixtures
The suite exercises each of PostgreSQL's password authentication methods and
needs `citext` and `hstore`, none of which the stock image provides. Those are
set up by ci/initdb/00-setup.sh, mounted into the container's
docker-entrypoint-initdb.d hook:
- md5_user, pass_user, scram_user, each with the matching verifier
- a pg_hba.conf routing each role to its method
pg_hba is written wholesale rather than appended, because it is
first-match-wins and appending would leave the per-role lines behind the
image's catch-all.
Notes on the shape
PostgreSQL is started with `docker run` rather than a `services:` block: the
container needs fixture scripts from the repository, and service containers
are created before checkout runs.
The readiness check waits for a fixture role to exist, not just for
pg_isready. The entrypoint restarts the server after running initdb scripts,
so pg_isready alone can pass against the temporary instance and race the
restart.
Jobs carry `timeout-minutes`. The suite talks to the database over a
coroutine-driven connection, where a protocol-level stall presents as a hang
rather than a failure; without a limit a stalled run would occupy a runner for
six hours instead of going red.
Clippy is not `-D warnings`. That would fail on pre-existing lints unrelated
to whatever change is under review. Warnings stay in the log and can be
promoted once the tree is clean.
Verified locally with nektos/act. With these fixtures all nine authentication
tests and both extension tests pass, having previously been unrunnable.
Expected result on this branch
This will not go green yet: `copy_in_error`, `copy_in_large` and the
`binary_copy` write tests hang on this base commit. That is the deadlock fixed
in the COPY IN pull request, not a defect in this workflow — the job-level
timeout is what turns it into a legible failure. CI should go green once that
change lands.
There is currently no CI, so nothing checks a change before it lands.
Workflow
test — starts PostgreSQL, runs `cargo test --all` against 14 and 16
lint — `cargo fmt --check` and `cargo clippy --all-targets`
Test fixtures
The suite exercises each of PostgreSQL's password authentication methods and
needs `citext` and `hstore`, none of which the stock image provides. Those are
set up by ci/initdb/00-setup.sh, mounted into the container's
docker-entrypoint-initdb.d hook:
- md5_user, pass_user, scram_user, each with the matching verifier
- a pg_hba.conf routing each role to its method
pg_hba is written wholesale rather than appended, because it is
first-match-wins and appending would leave the per-role lines behind the
image's catch-all.
Notes on the shape
PostgreSQL is started with `docker run` rather than a `services:` block: the
container needs fixture scripts from the repository, and service containers
are created before checkout runs.
The readiness check waits for a fixture role to exist, not just for
pg_isready. The entrypoint restarts the server after running initdb scripts,
so pg_isready alone can pass against the temporary instance and race the
restart.
Jobs carry `timeout-minutes`. The suite talks to the database over a
coroutine-driven connection, where a protocol-level stall presents as a hang
rather than a failure; without a limit a stalled run would occupy a runner for
six hours instead of going red.
Clippy is not `-D warnings`. That would fail on pre-existing lints unrelated
to whatever change is under review. Warnings stay in the log and can be
promoted once the tree is clean.
Verified locally with nektos/act. With these fixtures all nine authentication
tests and both extension tests pass, having previously been unrunnable.
Expected result on this branch
This will not go green yet: `copy_in_error`, `copy_in_large` and the
`binary_copy` write tests hang on this base commit. That is the deadlock fixed
in the COPY IN pull request, not a defect in this workflow — the job-level
timeout is what turns it into a legible failure. CI should go green once that
change lands.
`binary_copy::write_basic` hangs indefinitely when run on an otherwise idle
connection. It passes only when a busy test suite happens to generate the
concurrent activity that unsticks the loop, which is why the suite appeared
healthy.
The deadlock
process_req's CopyIn arm blocked once the copy channel drained:
Ok(None) => {
nonblock_write(stream, write_buf)?;
copy_in_msg = rcv.recv(); // blocks
}
That parks the only coroutine that reads the socket. Meanwhile copy_in()
queues its initial buffer and waits on responses.next() for BindComplete — a
message only that parked coroutine could decode. Two halves of one connection
waiting on each other.
The fix
Copy data is now drained without blocking. The receiver is held as
`pending_copy_in` state on the connection loop and polled each pass, so reads
keep being serviced while a copy is in flight.
Two consequences of no longer sitting inside the copy arm had to be made
explicit, because the blocking recv had been providing them as side effects:
- CopyInSink wakes the connection after queuing data. Copy data travels on
its own channel rather than the request queue, so nothing else would tell
the loop there is more to write and it would stay parked in wait_io.
- process_req defers other queued requests while a copy is in flight. The
connection is in COPY mode and the server accepts only
CopyData/CopyDone/CopyFail; writing a queued request mid-copy injects it
into the stream and the server rejects the transfer with "unexpected
message type 0x43 during COPY from stdin".
WaitIoWaker is not Clone, so Connection now holds it in an Arc to share with
the sink. process_req no longer writes, so its unused stream parameter is
dropped.
Tests
Four regression tests, each bounded by a 20s deadline so a deadlock fails
legibly instead of hanging CI:
- a COPY IN completes on an idle connection, with no concurrent activity —
the case the existing test could not catch
- the connection stays usable for ordinary queries afterwards
- an empty copy completes, the boundary case where the channel is empty from
the outset
- abandoning a copy without finish() aborts it and leaves the connection
answering
Verified against PostgreSQL 16. binary_copy goes from hanging to 10/10 passing
in 0.08s, and write_basic alone from a timeout to passing in 0.02s.
Notably the suite now completes at all: on the base commit it aborts after 38
tests, and with this change all 81 run. The remaining failures are
environmental — auth-mode tests needing pg_hba entries, and citext/hstore tests
needing those extensions — and the citext/hstore ones are newly visible only
because execution now reaches them.
`NotificationResponse` was decoded and then dropped:
BackendMessage::Async(Message::NotificationResponse(_body)) => {}
so `LISTEN` could be issued but nothing could ever be observed. `Notification`
and `AsyncMessage` remained in lib.rs as vestiges of the tokio-postgres origin,
along with a commented-out `notifications` test, but nothing constructed them.
The connection coroutine now decodes each notification and pushes it onto an
unbounded queue that the client drains:
let client = may_postgres::connect("host=localhost user=postgres")?;
client.batch_execute("LISTEN orders")?;
while let Some(n) = client.notifications().pop() {
println!("{}: {}", n.channel(), n.payload());
}
Design
Uses `may::queue::mpsc::Queue`, the same primitive already carrying the
request queue between client and connection. No futures, no streams, no
reactor — draining fits in whatever loop the caller already runs, which suits
the coroutine model.
The upstream tokio-postgres approach (`connection.poll_message(cx)` yielding
an `AsyncMessage` stream) is deliberately not revived; the commented-out test
that used it is replaced with tests for the queue API.
The queue is unbounded and only the caller drains it, so a client that
subscribes and never reads will grow it without limit. Documented on the
accessor.
Tests
Four covering delivery to a LISTENing client from a second session, ordering
and payload preservation across several notifications, isolation so an
unsubscribed channel is never queued, and that a client which never issues
LISTEN receives nothing.
Verified against PostgreSQL 16. Three tests fail in this environment
(custom_array, pipelined_prepare, plain_password_wrong) — confirmed failing
identically on the unmodified base commit, so unrelated.
Example
examples/listen_notify.rs shows the two-connection pattern and a long-running
listener of the kind a cache invalidator or job runner would use.
There is currently no CI, so nothing checks a change before it lands.
Workflow
test — starts PostgreSQL, runs `cargo test --all` against 14 and 16
lint — `cargo fmt --check` and `cargo clippy --all-targets`
Test fixtures
The suite exercises each of PostgreSQL's password authentication methods and
needs `citext` and `hstore`, none of which the stock image provides. Those are
set up by ci/initdb/00-setup.sh, mounted into the container's
docker-entrypoint-initdb.d hook:
- md5_user, pass_user, scram_user, each with the matching verifier
- a pg_hba.conf routing each role to its method
pg_hba is written wholesale rather than appended, because it is
first-match-wins and appending would leave the per-role lines behind the
image's catch-all.
Notes on the shape
PostgreSQL is started with `docker run` rather than a `services:` block: the
container needs fixture scripts from the repository, and service containers
are created before checkout runs.
The readiness check waits for a fixture role to exist, not just for
pg_isready. The entrypoint restarts the server after running initdb scripts,
so pg_isready alone can pass against the temporary instance and race the
restart.
Jobs carry `timeout-minutes`. The suite talks to the database over a
coroutine-driven connection, where a protocol-level stall presents as a hang
rather than a failure; without a limit a stalled run would occupy a runner for
six hours instead of going red.
Clippy is not `-D warnings`. That would fail on pre-existing lints unrelated
to whatever change is under review. Warnings stay in the log and can be
promoted once the tree is clean.
Verified locally with nektos/act. With these fixtures all nine authentication
tests and both extension tests pass, having previously been unrunnable.
Expected result on this branch
This will not go green yet: `copy_in_error`, `copy_in_large` and the
`binary_copy` write tests hang on this base commit. That is the deadlock fixed
in the COPY IN pull request, not a defect in this workflow — the job-level
timeout is what turns it into a legible failure. CI should go green once that
change lands.
…ke the response sender owned rather than a transmuted borrow Dropping a Client leaves its server backend open until process exit: drop cancels the io coroutine, cancel() does not run the coroutine destructors, and the TcpStream is owned by the coroutine closure, so the fd is never closed. Ten connect/query/drop cycles leave ten backends in pg_stat_activity and ten sockets in /proc/self/fd, one for one, and a process that opens connections over time eventually meets the servers too-many-clients limit. The obvious repair - have the io loop exit gracefully so it closes its own socket - trips over a second, latent bug. Requests carry the response sender as RefOrValue with a transmuted static borrow of the CoChannel field inside Client, under the stated safety condition that the client waits until all responses come back. An instantly-cancelled coroutine upholds that condition by force; a graceful one runs at least one more pass and pushes responses through a pointer into freed Client memory. Under load that shows up as SIGSEGV and SIGABRT inside the spsc send. So the fix has two halves that only work together: 1. The sender is now Arc-owned, cloned into each Request, and the RefOrValue transmute is deleted. Queued requests keep the channel alive however long the io coroutine outlives the Client, making shutdown ordering safe by construction. 2. Connection::drop sets a shared AtomicBool and wakes the loop with the same waker send() uses, then DETACHES. The loop polls the flag at the top of each pass, and on every exit path sends the protocol Terminate and shuts down its own socket on its own stack. Drop deliberately does not join: a Client can be dropped inside a coroutine that is itself being cancelled, and parking there aborts the process. The waker holds an Arc into the reactor slot, so a wake after the loop has already exited is a no-op. The unsafe cancel() is gone. Verified, each against PostgreSQL 15: - examples/leak_probe.rs: backends AND open socket fds stay at baseline across 10 connect/query/drop cycles; previously both climbed one per cycle and never returned. - Soak: 500 sequential connect/query/drop against a server with max_connections set to 100 - impossible without live reclamation - both counters pinned at baseline throughout. - The added regression test asserts pg_stat_activity returns to baseline, tagged via application_name so it is immune to parallel tests. - A downstream ORM integration suite of 139 tests that opens a connection per test: previously failed at exactly the connection limit; now 138 passed / 0 failed across 12 consecutive runs with no crashes. - This crate-s own suite: the same tests pass and fail before and after (the failures are auth roles and custom types the local server does not provide).
There is currently no CI, so nothing checks a change before it lands.
Workflow
test — starts PostgreSQL, runs `cargo test --all` against 14 and 16
lint — `cargo fmt --check` and `cargo clippy --all-targets`
Test fixtures
The suite exercises each of PostgreSQL's password authentication methods and
needs `citext` and `hstore`, none of which the stock image provides. Those are
set up by ci/initdb/00-setup.sh, mounted into the container's
docker-entrypoint-initdb.d hook:
- md5_user, pass_user, scram_user, each with the matching verifier
- a pg_hba.conf routing each role to its method
pg_hba is written wholesale rather than appended, because it is
first-match-wins and appending would leave the per-role lines behind the
image's catch-all.
Notes on the shape
PostgreSQL is started with `docker run` rather than a `services:` block: the
container needs fixture scripts from the repository, and service containers
are created before checkout runs.
The readiness check waits for a fixture role to exist, not just for
pg_isready. The entrypoint restarts the server after running initdb scripts,
so pg_isready alone can pass against the temporary instance and race the
restart.
Jobs carry `timeout-minutes`. The suite talks to the database over a
coroutine-driven connection, where a protocol-level stall presents as a hang
rather than a failure; without a limit a stalled run would occupy a runner for
six hours instead of going red.
Clippy is not `-D warnings`. That would fail on pre-existing lints unrelated
to whatever change is under review. Warnings stay in the log and can be
promoted once the tree is clean.
Verified locally with nektos/act. With these fixtures all nine authentication
tests and both extension tests pass, having previously been unrunnable.
Expected result on this branch
This will not go green yet: `copy_in_error`, `copy_in_large` and the
`binary_copy` write tests hang on this base commit. That is the deadlock fixed
in the COPY IN pull request, not a defect in this workflow — the job-level
timeout is what turns it into a legible failure. CI should go green once that
change lands.
Client::_is_closed was a stub (`unimplemented!()`, with the intended body commented out), so there was no way to tell that a connection had died. The connection coroutine now sets a flag when its loop exits (an I/O or protocol error, or the server ending the session), and Client::is_closed() returns it, as in tokio-postgres. Test: a connection terminated with pg_terminate_backend reports closed.
A statement streamed with `query_raw` ends with a CommandComplete message carrying its row count, but RowStream dropped it, so a caller that pipelines writes through `query_raw` could not learn how many rows each changed without a separate `execute`. RowStream::rows_affected() returns the count once the stream has ended: Some(n) from the CommandComplete tag (Some(0) for an empty query), None before the end, for a suspended portal, or for a tag without a count. Test: an UPDATE without and with RETURNING, and two pipelined streams that each report their own count.
The connection decoded NotificationResponse and dropped it, so a client
could issue LISTEN but never observe a notification. Notification and
AsyncMessage were left in lib.rs from the tokio-postgres port, with a
commented-out test, but nothing constructed them.
The connection coroutine now pushes each notification onto an unbounded
queue (may's mpsc Queue, as used for requests), which
Client::notifications() exposes for the caller to drain:
client.batch_execute("LISTEN orders")?;
while let Some(n) = client.notifications().pop() {
println!("{}: {}", n.channel(), n.payload());
}
The commented-out test, written against tokio-postgres's poll_message
stream, is replaced by tests for delivery from another session, order
and payloads, channel isolation, and nothing delivered without LISTEN.
Client::_is_closed was a stub (`unimplemented!()`, with the intended body commented out), so there was no way to tell that a connection had died. The connection coroutine now sets a flag when its loop exits (an I/O or protocol error, or the server ending the session), and Client::is_closed() returns it, as in tokio-postgres. Test: a connection terminated with pg_terminate_backend reports closed.
Client::notifications() is a queue the caller has to poll. Add Client::next_notification(timeout), which parks until a notification is delivered, the timeout passes, or the connection closes. The notification queue is paired with a semaphore posted on every push, and once more when the connection coroutine exits. A waiter returns queued notifications first, wakes on delivery, returns None at its timeout, and returns promptly once the connection is closed. Waits are never shorter than 1 ms, because may (up to 0.3.51) treats a park timeout under 1 ms as no timeout. Tests: a parked waiter wakes on arrival; queued notifications come back in order, then an empty wait lasts about its timeout; a terminated connection releases the waiter; sub-millisecond timeouts return.
Its changes are here as 2a8fa9c (delivery), a400752 (COPY IN), 8f7df1e (CI); docs since revised; recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
Its changes are here as a400752; recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
Its changes are here as 8f7df1e (+ 6ca08d1 TEST_PG_PORT); recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
…anch) Its changes are here as 46ce747, extended by 2d4127c; recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
Its changes are here as afd37db; recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
Its changes are here as 2a8fa9c with revised docs (3597ac0); recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
Its changes are here as the fork's Client::is_closed (2d4127c's dead flag); recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
Its changes are here as 3597ac0 + f2446e0 (1 ms guard); recorded as merged so branch comparisons and patch-id checks see it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
…-fork Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Creates the fork's integration branch with every change we have proposed upstream recorded on it.
integration/microscaler-forkstarts atmaster(cb92eff), which already carries upstream master and the content of all our PR branches, but as the fork's own commits, so GitHub showed each PR branch as unmerged. This PR records each branch as merged (merge commits that keep this branch's tree, each naming the fork commit that carries the change): Xudong-Huang#2, #3, #4, #6, Xudong-Huang#7, Xudong-Huang#8, Xudong-Huang#9, Xudong-Huang#10. Fork-only fixes (fail pending on loop exit, 1 MiB I/O stack,sslmode=disable, rustls/sslmode, the 1 msnext_notificationguard,user_column_decode_failed) were already ancestors. CONTRIBUTING now names this branch;masterstays where it is because lockfiles pin it.No code change: the tree equals
masterapart from CONTRIBUTING.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01Y3TSRmEZAYU6RBUXhkn7WF