Skip to content

Make rebalance colocation-group representative selection deterministic - #8789

Draft
ibrahim halatci (ihalatci) wants to merge 1 commit into
mainfrom
ihalatci-rebalance-test-determinism
Draft

Make rebalance colocation-group representative selection deterministic#8789
ibrahim halatci (ihalatci) wants to merge 1 commit into
mainfrom
ihalatci-rebalance-test-determinism

Conversation

@ihalatci

Copy link
Copy Markdown
Contributor

This is a product defect, not a test flake. It was found via a nightly background_rebalance_parallel_reference_tables
failure, but the test was reporting the bug correctly.

Root cause

NonColocatedDistRelationIdList() (shard_rebalancer.c:2013-2058) builds its list from
CitusTableTypeIdList() and then keeps the first table seen per colocationId (the dedup
continue at :2050).

CitusTableTypeIdList() scans pg_dist_partition with an unordered systable_beginscan
(metadata_cache.c:5078-5080). So the representative table for each colocation group is whichever
tuple physical order happens to yield — it can change after a HOT update, a vacuum, or any other
storage-level churn, with no schema or data change at all.

Two consequences:

  1. The representative relation name is printed in DEBUG1 output.
  2. The shard IDs emitted in the rebalance plan are those of whichever table won.

The second is the serious one, and it also exposes the sibling test background_rebalance_parallel,
which is only shielded from the first because it never raises client_min_messages to DEBUG1. That
ruled out a test-only fix.

Reproduced on demand

Two arms on a fresh 3-node cluster with a forced HOT update on pg_dist_partition, differing only in
the installed library (verified binary-identical to the intended build at both ends of each arm):

Library representative before churn after churn
Unpatched table1_colg1 table2_colg1 ← bug reproduces
Patched table1_colg1 table1_colg1 ← fixed

Measured OIDs: 17320 table1_colg1 < 17326 table2_colg1.

Fix

Sort by OID before the de-duplication. Lowest OID is creation order, which is exactly what the
existing committed baselines already assert — so this converts an incidental property into a
guaranteed one rather than changing behaviour.

The sort is in-place on a list built fresh per call, and needs no new #include.

Validation

  • Full check-operations17/17 pass, with the installed library MD5-verified against the build
    tree both before and after the run.
  • Zero expected-output churn. git status --porcelain -- src/test/regress/ is empty; no .out and
    no .sql file is touched by this PR. That is the actual evidence this is a real fix and not
    test-shaping.
  • Six-call-site audit of NonColocatedDistRelationIdList() consumers (:1138, :1179, :1334,
    :1445, :3787, :4051) — all clean; none depends on the previous arbitrary order.

Deliberately not changed

CitusTableTypeIdList() in metadata_cache.c is left unordered. Sorting at the source would impose
cost on every caller; the rebalancer is the one that needs the ordering guarantee.

Refs #8776

NonColocatedDistRelationIdList() builds its list from CitusTableTypeIdList(),
which scans pg_dist_partition with an unordered heap scan. The first-wins
de-duplication that follows therefore picks an arbitrary table to represent
each colocation group, depending on physical tuple order.

That makes the rebalance plan non-reproducible for identical cluster state:
the representative relation name appears in DEBUG1 output, and the shard IDs
emitted in the plan are those of whichever table won. This flaked the nightly
cassert run in background_rebalance_parallel_reference_tables, and
background_rebalance_parallel is exposed to the same shard-ID instability.

Sorting by oid makes the lowest-oid table win, which is creation order and
matches what the existing expected output already asserts, so no baselines
change.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0789ce16-9e4e-4f49-b486-dad8d41d9b87
@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.68%. Comparing base (52663ed) to head (aa99c96).

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

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.

2 participants