support arbitrary string parallel snapshotting by jgao54 · Pull Request #4522 · PeerDB-io/peerdb

jgao54 · 2026-06-30T22:27:32Z

Add parallel snapshotting support for arbitrary string using an adaptive bisection strategy.

The partitions are determined by starting with a single partition using min/max fetched values; and then continuously bisecting the largest partition into 2 smaller partitions, until we reached the target number of partitions or until they are no longer splittable.

To determine largest partition, we leverage mysql's query planner's estimated row count for a given range.
To determine the midpoint, we encode the first 8 characters (after shared prefix) as a base-95 integer (one digit per character over printable ASCII); bytes outside of the range get rounded to the nearest edge.
To ensure correctness in coverage, we always fetch the next real key after the interpolated midpoint to ensure server-side collation is used.

There are known edge cases that can lead to skewed partitions (see unit tests), but never compromises on correctness. We sacrifice some balancing optimization for a more simple, deterministic partitioning algorithm.

Fixes: DBI-885

codecov · 2026-06-30T22:48:04Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
2996	1	2995	348

View the top 1 failed test(s) by shortest run time

github.com/PeerDB-io/peerdb/flow/connectors/mysql::TestBuildAdaptiveStringPartitions_MixedCase_UUID

Stack Traces | 0s run time

=== RUN   TestBuildAdaptiveStringPartitions_MixedCase_UUID
2026/07/08 03:30:03 INFO [mysql] built adaptive string partitions targetNumPartitions=5 numPartitions=3
    qrep_partition_test.go:416: 
        	Error Trace:	.../connectors/mysql/qrep_partition_test.go:416
        	Error:      	"[partition_id:"2652d323-941f-4da6-8668-7e9ca504ec9e"  range:{string_range:{start:"4EC56E71-7A7D-11F1-A984-0022487DE218"  end:"4ec56e86-7a7d-11f1-a984-0022487de218"}} partition_id:"411e45aa-32cd-4ddb-9a3a-1a3cab46ab2a"  range:{string_range:{start:"4ec56e86-7a7d-11f1-a984-0022487de218"  end:"4ec57003-7a7d-11f1-a984-0022487de218"}} partition_id:"727993fb-d61d-467c-9ce7-20b8400c3559"  range:{string_range:{start:"4ec57003-7a7d-11f1-a984-0022487de218"  end:"4ec5700b-7a7d-11f1-a984-0022487de218"  end_inclusive:true}}]" should have 5 item(s), but has 3
        	Test:       	TestBuildAdaptiveStringPartitions_MixedCase_UUID
--- FAIL: TestBuildAdaptiveStringPartitions_MixedCase_UUID (0.00s)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

github-actions · 2026-06-30T22:50:58Z

❌ Test Failure

Analysis: The PR's own new feature test (Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot) fails deterministically across all three relevant suites with a value-correctness mismatch (expected 8, got 1), indicating a real bug in the arbitrary-string parallel-snapshot logic rather than a flake.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-06-30T22:54:20Z

❌ Test Failure

Analysis: A real bug: the new Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot test (the exact feature this branch implements) fails deterministically across two suite variants with an identical partition-count assertion mismatch (expected 8, got 1), indicating the arbitrary-string-key parallel snapshot isn't splitting into multiple partitions — no timeout, race, or network signature.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-06-30T23:36:26Z

❌ Test Failure

Analysis: The new Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot test added by this PR fails deterministically across all four CH suite variants with a consistent partition-key string value mismatch, indicating a real logic bug in the PR's arbitrary-string parallel snapshot feature rather than a flaky/timeout/race failure.
Confidence: 0.93

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-07-01T00:48:45Z

❌ Test Failure

Analysis: The new test Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot (matching the PR's own feature branch) fails deterministically with assertion mismatches on partition/row counts across all three suites and matrix jobs, indicating a real bug in the PR's new feature rather than flakiness.
Confidence: 0.97

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

github-actions · 2026-07-01T02:10:07Z

❌ Test Failure

Analysis: The PR's own new test Test_MySQL_String_Partition_Key_Arbitrary_Parallel_Snapshot fails deterministically with partition/row-count assertion mismatches across every suite and compatibility matrix, indicating a real bug in the arbitrary-string-parallel-snapshot feature rather than flakiness.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

claude · 2026-07-07T02:45:07Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

jgao54 · 2026-07-08T03:24:04Z

+	ctx context.Context, tableName string, quotedCol string, start string, end string,
+) (uint64, error) {
+	query := fmt.Sprintf(
+		"EXPLAIN FORMAT=TRADITIONAL SELECT 1 FROM %[1]s WHERE %[2]s >= '%[3]s' AND %[2]s < '%[4]s'",


without FORMAT=TRADITIONAL, on newer version of mysql it will return:

mysql> explain select 1 from str_test where id > 'a' and id <= 'z'; +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | EXPLAIN | +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | -> Filter: ((str_test.id > 'a') and (str_test.id <= 'z')) (cost=18.9 rows=93) -> Covering index range scan on str_test using PRIMARY over ('a' < id <= 'z') (cost=18.9 rows=93) | +----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

what we want:

mysql> explain format=traditional select 1 from str_test where id > 'a' and id <= 'z'; +----+-------------+----------+------------+-------+---------------+---------+---------+------+------+----------+--------------------------+ | id | select_type | table | partitions | type | possible_keys | key | key_len | ref | rows | filtered | Extra | +----+-------------+----------+------------+-------+---------------+---------+---------+------+------+----------+--------------------------+ | 1 | SIMPLE | str_test | NULL | range | PRIMARY | PRIMARY | 202 | NULL | 93 | 100.00 | Using where; Using index | +----+-------------+----------+------------+-------+---------------+---------+---------+------+------+----------+--------------------------+

github-actions · 2026-07-08T03:50:59Z

❌ Test Failure

Analysis: A deterministic, pure unit test (TestBuildAdaptiveStringPartitions_MixedCase_UUID, 0.00s, no network/concurrency) newly added by this PR fails because adaptive string partitioning produced 3 partitions instead of the expected 5 for mixed-case UUIDs — a real logic bug in the PR's own feature, not flakiness.
Confidence: 0.95

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

if fetchNextRealKey does not find a valid key; try fetchPrevRealKey before deeming the partition as unsplittable. This is useful for narrow ranges

jgao54 · 2026-07-08T06:31:20Z

The last commit 7e97b79 also fixes the flaky test scenario TestBuildAdaptiveStringPartitions_MixedCase_UUID reported above, which happens occasionally if the generated UUIDs have a midpoint that does not return a valid fetchNextRealKey; but would return a valid fetchPrevRealKey.

ilidemi

A bunch of Fable finds

ultracode an adversarial code review of the changes on this branch, especially the string wrangling logic. report the findings here

ilidemi · 2026-07-09T05:54:19Z

+			stringPartitions, err := buildAdaptiveStringPartitions(
+				ctx, c, c.logger, parsedWatermarkTable, config.WatermarkColumn, start, end, numPartitions)
+			if err != nil {
+				c.logger.Warn("failed to build adaptive string partitions, falling back to full table partition",


We should still be deterministic in face of network errors.
Haven't we had this conversation before
#4097 (comment)
#3972 (comment)

This masks MariaDB rejecting EXPLAIN FORMAT=TRADITIONAL

surprised that tests that expect N partitions did not catch this, will take a look what happened here.

My bad on Maria, gotta keep my AIs in check

ilidemi · 2026-07-09T06:50:52Z

+
+	partitions := make([]*protos.QRepPartition, 0, len(outputs))
+	for _, p := range outputs {
+		partitions = append(partitions, utils.CreateStringPartition(p.start, p.end, p.end == maxVal))


🟠 Major — NUL byte in a boundary key → deterministic stuck snapshot

MySQL VARCHAR permits 0x00; mysql.Escape maps it to a valid literal so probes succeed and the NUL key lands in a StringRange boundary. Postgres then rejects it: the catalog insert of partition_start/partition_end (text columns, monitoring.go:393) fails with invalid byte sequence for encoding "UTF8": 0x00 (reproduced live against the dev catalog). GetQRepPartitions fails, Temporal recomputes identical partitions, fails identically — stuck forever. Bounded by defaults: with PEERDB_OFFLOAD_PARTITION_RANGES=true (default) and InitialCopyOnly snapshots, ranges are offloaded to encrypted bytea and this insert is skipped. Trigger requires offload disabled or a continuous mirror, plus a NUL in the min/max key.

ilidemi · 2026-07-09T07:27:40Z

+	for len(seen) < numRows {
+		keyBytes := make([]byte, 10)
+		for i := range keyBytes {
+			keyBytes[i] = byte('a' + rand.IntN(26)) //nolint:gosec // test data


I think this needs a fuzz integration test (quicker than e2e but real mysql/mariadb) and leave it overnight/weekend. LLMs can help with throwing in charsets/collations/index types (idk if relevant for PK)/padding and setting up the data generation.

ilidemi

huh forgot to post

ilidemi · 2026-07-09T07:39:10Z

+) (string, bool, error) {
+	query := fmt.Sprintf(
+		"SELECT %[1]s FROM %[2]s WHERE %[1]s >= '%[3]s' AND %[1]s > '%[4]s' AND %[1]s < '%[5]s' ORDER BY %[1]s LIMIT 1",
+		quotedCol, tableName, mysql.Escape(midpoint), mysql.Escape(start), mysql.Escape(end))


🔴 Critical — Watermark values interpolated with the wrong escaping for the session's sql_mode

flow/connectors/mysql/qrep_partition.go:281 (and :294, :325, plus the pull query at flow/connectors/mysql/qrep.go:379)

Every new probe query and the StringRange pull query build SQL literals with mysql.Escape(...) wrapped in single quotes. But the connector unconditionally runs SET sql_mode = 'ANSI,NO_BACKSLASH_ESCAPES' (mysql.go:237). mysql.Escape does backslash escaping (' → \'), which under NO_BACKSLASH_ESCAPES is a no-op — the backslash is a literal character. This was masked at merge-base because non-UUID strings fell back to full-table and emitted no per-value SQL; UUID boundaries are hex-only. This branch routes arbitrary row data through these literals for the first time. Three concrete failure modes, reproduced live on MySQL 8.0:

Silent data loss (this is the critical core). A max watermark value like Z:\tmp (Windows/UNC path as key) is stored unescaped, so partitioning succeeds — no fallback. The last partition's inclusive bound becomes BETWEEN ... AND 'Z:\\tmp', which under NO_BACKSLASH_ESCAPES is the literal 7-char string Z:\\tmp. The real Z:\tmp sorts above it, so the max row (and neighbors) are silently never replicated. Verified: SELECT ... BETWEEN 'Mmm' AND 'Z:\\tmp' returned only Mmm, dropping the real max. Symmetric at the min bound with \n/\t/\0. Interior boundaries stay consistent (adjacent partitions share the identical escaped key), so the leak is concentrated at the two endpoints — but it's a successful query with no error and no fallback.

SQL injection. A source row value like x' OR '1'='1 escapes to 'x\' OR '1'='1'; the backslash is literal, the string closes early, and the tail executes. Watermark values are attacker-influenceable table data. Reachable but harder — quote-bearing values usually syntax-error the probe phase first (see below).

Quote-free data breaks too. stringMidpoint synthesizes bytes in [0x20,0x7E] as base95 digits — including 0x27 (') and 0x5C (\). Verified: stringMidpoint("&&&", "(((") = "'''", invented from quote-free input. The probe then throws ERROR 1064 and the build silently degrades to a single full-table partition.

Fix: use parameterized Execute(query, args...) (already supported at mysql.go:303) or '' quote-doubling instead of mysql.Escape. This fixes all three at once.

jgao54 force-pushed the arbitrary-string-parallel-snapshot branch from bd316b6 to 3c9cf7d Compare June 30, 2026 22:29

jgao54 force-pushed the arbitrary-string-parallel-snapshot branch from 3c9cf7d to 87683c6 Compare June 30, 2026 23:09

jgao54 force-pushed the arbitrary-string-parallel-snapshot branch from 87683c6 to 4b4de52 Compare July 1, 2026 00:25

jgao54 force-pushed the arbitrary-string-parallel-snapshot branch from 4b4de52 to 2817e2e Compare July 1, 2026 01:47

jgao54 force-pushed the arbitrary-string-parallel-snapshot branch from 2817e2e to 29d182c Compare July 7, 2026 00:52

jgao54 marked this pull request as ready for review July 7, 2026 01:59

jgao54 requested a review from a team as a code owner July 7, 2026 01:59

support arbitrary string parallel snapshotting

83aeb72

jgao54 force-pushed the arbitrary-string-parallel-snapshot branch from 29d182c to 83aeb72 Compare July 8, 2026 02:03

jgao54 commented Jul 8, 2026

View reviewed changes

remove dupe code

6d512e2

partitioning improvement

7e97b79

if fetchNextRealKey does not find a valid key; try fetchPrevRealKey before deeming the partition as unsplittable. This is useful for narrow ranges

ilidemi reviewed Jul 9, 2026

View reviewed changes

Uh oh!

Conversation

jgao54 commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 1 Tests Failed:

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jun 30, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jul 1, 2026

❌ Test Failure

Uh oh!

github-actions Bot commented Jul 1, 2026

❌ Test Failure

Uh oh!

claude Bot commented Jul 7, 2026

Code review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 8, 2026

❌ Test Failure

Uh oh!

jgao54 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilidemi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilidemi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jgao54 commented Jun 30, 2026 •

edited

Loading

codecov Bot commented Jun 30, 2026 •

edited

Loading

jgao54 commented Jul 8, 2026 •

edited

Loading

ilidemi left a comment •

edited

Loading