Skip to content

Fix flaky single_node_enterprise multi-shard connection race - #8788

Closed
ibrahim halatci (ihalatci) wants to merge 1 commit into
mainfrom
ihalatci-single-node-extra-connection
Closed

Fix flaky single_node_enterprise multi-shard connection race#8788
ibrahim halatci (ihalatci) wants to merge 1 commit into
mainfrom
ihalatci-single-node-extra-connection

Conversation

@ihalatci

Copy link
Copy Markdown
Contributor

single_node_enterprise asserts on the exact set of NOTICE: issuing ... lines produced by a
6-shard multi-shard SELECT. That output is racy by design.

Why

CalculateNewConnectionCount() (adaptive_executor.c:2734) decides on the executor's first cycle to
open 3 additional connections beyond the first:

  • maxNewConnectionCount = targetPoolSize - initiatedConnectionCount (:2772)
  • newConnectionsForReadyTasks = Max(0, readyTaskCount - usableConnectionCount) (:2781)
  • the slow-start cap at :2784 is inside if (ExecutorSlowStartInterval != SLOW_START_DISABLED), and
    the regression harness sets citus.executor_slow_start_interval = 0ms
    (pg_regress_multi.pl:641), so it is skipped
  • Min(4, 3) = 3 at :2794
  • the cost guard at :2843 cannot fire on the first cycle

So 3 extra connections are started on every run, including every green one. The decision is
deterministic; what is not deterministic is whether any of those connections completes TCP connect
plus authentication before the 6 very small sequential queries have already drained. When one wins,
its BEGIN is emitted into the notice stream and the diff appears.

Because there is exactly one last task, a winning connection can insert its BEGIN in exactly one
place — which is why every observed failure diff is byte-identical. That is the signature of this
race, not evidence against one.

Fix

Pin citus.max_adaptive_executor_pool_size to 1 for the duration of the block, so no extra
connection is ever started and the notice stream is fully determined.

+6/−0, zero deletions. The zero-deletion count is the load-bearing verification: if pinning the
pool had altered execution in any way, existing NOTICE: issuing ... lines would have had to change.
They did not. No baseline was re-recorded to make this pass.

The test occupies its own test: line in both enterprise_minimal_schedule and
enterprise_schedule, and there is no parallel-schedule variant, so only external load can win the
race — consistent with it being seen on shared CI runners and rarely locally.

History

This has been failing intermittently for two years. #7671 reports it with the identical hunk header
@@ -465,28 +465,30 @@, and #8348 lists it as item 2 of the unstable-test tracker.

Fixes #7671
Refs #8348 (unstable test list, item 2)
Refs #8776

The "followed by a multi-shard command" block after master_drain_node
intermittently logged an extra

  issuing BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED;
  SELECT assign_distributed_transaction_id(...)

just before the last shard query, plus a matching extra ROLLBACK.

The regression suite runs with citus.max_adaptive_executor_pool_size=4
and citus.executor_slow_start_interval=0ms, so slow start is disabled.
On the first ManageWorkerPool() cycle of "SELECT count(*) FROM test",
CalculateNewConnectionCount() sees readyTaskCount=4, usableConnections=0
and initiatedConnections=1, giving newConnectionCount=3. The cost guard
in UsingExistingSessionsCheaperThanEstablishingNewConnections() cannot
suppress this because workerPool->totalExecutedTasks is still 0, so it
returns false. The executor therefore always starts 3 extra connections.

Whether they are visible is a pure race: normally all six shard queries
complete on the pre-existing connection before any of the three finish
connecting, so nothing is logged. On a loaded machine one of them wins
the race in time to claim the last task, emitting the extra BEGIN and,
at rollback, an extra ROLLBACK. This is also why the recorded diff is
always byte-identical even though the failure is intermittent: only one
task remains, so a winner can appear in exactly one place, immediately
before the query on the sixth shard.

Pin the pool size for that block so no additional connections can be
opened, which makes the output deterministic without weakening what the
test asserts (that all six shards are executed remotely).

This has been reported independently three times across two years, on
PG16, PG17 and PG18 and on different operating systems, always with the
same diff at the same line. That matches the analysis above: what the
reporters needed was a repeated schedule or a small, slow CI machine,
not any particular version or platform.

Verified on PG17 with a build that delays each remote task by 3ms: this
reproduces the failure before the change (three extra BEGIN/ROLLBACK
pairs) and passes 3/3 with it. Also passes 3/3 on a clean build.

Fixes #7671
Refs #8348 (unstable test list, item 2)
Refs #8776 (nightly cassert consolidated triage)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ihalatci

Copy link
Copy Markdown
Contributor Author

Note for reviewers: this overlaps #8752.

#8752 (opened 2026-08-12, still draft, "Fixes #8749") makes the functionally identical change — same file, same line, SET LOCAL citus.max_adaptive_executor_pool_size TO 1. I did not see it until after opening this PR; the two were arrived at independently, which is decent corroboration that the fix is the right one.

Differences, so a decision is easy:

Either is fine to merge — they should not both be. Happy to close this in favour of #8752 plus a follow-up comment, or to have #8752 closed as superseded. No preference beyond not shipping it twice.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.70%. Comparing base (52663ed) to head (1e27ede).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8788      +/-   ##
==========================================
- Coverage   88.70%   88.70%   -0.01%     
==========================================
  Files         289      289              
  Lines       64889    64890       +1     
  Branches     8181     8180       -1     
==========================================
  Hits        57559    57559              
+ Misses       4967     4966       -1     
- Partials     2363     2365       +2     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ihalatci

Copy link
Copy Markdown
Contributor Author

Superseded by #8752, which made the identical change (same two files, same line, same placement) and was opened first. Closing this in its favour rather than duplicating the fix.

The root-cause analysis and the Fixes #7671 / Refs #8348 links from this PR have been moved into #8752's description, so nothing is lost by closing here. Branch deleted.

@ihalatci
ibrahim halatci (ihalatci) deleted the ihalatci-single-node-extra-connection branch August 20, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky single_node_enterprise test

2 participants