Repository navigation
fix(disttae): retain read barriers through logtail reconnects - #30109
Conversation
Fixes matrixorigin#30123. Keep the barrier bound to its caller and PushClient owner; obtain a fresh TN frontier after replacing a cancelled transport.
matrix-meow
added
size/M
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #29849 ## What this PR does / why we need it: Airbyte's MySQL destination can emit `LOAD DATA` with a row variable in the input list and `SET target = NULLIF(@user_variable, ...)`. MatrixOne rejected that form because the planner only accepted a column name as the first `NULLIF` argument, so Airbyte could not load these rows. This change adds nullable VARCHAR scan fields for referenced row variables, binds `NULLIF` against the raw field before casting to the target type, and removes temporary assignment AST state before plan serialization. The legacy LOAD path binds SET expressions before resolving defaults and treats SET targets as supplied values while preserving the file-column index map. Existing column-name `NULLIF` behavior and SET assignments to other columns remain supported. ## Validation - On the pre-sync Issue source, `.agents/skills/mo-dev/scripts/mo-cgo-test -run '^TestIssue29849LoadDataSetNullIfUserVariable' -count=1 ./pkg/sql/plan` passed; `.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 ./pkg/sql/plan` passed (13.295s). - On candidate source tree `8b3f822c25a5e72cd8aecf4adf5d92e62a16a661` (same eight Issue-file blobs, based on upstream main `35c8dd5854750ef96f52f9dd298a70acd031d5e4`), `make cgo NATIVE_BUILD_JOBS=4` passed and `.agents/skills/mo-dev/scripts/mo-cgo-test -run '^TestIssue29849LoadDataSetNullIfUserVariable' -count=1 ./pkg/sql/plan` passed (3.827s). - The candidate's focused base compile check, `.agents/skills/mo-dev/scripts/mo-cgo-test -run '^$' ./pkg/sql/plan/explain`, fails on unrelated current-main EXPLAIN code: `sql_json.go` references removed `Node_VECTOR_INDEX_SCAN` / `VectorIndexScan` types. This is tracked by [#30108](#30108) and its open repair [#30109](#30109). The candidate does not modify those files and is not published. - Local SQL BVT on the rebuilt pre-sync Issue binary passed twice: 9/9 statements each, 0 failed/ignored/abnormal. It covers unknown-variable rejection, column-name `NULLIF`, row-variable `NULLIF` for NULL/ordinary/empty/\N values, JSON conversion, and cleanup. - Existing control BVT `load_data_set_null.sql` passed: 38/38 statements, 0 failed/ignored/abnormal. - The local `make build` and those BVTs were run on the pre-sync Issue source, before the current-main EXPLAIN regression. The EXPLAIN failure prevents claiming a complete service build for current main. The official Airbyte connector suite was not run. ## Current published CI The published PR head is `2e7ab5dded1103ed00d51f672ca54c0ad4b0eaa0`; the GitHub base is `21f19ae06305ca2acd253d15123f851899d6b6a1`. The final tree contains the eight Issue #29849 files and one narrowly scoped logservice test-fixture compatibility change needed to compile the existing test package. - On the previous head `6ab88d9b26b0a4ae0b2e87173d55b2a7b3f8810c`, run [38358986817](https://github.com/matrixorigin/matrixone/actions/runs/38358986817) attempt 1 failed SCA and UT Coverage because `pkg/logservice/gossip_fixture_test.go:130:31` called `randomPorts.allocate` with one callback while its signature requires `(int, int, func(int) error)`. This is the base compile mismatch tracked by [#30047](#30047). After the user-authorized failed-only retry, attempt 2's SCA and UT Coverage failed at fork checkout with GnuTLS termination; these were infrastructure failures matching [#28113](#28113) and [#28411](#28411). That old-head run was superseded when this new head was pushed; no manual cancellation was issued. - This PR's one-line fixture repair calls `testPortRange()` and passes `(first, limit, probeTestPort)` to the allocator. The changed fixture file compiled in a package closure matching the PR's other 51 `pkg/logservice` blobs. Command: `.agents/skills/mo-dev/scripts/mo-cgo-test -tags matrixone_test -json -count=1 -run '^Test(TestPortAllocation|TestPortRangeOutsideEphemeral|GossipFixtureConfigAllocationFailure|GossipFixtureStartupConfigurationFailure)$' ./pkg/logservice`. Result: exit 0; all four named tests and their subtests passed (1.509s). This package-scoped check used the local native-ready worktree; it is not a full repository build on the PR's base. - Current-head run [38388197404](https://github.com/matrixorigin/matrixone/actions/runs/38388197404), attempt 1 on `2e7ab5dded1103ed00d51f672ca54c0ad4b0eaa0`, completed with one substantive failure: Ubuntu/x86 UT job [115265250434](https://github.com/matrixorigin/matrixone/actions/runs/38388197404/job/115265250434) could not fetch the fork. Three fetch attempts failed with `gnutls_handshake() failed: The TLS connection was non-properly terminated` (exit 128); no Go compilation or unit tests ran. Docker-daemon messages in the optional disk-cleanup step were ignored by `|| true`; missing scratch/report files followed the failed checkout. Preflight, PR-size label, coverage eligibility, Shared Build, SCA, UT Coverage, Coverage, and both BVTs passed. This is the known checkout-infrastructure failure class tracked by [#28113](#28113) and [#28411](#28411). Following the user's retry instruction, one failed-only retry was accepted. Attempt 2's Ubuntu/x86 UT job `115292620705` and aggregate `CI Required` job `115332368781` both passed; run `38388197404` completed successfully on this head. No further retry was needed. Earlier LOAD DATA planner, SQL BVT, and control BVT results are listed above. The official Airbyte connector suite was not run. ## Base and review status The PR remains based on `21f19ae06305ca2acd253d15123f851899d6b6a1`, and GitHub reports it behind current upstream main. A local independent `gpt-6.1-sol / xhigh` review returned GO on exact head `2e7ab5dded1103ed00d51f672ca54c0ad4b0eaa0`: it verified the one-file test-fixture signature fix, parent, and tree, with no findings. GitHub's formal review endpoint is still empty and review is required. The Issue fix preserves the original eight-file behavior. The additional logservice change is test-only and repairs the exact allocator call that prevented SCA and UT Coverage from compiling the package. No unrelated runtime or EXPLAIN changes are included. CLAassistant currently reports 1 of 2 commit authors signed: `Lundomn` is signed. The original commit carries the generated author label `Codex issue-29849`, which is not a linked GitHub account and has no separate human contributor who can sign. A reviewed history-only candidate changes that identity to `Lundomn` while preserving this PR's exact code tree; it needs a non-fast-forward branch update and remains unpublished pending authorization. ## Compatibility, performance, and remaining validation No SQL syntax or serialized-plan format changes are introduced. Each distinct referenced row variable adds a nullable VARCHAR scan field and projection, adding input width proportional to the referenced variables. No performance benchmark was run. The existing column-name `NULLIF` path remains covered by the control BVT. The full distributed LOAD DATA group, race tests, vet, benchmarks, and official Airbyte connector suite were not run. Current-main service build and its dependent BVT remain blocked by #30108; the focused Issue planner test passes on the candidate tree. Final published PR head SHA: `2e7ab5dded1103ed00d51f672ca54c0ad4b0eaa0` --------- Co-authored-by: Codex issue-29849 <codex-issue-29849@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
|
mo-bot review · Commit: Assessment: approve Reviewed the complete two-file delta, reconnect and shutdown ownership, cancellation, apply waits, and SQL callers. The fix reuses existing lifecycle mechanisms and retries only retired transport failures. No actionable findings remain. Model: Session: Validation:
Limitations:
|
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: f1ebe024662b3c3e2447c5544310bbbaa74d7817
Assessment: approve
Reviewed the complete two-file delta, reconnect and shutdown ownership, cancellation, apply waits, and SQL callers. The fix reuses existing lifecycle mechanisms and retries only retired transport failures. No actionable findings remain.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
Validation:
-
Verified frozen head, patch, clean source, native provenance and successful isolation preflight.
-
17 focused barrier tests passed. Complete disttae and logtail/service race suites passed: 622 top-level tests, no skips.
-
Each of the four new tests passed 100 individually selected race repetitions.
-
The reconnect regression failed against exact merge-base production code with the expected cancellation error, confirming regression sensitivity.
-
Commands, source fingerprints, exit receipts and logs reconciled in /home/xupeng/.local/state/mo-bot/jobs/d4a63884e58bee8da645d0e2/attempt-1/validation-summary.json; evidence-audit.log records the successful audit.
-
Frozen exact-head CI metadata records successful build, full x86 UT, SCA, coverage and PROXY/PESSIMISTIC BVT jobs.
Limitations:
-
No local SQL-cluster rerun. Broader integration coverage relies on frozen CI metadata; author-reported amplified-fault counts were not independently verified from raw logs.
-
Issue #30123's contract was assessed from the frozen PR description; its separate issue body was not supplied.
Merge Queue Status
This pull request spent 2 minutes 45 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
This branch was successfully deployed
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
Closes #30123
What this PR does / why we need it:
An injected logtail receive error during
ddl_atomicity.sqlreplaced the transport while DROP DATABASE obtained its read barrier. The retired stream's private cancellation escaped to the still-live SQL request. DROP DATABASE returnedcontext canceled, left the database present and caused three subsequent BVT failures.Keep the read bounded by both its caller and its existing PushClient owner. Retry a read-only barrier only after its failed transport has been replaced, while its request remains live. Wait for the replacement's ready generation, obtain a fresh TN frontier, then use the existing apply waiter. Caller cancellation, deadlines, owner shutdown, an unchanged failed client and non-transport errors terminate the operation. The existing transaction ownership and original BVT fault probability/golden remain intact.
The delivery changes two files: read-barrier handling and its deterministic regression tests. It is integrated with main
f14a1fa9ee, which contains the separately merged EXPLAIN repair #30111 related to #30108.Validation
live caller abandoned during reconnect: context canceled.Remote CI
MatrixOne ALL CI run 38398293278, attempt 2 is successful on head
f1ebe024662b3c3e2447c5544310bbbaa74d7817. Full x86 unit tests, SCA (license/format/static checks), shared native build, both PROXY/PESSIMISTIC Compose BVT groups, UT coverage, coverage aggregation andCI Requiredall passed. Changed-code coverage is 16/19 lines (84.21%), above the required 75% threshold; merged overall coverage is 79.88%.The first attempt's remaining failure was UT coverage checkout: three GitHub TLS connection failures before tests executed. Coverage aggregation consequently had no complete producer profile and calculated no verdict. Rerunning only failed jobs preserved the successful full unit suite, SCA, build and both BVT groups; the coverage producer, aggregation and final required check then passed. This PR remains a draft.