fix(universaldb): run postgres transaction reads concurrently

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5704 fix(universaldb): run postgres transaction reads concurrentl … 👈
• #5705 test(gasoline): reproduce slow workflow pulls on postgres
• #5702 fix(universaldb): chunk postgres commit requests that exceed …
• #5703 fix(universaldb): clear postgres key ranges one statement pe …
• #5706 perf(universaldb): time each drain batch statement to locali …
• #5695 fix(universaldb): report the last error when transaction ret …
• main

Get stack: forklift get 5704
Push local edits: forklift submit
Merge when ready: forklift merge 5704

_{change ymtnsyuw}

[the-company-company]

✅ No issues found

_{Reviewed commit 653af53.}

[claude]

Review

Nice fix — moving from sequential per-command awaits to a FuturesUnordered-backed select loop is a clean way to let tokio-postgres pipeline reads on the single pinned snapshot connection, and the commit-drain ordering (while reads.next().await.is_some() before tx.commit()) correctly preserves the invariant that all outstanding reads finish against the snapshot before it's ended. The extraction of handle_get* into free functions taking &Transaction<'_> is the right call since it's required for the borrow checker to allow concurrent futures over a shared &Transaction.

Code quality

• engine/packages/universaldb/src/driver/postgres/transaction_task.rs: logic looks correct. Comments explain the why (pipelining, drain-before-commit) rather than the what, per repo convention.

Test coverage concern

• engine/packages/universaldb/tests/postgres_concurrent_reads.rs::concurrent_reads_share_round_trips is not marked #[ignore], so it runs by default under cargo test -p universaldb. It requires docker run --cap-add NET_ADMIN plus apk add --no-cache iproute2-tc at test time (network access to the alpine package mirror). The existing precedent for this exact netem-sidecar pattern, engine/packages/gasoline/tests/pull_backlog.rs::add_network_latency, is explicitly marked #[ignore = "investigation harness; run explicitly with --ignored --nocapture"] for this reason.
  • Contributors/CI environments without Docker NET_ADMIN privilege (e.g. rootless Docker) or without outbound access to the alpine package repo will fail this test even though the code under test is correct, a spurious failure unrelated to functional regressions.
  • Worth deciding deliberately: if this is meant to be a real CI regression guard (reasonable, since it directly protects the fix in this PR), consider vendoring tc/iproute2-tc into the test-deps postgres image so it doesn't depend on network access at test time, or otherwise confirm CI always has the required Docker privileges. If it's meant more as a diagnostic/benchmark like pull_backlog.rs, it should probably get the same #[ignore] treatment for consistency.

Minor

• The FuturesUnordered of in-flight reads has no concurrency cap. Since transaction.rs uses mpsc::unbounded_channel(), a caller that fans out a very large batch of reads (the PR's own test docstring cites gasoline loading history for every workflow a pull leased, which pull_backlog.rs defaults to 5,000) will fire all of them as concurrently pipelined queries against one connection. This is presumably the intended behavior and tokio-postgres pipelining should handle it fine, but flagging in case an upper bound was intended.
• add_network_latency in the new test blocks the async test thread on a synchronous Command::output() call while installing packages/setting up tc. Since the test runs with 4 worker threads it's unlikely to starve the runtime, but tokio::task::spawn_blocking would be more idiomatic here and in the near-identical helper in pull_backlog.rs.

Overall the core driver fix is solid; the main thing to resolve before merge is whether the new test's default (non-ignored) execution and its Docker/network runtime requirements are intentional for this crate's CI.