Skip to content

Parallelize citus_check_cluster_node_health across source nodes - #8935

Merged
Volkan Cetin (vlkncetin) merged 2 commits into
mainfrom
parallelize-cluster-node-health-check
Oct 20, 2026
Merged

Volkan Cetin (vlkncetin) merged 2 commits into
mainfrom
parallelize-cluster-node-health-check

Conversation

@vlkncetin

Copy link
Copy Markdown
Collaborator

DESCRIPTION: Parallelizes connectivity checks in citus_check_cluster_node_health

Problem

citus_check_cluster_node_health() checks connectivity between every ordered pair
of active readable nodes, including self-pairs. It ran these probes one after another.
On a 33-node cluster that is 1,089 probes, and during cluster initialization this
query took longer than the caller's command timeout:

SELECT json_agg(row_to_json(h)) AS healthcheck
FROM citus_check_cluster_node_health() h;

Change

Only StoreAllConnectivityChecks() in operations/health_check.c changes:

  1. Open connections to all source nodes at the same time (StartNodeConnection +
    FinishConnectionListEstablishment). An unreachable source now costs one
    connection timeout in total instead of one per source.
  2. For each target node, in sorted order, send
    citus_check_connection_to_node(target) to every source, then wait for all
    sources with WaitForAllConnections(..., raiseInterrupts = true) before
    reading results and moving on to the next target.
  3. Store the results in an N×N matrix and write the tuplestore in the original
    source-major order.

Unchanged behavior:

  • Same node set (ActiveReadableNodeList(), sorted, followers included), same
    columns, and all N² pairs, including self-pairs.
  • Rows come out in the same deterministic source-then-target order without an
    ORDER BY.
  • Each source still checks targets one at a time in sorted order, with at most one
    probe in flight per source.
  • true / false / NULL mean the same as before: NULL when the probe could not
    be sent to the source, or the source's response was not OK. Failures are still
    reported as WARNINGs with the same text, using the same calls as
    ExecuteOptionalRemoteCommand. Only the order of warnings from different sources
    can now vary.
  • Cancellation still raises canceling statement due to user request, through the
    existing interrupt handling in WaitForAllConnections.
  • The coordinator still uses one connection per source node.
  • No SQL, catalog, or UDF signature changes. citus_check_connection_to_node is
    untouched, so there is no upgrade or mixed-version concern.

Trade-off: inbound connections on target nodes

Probes for the same target now run at the same time, so a target can briefly receive
one probe connection from every source (up to N) instead of about one at a time.
The total number of probe connections per run does not change, only how many are
open at once.

There is no cap. On a node with fewer than about N+1 free max_connections slots, a
probe to it could fail and be reported as false. With the default
max_connections = 100 there is plenty of headroom at 33 nodes. The benchmark below
measures the peak.

Benchmark

Setup: a 33-node cluster (coordinator + 32 workers) on one 16-core host, PostgreSQL
18.6 built with --enable-cassert, all nodes healthy. Every run returned 1,089 rows,
all true. Each version had one warm-up run, then 5 timed runs of the query above.
Peak inbound connections per node come from log_connections / log_disconnections
over 3 runs.

Serial (before) Concurrent (this PR)
Median wall time, 5 runs 9.794 s 1.462 s (~6.7× faster)
Min / max 8.777 s / 9.916 s 1.352 s / 1.965 s
Inbound connections per node, 3 runs 102–105 102–105 (unchanged)
Peak simultaneous inbound connections per node 2–3 24–34 (median 30)

All 33 nodes shared one machine and assertions were enabled, so CPU contention
limits the speedup. On real multi-host clusters, where network round-trips dominate,
the gain should be larger. This was not measured.

Unresponsive targets (from the new pytest; 3 healthy nodes plus 3 fake nodes that
accept TCP connections but never respond; citus.node_connection_timeout = 1s on
every node):

Serial (before) Concurrent (this PR)
Wall time 12.15 s ~4.06 s

Tests

New src/test/regress/citus_tests/test/test_cluster_node_health.py:

  • test_cluster_node_health_checks_sources_concurrently
    • Adds 3 fake nodes (TCP listeners that never respond) inside a transaction that is
      rolled back. citus.node_connection_timeout is set on every node, because each
      probe's timeout is decided on its source node.
    • Checks the exact rows in source-major order without ORDER BY: real→real is
      true, real→fake is false, fake source is NULL.
    • Checks that wall time is closer to the concurrent estimate than to the serial
      one, and that each fake target saw exactly one probe connection from every real
      source at the same time.
    • Fails on the serial implementation (12.15 s vs an 8 s limit, with correct
      rows) and passes with this change (~4 s).
  • test_cluster_node_health_cancellation_stops_remote_checks
    • Uses a 120 s probe timeout on the sources and cancels the coordinator backend
      while every source has a probe to a fake node in flight.
    • Checks that the query fails with QueryCanceled, and that every source node
      logs canceling statement due to user request for its probe. This shows the
      remote probes are actually cancelled, not just orphaned by the closed socket.
    • Checks that no probe is still active on any node and that all connections to
      the fake node are closed within about 0.06 s, far below the 120 s timeout.

The existing regression tests passed with unchanged expected output, including the
mitmproxy kill/cancel/after(1) cases in failure_connection_establishment.

Validation

Run locally against PostgreSQL 18.6 (--enable-cassert), from src/test/regress:

Command Result
make check-custom-schedule SCHEDULE=<minimal_cluster_management, multi_test_helpers, multi_test_helpers_superuser, connectivity_checks> WORKERCOUNT=2 passed (4/4)
python citus_tests/run_test.py multi_citus_tools passed (7/7)
python citus_tests/run_test.py multi_mx_add_coordinator passed (5/5)
python citus_tests/run_test.py failure_connection_establishment passed (6/6)
make check-follower-cluster (includes multi_follower_select_statements) passed (all tests)
pytest citus_tests/test/test_cluster_node_health.py passed (2/2, 3 consecutive runs)
pytest citus_tests/test/test_cluster_node_health.py::test_cluster_node_health_checks_sources_concurrently against the pre-change build failed as expected (12.15 s vs 8 s limit)
pytest -n auto 12 passed, 1 skipped
uncrustify -c citus-style.cfg --check on health_check.c (uncrustify 0.82.0, citus_indent v0.8.33) passed
black --check, isort --check, flake8 on the new test file passed

Not run locally: PostgreSQL 17 and 19, and the full check-multi / check-failure
schedules. CI covers these.

Comment thread src/test/regress/citus_tests/test/test_cluster_node_health.py Fixed
@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.82%. Comparing base (84489d8) to head (0f980a8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8935   +/-   ##
=======================================
  Coverage   88.81%   88.82%           
=======================================
  Files         290      290           
  Lines       65455    65494   +39     
  Branches     8255     8261    +6     
=======================================
+ Hits        58134    58174   +40     
  Misses       4937     4937           
+ Partials     2384     2383    -1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@onurctirtir Onur Tirtir (onurctirtir) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@vlkncetin
Volkan Cetin (vlkncetin) merged commit fadced9 into main Oct 20, 2026
163 checks passed
@vlkncetin
Volkan Cetin (vlkncetin) deleted the parallelize-cluster-node-health-check branch October 20, 2026 13:16
Volkan Cetin (vlkncetin) added a commit that referenced this pull request Oct 20, 2026
…#8937)

Backports #8935 (fadced9).

Adapts four loops to release-12.1’s `foreach_ptr` macro. Preserves the
merged behavior and regression tests.

Co-authored-by: Volkan Cetin <volkanetin@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Volkan Cetin (vlkncetin) added a commit that referenced this pull request Oct 20, 2026
…#8938)

Backports #8935 (fadced9).

Applies the merged implementation and regression tests unchanged.

---------

Co-authored-by: Volkan Cetin <volkanetin@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
ibrahim halatci (ihalatci) pushed a commit that referenced this pull request Oct 21, 2026
Backport the health-check parallelization from fadced9, including the cancellation-test synchronization correction from release-13.2 (#8938).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ibrahim halatci (ihalatci) pushed a commit that referenced this pull request Oct 21, 2026
…#8939)

Backports #8935 to release-14.0.

- Parallelizes source connections and connectivity probes across source
nodes.
- Preserves result ordering and true/false/NULL semantics.
- Includes the cancellation-test synchronization correction from the
release-13.2 backport (#8938).

Production code matches #8935; both changed files match #8938.

Co-authored-by: Volkan Cetin <volkanetin@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

3 participants