Skip to content

Upgrade from math/rand to math/rand/v2#1788

Open
Onyx2406 wants to merge 2 commits intoClickHouse:mainfrom
Onyx2406:fix/upgrade-math-rand-v2
Open

Upgrade from math/rand to math/rand/v2#1788
Onyx2406 wants to merge 2 commits intoClickHouse:mainfrom
Onyx2406:fix/upgrade-math-rand-v2

Conversation

@Onyx2406
Copy link
Copy Markdown
Contributor

@Onyx2406 Onyx2406 commented Mar 7, 2026

Summary

  • Migrate all math/rand usages to math/rand/v2 across the entire codebase (19 files)
  • Fix ConnOpenRandom strategy to pick random offset once outside the loop
  • Re-enable skipped random failover tests

Motivation

Go 1.25 broke rand.Seed which caused ConnOpenRandom failover tests to be skipped (#1652). The math/rand package is deprecated in favor of math/rand/v2 which provides automatic seeding and better APIs.

As discussed in #1653, the ConnOpenRandom strategy also had a bug where rand.Int() was called on every loop iteration, meaning a connection attempt could pick the same (broken) address multiple times and fail to try all addresses.

Changes

Core library (clickhouse.go, clickhouse_std.go):

  • Replace math/rand with math/rand/v2
  • Fix ConnOpenRandom: pick random starting offset once via rand.IntN(len(addrs)) before the loop, then iterate through all addresses deterministically from that offset. This ensures every address is tried exactly once before failing.

Tests (11 files):

  • Replace rand.NewSource/rand.New with rand.NewPCG/rand.New
  • Replace rand.Intn(n) with rand.IntN(n) (v2 naming)
  • Replace src.Int63() with rng.Int64() in string generators
  • Remove rand.Seed calls (v2 auto-seeds)
  • Re-enable TestConnFailoverRandom and TestStdConnFailoverRandom

Examples (5 files):

  • Remove rand.Seed calls from multi-host and test setup
  • Update ResetRandSeed() to no-op (v2 has no rand.Seed)
  • Remove unused imports (time, math/rand)

Test Plan

  • go build ./... passes cleanly
  • No remaining math/rand v1 imports (grep confirms zero matches)
  • TestConnFailoverRandom and TestStdConnFailoverRandom re-enabled (previously skipped)
  • CI integration tests (require ClickHouse Docker container)

Fixes #1653

Migrate all usages of the deprecated math/rand package to math/rand/v2
across the codebase. This fixes the Go 1.25 compatibility issue where
rand.Seed no longer works (ClickHouse#1652).

Key changes:
- Replace math/rand imports with math/rand/v2 in all 19 affected files
- Use rand.IntN instead of rand.Intn (v2 naming convention)
- Replace rand.NewSource with rand.NewPCG for local random generators
- Remove all rand.Seed calls (v2 auto-seeds the global source)
- Fix ConnOpenRandom strategy: pick random offset once outside the
  loop instead of on every iteration, ensuring all addresses are
  tried before failing over
- Re-enable TestConnFailoverRandom and TestStdConnFailoverRandom
  tests that were skipped due to Go 1.25 math/random changes

Fixes ClickHouse#1653
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 7, 2026

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

Summary

This PR migrates the codebase from math/rand (v1) to math/rand/v2 across 19 files, removes deprecated rand.Seed calls (v2 auto-seeds), renames API calls (Intn to IntN, Int63() to Int64()), and fixes the ConnOpenRandom strategy to pick a random starting offset once before the loop — ensuring all addresses are tried exactly once per dial attempt. The two failover tests that were previously skipped due to the Go 1.25 rand.Seed breakage are re-enabled. The migration is mechanically correct and the failover bug fix is the right approach.

Should fix

DefaultDialStrategy can panic on empty opt.Addr (clickhouse.go:286)

The PR hoists random := rand.IntN(len(opt.Addr)) unconditionally before the loop. If opt.Addr is empty, rand.IntN(0) panics. In the current code, rand.Int() was only called inside case ConnOpenRandom:, which is unreachable when the loop never iterates, so empty addrs were handled gracefully (loop is a no-op, caller gets an error). This converts a safe no-op into a library panic — which the project rules prohibit outside init().

The clickhouse_std.go counterpart is protected by its len(addrs) == 0 / ErrAcquireConnNoAddress guard that runs before the rand call. DefaultDialStrategy has no equivalent guard.

Fix: either add a len(opt.Addr) == 0 early return before the rand call, or compute random lazily only when the strategy is ConnOpenRandom.

Nits

  • nit: rand.NewPCG(uint64(time.Now().UnixNano()), uint64(time.Now().UnixNano()>>32)) calls time.Now() twice; the two nanosecond values may differ, making the seed non-reproducible for debugging. Capture once: now := time.Now().UnixNano(); rand.NewPCG(uint64(now), uint64(now>>32)). Affects tests/utils.go and tests/issues/1271_test.go.

  • nit: The comment "rng.Int64() generates 63 random bits" (and the identical one in 1271_test.go) is carried over from v1 but is no longer accurate — (*rand.Rand).Int64() in v2 returns the full int64 range (64 bits, including the sign bit). The constant letterIdxMax = 63 / letterIdxBits is still safe since 10x6 = 60 < 64, but the comment misleads readers.

  • nit: random := rand.IntN(len(opt.Addr)) is evaluated for every connection attempt regardless of the configured strategy. For ConnOpenInOrder and ConnOpenRoundRobin the value is computed and immediately discarded. Minor, but worth scoping to the ConnOpenRandom case once the panic issue above is addressed anyway.

Verdict

Request changes

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.

Upgrade to math/rand/v2

3 participants