fix: honor div_precision_increment in decimal division - #29241
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
89688a2 to
f1a1890
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — P2 false overflow for valid Decimal256 division in pkg/sql/plan/function/arith_decimal_fast.go around lines 3297–3298.
Reproducer:
SET SESSION div_precision_increment = 30;
SELECT CAST(100000000000000000000 AS DECIMAL(38,0))
/ CAST(1 AS DECIMAL(38,30));Both operands fit Decimal128, so the Decimal256 path dispatches to d256DivViaD128. Scaling the numerator for the requested result scale creates an intermediate 10^80 and reports overflow, even though the correctly rounded result coefficient is only 10^50 and fits the inferred DECIMAL(65,30). The new d256DivBig fallback is bypassed on this all-fit-D128 route. Please route this fast-path scaling failure through the big-integer fallback and add the regression. Source tracing confirms the path; SQL repro not run locally. Exact-head CI passes, including the 38/38 decimal BVT.
f1a1890 to
3ddde88
Compare
|
Addressed the Decimal256 false-overflow review in 3ddde88. The all-fit-D128 path now preserves the unscaled numerator and falls back to d256DivBig when fixed-width scaling overflows. Added focused positive/negative/true-overflow coverage plus the exact SQL regression; the BVT passes 39/39 twice. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of 3903e9cbf5711315b58818d389af86d72ac6430d against base/merge-base 71c415705c909e524abd21dd0cf3e31b1c18e941: REQUEST_CHANGES — one verified P2 in CTAS inherited default semantics (inline).
Reviewed all 65 changed files across session/cache/prepared execution, decimal typing and kernels, DDL persistence/replay, protocol compatibility, tests and goldens. Reused semantically valid local evidence: full plan/compile UT, go vet, SCA (0 issues), final service build, and strict metadata #28594 BVT 117/117 twice. Additional exact-head SQL probes passed nullability changes, row-reference defaults across COPY ALTER, generated-column type changes, and large direct/prepared arithmetic; CTAS default precision drift was reproduced with both populated and empty sources.
No additional confirmed leak, double cleanup, hang or unbounded retention found in the changed closures. DDL replay allocations are statement-scoped and proportional to schema/expression size; capability probes retain the existing cancellation/deadline and response-release rules. Existing temporal and scale-cap performance results remain applicable, but do not establish universal non-regression or high-fanout RPC latency. No broader replay framework is needed for the proposed fix.
New-head CI is still running; this review does not treat pending checks as passes.
| } | ||
|
|
||
| binder := NewDefaultBinder(ctx.GetContext(), nil, nil, typ, nil) | ||
| bindCtx := ddlExpressionContext(ctx, ctx.GetContext()) |
There was a problem hiding this comment.
[P2] Preserve inherited CTAS defaults when their referenced types are unchanged
finalizeCTASDefaults calls this binder for every default containing a local column reference. With the new session-aware context, a plain CTAS now silently changes an inherited default when the creating session uses a different div_precision_increment, even though no column type or expression was changed.
Reproduced on this head:
set session div_precision_increment=10;
create table src(a decimal(10,2), b decimal(10,2),
q decimal(30,12) default(a/b));
set session div_precision_increment=4;
create table dst as select * from src;
insert into src(a,b) values(1,3);
insert into dst(a,b) values(1,3);
select q from src; -- 0.333333333333
select q from dst; -- 0.333333000000The target should retain the inherited source default's bound scale when the operand and assignment types are unchanged. Constant defaults in the same CTAS already retain their source value, while row-dependent defaults are unconditionally rebound here. This leaves the same persisted-expression semantic drift that the new COPY ALTER/LIKE handling fixes.
Retain/remap the existing bound expression for compatible schemas, and rebind only when a real type override requires it. Extend the existing CTAS default rebind test/BVT with a 10-to-4 session switch, unchanged/renamed/reordered columns, and the existing changed-type control.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Recorded the exception/case inventory in 2243f42.
- 72 numbered entries: 45 existing coverage/evidence entries, 17 executable CTAS probes, 10 explicit coverage gaps. Each entry identifies its expected behavior, evidence and status.
- Ran the 17 CTAS probes twice against the unchanged 3903e9 production binary: identical 8 passed / 9 failed. All nine failures are variants of the previously reported inherited-default rebinding defect, including aliases, reordered/prepended columns, repeated CTAS and the reverse precision change. Actual output is committed alongside the probes.
- The runner checks every case ID and expected value, exits nonzero on incorrect results, and cleans up its unique database. No probe databases or task service remained after validation.
- These are manual diagnostic cases outside automatic CI selection. No production fix is included in this inventory commit, and no incorrect output was accepted as a golden. The previously reported P2 remains unresolved.
Once the CTAS fix is made, promote the distinct preservation/type-override cases to the existing planner UT and expression-default BVT. The inventory explicitly retains mixed-binary, restart, failure-publication and fanout-performance gaps rather than treating mock or local evidence as those results.
There was a problem hiding this comment.
REQUEST_CHANGES: one confirmed P2 remains at cb7025e3a31703896572bbd7f5eac752a50fd573.
The latest CTAS repair closes the previous inherited-default finding: unchanged bindings survive setting changes, while actual type overrides rebind; source metadata, dependency validation, nullability and protocol admission are covered. The remaining failure is SQL schema export/reconstruction and DUMP/LOAD.
P2 — Preserve each expression's bound precision through dump/load. The new replay context exists only inside COPY ALTER/LIKE. SHOW CREATE and the dump manifest still emit DEFAULT (a / b) / GENERATED ALWAYS AS ((a / b)) without the original binding. Reconstructing that schema in a default session rebinds it with increment 4, and LOAD accepts the table because its schema hash uses the same SQL text.
Reproduced on the final rebuilt service with this exact production source:
set div_precision_increment=10;
create table src(a decimal(10,2), b decimal(10,2),
q decimal(30,12) default(a/b));
set div_precision_increment=4;
alter table src add column r decimal(30,12)
generated always as (a/b) stored;
insert into src(a,b) values(1,3);
-- src q=0.333333333333, r=0.333333000000
-- Flush src, DUMP TABLE src ... METADATA ONLY, recreate dst from
-- SHOW CREATE TABLE src (rename only), then LOAD TABLE dst ...
insert into dst(a,b) values(1,3);The restored table contains the loaded row with q=0.333333333333 and a new row with identical (a,b) but q=0.333333000000. r is 0.333333000000 in both rows. LOAD succeeds. A service restart is a successful control: a fresh session inserting (2,3) into the original source still gets q=0.666666666667 and r=0.666667000000.
Preserve the bound semantics in exported/imported schema and include them in compatibility validation. One table can contain expressions authored with different increments, so adding a single session SET to a dump is insufficient. Extend the existing table_dump_load.sql regression with loaded rows and future inserts, plus this mixed-binding control.
Review scope: all 70 changed files, merge-base 71c415705c909e524abd21dd0cf3e31b1c18e941; freshly fetched main f4cc6bfbf78763de48b3aab2cb70a0555987ec49. Worktree stayed clean. Ordinary bug/compatibility fix; existing design decisions retained. Reviewed session/cache/prepared/retry, arithmetic/type/physical layout, DDL/catalog replay, mixed-CN placement/sending/receiving, test oracles and result files.
Validation reused where relevant inputs are unchanged: full planner UT, go vet, incremental lint (0 issues); strict #28594 BVT 140/140 twice and 19/19 manual CTAS probes twice. Older function/protobuf/compile evidence remains applicable to those unchanged closures. New live evidence is the dump/load failure and the successful restart control; test database was removed. Remote CI is still running.
| Unhappy-path audit | Result |
|---|---|
| Ownership | Capability responses/Futures have release owners; cache generations release displaced compiles; DDL contexts restore with defer. No new confirmed leak/double cleanup. |
| Wait termination | Capability probing uses caller cancellation and a shared 5s deadline, through bounded transport writes and Future completion. No new confirmed deadlock. |
| Memory/work bounds | Decimal vectors scale with the batch; valid SQL scale bounds keep wide fallback finite; replay maps/clones live for the statement. No new confirmed unbounded retention. |
Performance: unchanged 1,024-row temporal evidence is 373–393 us and 32,784 retained bytes; scale-cap kernels measured 0 allocations. The CTAS change adds planning-time expression traversal, not per-row work. These measurements do not establish universal nonregression: Decimal256 promotion and uncached per-destination capability RPCs have costs, and real mixed-version/high-fanout validation remains absent. Consolidating capability checks within one planning/send operation is a direction if fanout measurements show a problem; no additional framework is needed for the verified persistence fix.
Follow-up verification on the same head
The confirmed PR finding remains the P2 above. This is not a claim that all remaining behavior is defect-free.
New targeted evidence:
- 23/23 live SQL assertions passed. Two dedicated connections alternated increments 0/30/10/4 with identical explicit-DECIMAL SQL. Direct query, SQL PREPARE/EXECUTE and COM_STMT_EXECUTE agreed in value, type, precision and scale. Binary integer/float/string/NULL parameter transitions through explicit DECIMAL casts passed; reconnect restored the global default without changing the other session.
- CTAS with a missing DEFAULT dependency rejected without a target table. COPY ALTER generated-column overflow rejected with the original rows/schema intact and no replacement table visible. A subsequent CREATE used the current setting; inserting into the source still used its stored binding.
- The focused frontend overlay passed all four precision transitions with column-metadata failure injection and successful retry. Old plan/metadata/settings stayed together until publication succeeded.
- A second frontend overlay rejected invalid parameter count after publishing the rebuilt plan, marked that generation dirty, and successfully rebuilt it on the next valid execution at scale 30. This covers post-commit parameter failure, not an injected physical Compile failure.
- The compile overlay canceled an in-flight capability request, checked the shared deadline and exactly one response release, then successfully probed again with a fresh context. This is a synchronized mock-client test, not a real network partition test.
The diagnostic code was kept outside the clean PR worktree. The test database was dropped and absence verified. During subsequent SIGTERM cleanup, the service exited 2: final CN metadata-withdrawal heartbeat exceeded its 3-second deadline, propagated through Close, and panicked in main. Ordinary heartbeat timeouts were also logged before shutdown. These shutdown/heartbeat sources are unchanged from the PR merge-base; causation by this PR is not established. The SQL assertions passed, but this run is not evidence of a clean service shutdown.
Remaining limits: real protocol-96/97 topology, uncast nested prepared-parameter/public-metadata contract, physical compile-failure injection, and matched baseline performance for Decimal256 promotion and many capability destinations. These are evidence gaps, not additional proven defects. Current CI has no failed checks, but UT/SCA/coverage and two BVT jobs are still pending. REQUEST_CHANGES remains unchanged.
| // WithPersistedDDLReplay scopes expression preservation to one reconstructed | ||
| // CREATE, used by COPY ALTER and CREATE TABLE LIKE. It does not change the | ||
| // ordinary CREATE path. | ||
| func WithPersistedDDLReplay(ctx context.Context, original, target *planpb.TableDef) context.Context { |
There was a problem hiding this comment.
[P2] Preserve bound precision in exported/restored schemas too
This context only protects in-process COPY ALTER/LIKE. At this head, create q DEFAULT(a/b) under increment 10, add r GENERATED ALWAYS AS(a/b) under increment 4, then DUMP and recreate the table from SHOW CREATE before LOAD. LOAD succeeds, but the loaded (1,3) row has q=0.333333333333 while a subsequent identical insert has q=0.333333000000. Restarting the original table preserves its values, isolating the loss to SQL reconstruction. The exporter emits identical bare division text and the dump schema hash cannot distinguish the bindings. Carry each expression's bound semantics through export/import and validate them; one session SET cannot restore q and r because they were authored with different increments.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review — REQUEST_CHANGES
Reviewed full head 08495b29d837bebfb11c7c366dec1611fd0b3347 against base 3f0a68bd8034d37e098765920f328985c308c9ff (77 changed files), including arithmetic/types, session and prepared plans, internal replay, DDL persistence/protocol, DUMP/LOAD validation and rollback, tests/results, and the approved revision-10 design.
Four confirmed findings are attached inline. The current change still breaks existing internal-executor callers, rejects an ordinary generated-column layout, incurs quadratic CHECK-validation allocation, and fails required SCA. The preceding implementation PASS claim is superseded by this evidence.
Evidence
- Exact-head CI: both multi-CN BVT jobs pass; SCA, Ubuntu UT and coverage UT fail. The Ubuntu UT log contains 27 test failures with
variable foreign_key_checks not supported, traced to the new process-resolver delegation and the existing partial resolver. - New focused white-box diagnostic: bind ordinary CREATE at increment10, then call the exact helper used by DUMP,
AnalyzeTableDumpBindings(ctx, def, def). Withq generated as(a/b)first/middle/last, CREATE succeeds in all cases; DUMP analysis fails for first/middle and passes for last (diagnostic exits1). - New allocation diagnostic: build a two-column table with 1/10/100 named
CHECK(a/b > 0)clauses, then measure onlyAnalyzeTableDumpBindingswithruntime.MemStats.TotalAlloc. Results: 0.76/13.95/856.45 MB cumulative allocation; 1.58/15.73/430.34 ms. Successful validation does not satisfy the intended resource bound. - Reused existing arithmetic, cache, CTAS/replay and rollback/BVT evidence for unchanged paths. No broad suite or current-head race rerun was performed in this review. Mock/wire tests remain narrower than real mixed-version testing. No blanket claim of zero performance regression or exhaustive unhappy-path coverage is justified.
Repair and verification direction
Align partial/internal versus complete/session resolver contracts; preserve complete generated-column coordinates; remove full-table cloning from each CHECK candidate; remove the SCA nilness defect. These fixes fit the existing binder/executor structure. Verify resolver modes, generated first/middle/last positions, CHECK-count allocation scaling, and DUMP/LOAD success plus rollback, then rerun affected CI gates. The prior one-object v1/v2 LOAD timing uses different expression schemas and does not establish a matched performance baseline.
Temporary diagnostic tests were retained outside the repository with their logs and removed from the worktree; this review makes no production-code changes. The previously accepted standalone SHOW CREATE scope decision is unchanged.
| } | ||
| if c.proc != nil { | ||
| if resolve := c.proc.GetResolveVariableFunc(); resolve != nil { | ||
| return resolve(varName, isSystemVar, isGlobalVar) |
There was a problem hiding this comment.
[P1] Preserve compatibility with partial internal variable resolvers
This fallback now forwards every planner variable lookup to the process resolver, but existing internal-executor callers do not all provide a complete session resolver. In particular, testutils.ExecSQLWithReadResult supplies one that accepts only sql_mode; IsForeignKeyChecksEnabled now receives and propagates its variable foreign_key_checks not supported error instead of the previous nil/default result. On this exact head, Ubuntu UT reports 27 failed tests with that same error, including DROP/ALTER/LOAD/TRUNCATE paths (TestLoadS3, TestAffectedRows, TestLockNeedUpgrade, and partition tests). The coverage UT also fails. Align the resolver contract across callers, or restrict session delegation to explicitly session-backed replay; preserve actual session errors instead of blanket-swallowing them. Add a regression covering a partial internal resolver alongside the ALTER precision-inheritance case.
| } | ||
| cols := make([]*planpb.ColDef, 0, len(def.Cols)) | ||
| for _, candidate := range def.Cols { | ||
| if candidate != nil && !candidate.Hidden && !strings.EqualFold(candidate.Name, item.name) { |
There was a problem hiding this comment.
[P2] Keep the generated-column binder's original column coordinates
Excluding the generated column itself shifts every later column's ColPos, whereas ordinary CREATE binds against the complete allColDefs. A legal declaration such as create table t(q decimal(30,12) generated always as (a/b) stored, a decimal(10,2), b decimal(10,2)) therefore cannot be dumped: ordinary BuildPlan succeeds, but the new DUMP validator AnalyzeTableDumpBindings(ctx, def, def) returns table dump cannot verify bound generated expression q. I reproduced this on the reviewed head with the same expression at first/middle/last positions: first and middle fail; last passes. LOAD uses the same binder. Retain the authoring coordinate system and normal self-reference validation, and cover all three positions rather than only an appended generated column.
| return bound.Expr, nil | ||
| case "check": | ||
| wrapper := tableDumpBindContext{CompilerContext: ctx, ctx: bindCtx, increment: increment} | ||
| scratch := DeepCopyTableDef(def, true) |
There was a problem hiding this comment.
[P2] Avoid cloning all persisted expressions for every CHECK candidate
For each CHECK and each of the 31 increments, this deep-copies the entire table, including every other CHECK expression, then immediately discards scratch.Checks. With N simple division checks, that introduces O(31*N²) copying; the candidate-node budget counts only the current expression and misses this work. On this exact head, a valid table with two DECIMAL columns and 100 named CHECK(a/b > 0) clauses (3,138 bytes of CREATE SQL) takes 430 ms and 856,446,096 bytes (~817 MiB) of cumulative allocations for one AnalyzeTableDumpBindings call. Ten checks take 15.7 ms and 13,949,984 bytes. These are total allocations, not peak RSS. Both DUMP and LOAD pay this cost well below the configured limits. Reuse an immutable schema scope and allocate only the current candidate CHECK state; measure allocation growth with check count and include any remaining schema-copy work in the budget.
| return nil, false, moerr.NewInvalidInputNoCtx("target table contains a nil check") | ||
| } | ||
| source := sourceChecks[strings.ToLower(dest.Name)] | ||
| if source == nil || dest == nil || !strings.EqualFold(source.Name, dest.Name) || |
There was a problem hiding this comment.
[P2] Remove the impossible nil condition that fails required SCA
dest == nil already returns at lines 318–320, so this second test is impossible. The exact-head SCA job fails with nilness: impossible condition: non-nil == nil (govet) on this line. Remove the redundant condition and run the CI-equivalent linter on the changed package. The earlier successful plain go vet result did not cover this analyzer and cannot establish SCA success.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the complete PR range at 414c2d8f490c5fc9dcb5bcc9a8fd790d5c11b897, including the seven-file repair since my previous review. Approved; no remaining blocking finding established.
The four previously demonstrated failures are closed: partial internal variable resolvers retain their old behavior while an attached session remains authoritative; generated columns bind in their original column coordinates with explicit self-reference rejection; CHECK rebind uses a local table header instead of repeatedly cloning the full schema; the duplicate nil guard is gone. I checked the producer → manifest → replay path and the LOAD preflight, guarded catalog replacement, object tracking, statement rollback and bounded cleanup ownership. The repair does not add work to row execution paths.
Evidence at this head: full plan/compile/frontend UT, focused resolver and generated-position cases, two strict DUMP/LOAD fixture runs (96/96 each), issue 28594 fixture (140/140), changed-Go-package golangci-lint (0 issues), and the previously failing local partition/issues cases pass. The 100-CHECK benchmark is 36.7 ms and 18.4 MB allocated per operation, versus 430 ms and 856 MB cumulative allocation for the confirmed old-head reproducer; this demonstrates removal of the quadratic cloning cost for that input, not a universal throughput or peak-RSS guarantee. Remote arm64 SCA, Ubuntu coverage, proxy BVT and pessimistic multi-CN BVT are green. Ubuntu UT and the aggregate coverage job were still running at review time; I did not wait for them.
Scope note: the SHOW CREATE SQL copied into a new session does not encode per-expression division precision; this is an explicit separate design limit, not a failure of DUMP/LOAD replay in this PR.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-review of unchanged head 414c2d8f490c5fc9dcb5bcc9a8fd790d5c11b897. The full change map, prior defect closures, local UT/BVT/SCA and performance evidence remain applicable. No additional product correctness, lifecycle, or performance defect was established. The new terminal CI evidence changes the delivery decision to REQUEST_CHANGES: one validation blocker.
[P2] Restore complete CN coverage evidence and satisfy the required coverage gate. Coverage job 108309060484 reports changed-code coverage 71.21% (502/705) against the strict >75% gate, so CI Required fails. I downloaded the exact-head profiles/diff and reran the exact CI parser at matrixorigin/CI@b0f959479893a3ab54ff3d9422e135ae2e9c67b2; the same 502/705 result and exit 1 reproduce locally. The transient HTTP 503s recovered and are not the terminal failure.
This cannot currently be interpreted as all those SQL paths being untested: Compose BVT job 108301530519 explicitly reports table_dump_load.sql 96/96, zero failed/ignored/abnormal, in 4.362 s. Yet both original BVT profiles (bvt-compose.out and bvt-pessimistic.out) have zero hits in all 2,350 blocks of pkg/frontend/mysql_cmd_executor.go and all 679 blocks of pkg/frontend/table_dump.go. The v2 LOAD/ownership blocks are therefore absent from the collected execution evidence. Artifact manifests match this head and complementary groups in generation 36205017094-1; the parser merges nonzero hits correctly.
Both jobs spend about 10.5 s stopping each CN, consistent with the Compose stop deadline being reached before counters are written. This is a supported shutdown/collection hypothesis, not a proven container exit cause: final per-CN exit/counter evidence is unavailable. etc/launch-tae-compose/compose.yaml and cmd/mo-service/main.go are unchanged by this PR, so this is not evidence that the decimal patch caused the collection defect.
Required closure: capture/verify counters from each SQL-serving CN before removing the containers, recompute the existing gate with complete profiles, and add focused missing success/error tests if the valid result still fails. Do not lower the threshold or add redundant tests merely to compensate for lost BVT counters. The current 71.21% cannot establish the genuine combined UT+BVT coverage.
UT Ubuntu, SCA arm64, UT coverage production, proxy BVT and pessimistic BVT all finished successfully. The four previous code blockers stay closed. Existing performance evidence remains bounded to its tested inputs (100 CHECKs: 36.7 ms, 18.4 MB cumulative allocation; no new row-execution work from the final repair). No code or tests were modified during this review.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Follow-up review at 5ef74a001ef3b03a01fdd181c0d6a19bba9f3f27 (base 3f0a68bd8034d37e098765920f328985c308c9ff). The previous coverage blocker is resolved in the local evidence; no remaining confirmed code/review blocker.
The scope combines the previous complete arithmetic/planner/DDL/DUMP review with the two-commit delta: cancellable cgroup watcher ownership and adversarial DUMP wire tests. An independent reviewer checked the actual delta and coverage inputs. The correction is at the resource owner: a nonblocking inotify descriptor participates in the Go poller, cancellation closes the os.File, and initialization/admission failures release the descriptor. The CI collection guard is tracked separately in matrixorigin/CI#463, with the runtime rollout dependency documented.
QA evidence:
- Original CI binary: both 10s and 120s stops ended at exit 137, with the main goroutine waiting on the watcher blocked in
unix.Read; SQL-serving CN counters were missing. - Rebuilt production code at
ec6aa1a1f6(the following commit is tests only): strict DUMP/LOAD 96/96 twice, TimeToInt 267/267 twice, and #28594 division 140/140 twice after normal catalog admission. Both CNs exited0 and produced separate counters; the validated stop took about 7.4–7.7 s. The local runtime matches the locally built native libraries; this is not an exact CI-image benchmark. - System package normal/race tests, 100 focused race repetitions, and changed-package SCA pass. New frontend tests challenge correctly hashed malformed metadata, ensure restore returns no replacement and leaves the target unchanged, and check both sides of the CHECK-count and total-field limits. Focused normal/race and frontend SCA pass.
- Exact CI parser/filters, unchanged strict >75% threshold: 551/725=76.00% after union of the unchanged-source CI UT data, fresh valid local BVT counters, current watcher tests, and new passing QA tests. Stale watcher coverage coordinates were excluded. This is a local recomputation, not a claim that the new remote coverage job has passed.
Performance evidence remains scoped: the 100-CHECK local benchmark improved from about 430 ms / 856 MB cumulative allocation to 36.7 ms / 18.4 MB per operation. The 1,024-row temporal-division probe retained 32,784 B versus 81,952 B previously. The watcher repair adds bounded process-level state and no per-row work. These measurements do not establish universal throughput or peak-RSS guarantees.
The documented independent SHOW CREATE SQL limitation and v2-reader rollout requirements remain explicit. New-head remote CI results must still be checked before merging; this review does not represent those pending results as green.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Decision: APPROVE — no new confirmed blocking finding
Reviewed current head 5ef74a001ef3b03a01fdd181c0d6a19bba9f3f27 against main/merge-base 3f0a68bd8034d37e098765920f328985c308c9ff; both are unchanged since the preceding review. The working tree is clean. The complete 79-file scope is accounted for by the prior full change map plus the three-file shutdown/QA delta. I rechecked the relevant consumers and failure paths and added one missing width-scaling performance probe; I did not rerun unchanged successful suites.
Change and unhappy-path map
| Closure | Review conclusion |
|---|---|
| Exact decimal/temporal division | The session setting is bound into result precision/scale; execution consumes the bound type. Existing 0/4/10/30, Decimal128/256, rounding/overflow, NULL/masked-row and physical-layout oracles remain applicable. |
| Prepared/cache/retry and DDL replay | Setting changes trigger the existing rebuild lifecycle. COPY ALTER/LIKE/CTAS preserve compatible stored bindings and rebind actual type/declaration changes. Internal partial resolvers retain their previous unrelated-variable behavior. |
| DUMP/LOAD v2 | Approved design revision 10 is unchanged (SHA256 89d3587ff02d50cee59a442cd7eff6ba311e24ea045598550d67b6537a570868). The digest alone is not trusted: executable trees must match ordinary DDL binding. Private definition preparation, guarded replacement, transaction rollback and object ownership remain coherent. |
| Q1: resource ownership | The watcher closes its descriptor on partial initialization, failed task admission and task exit. Repeated cancellation/cleanup goes through idempotent os.File.Close. LOAD's fixture handle is deferred; catalog/object rollback uses the existing transaction owners. |
| Q2: waits/cancellation | The cancelled watcher closes the pollable descriptor to interrupt idle reads. In-flight callbacks finish before the task is counted as stopped. Error after ReplaceDef reaches statement rollback, including explicit transactions. |
| Q3: work/growth | Fixed watcher buffer/descriptor; DUMP payload, wire depth/field counts and candidate work have explicit bounds. Candidate binding remains an administrative-path cost; see width probe below. |
New performance challenge
The previous CHECK-count benchmark used two columns. I held CHECK count at 100 and exercised real CREATE binding plus AnalyzeTableDumpBindings with additional unused columns. The timed portion is the DUMP binding analysis only, one iteration per case, same process/toolchain:
| Columns | Time/op | Cumulative allocated bytes/op |
|---|---|---|
| 2 | 43.17 ms | 18,436,328 |
| 100 | 39.77 ms | 30,368,864 |
| 1,000 | 57.44 ms | 135,162,184 |
All cases passed. This shows remaining schema-width-dependent allocation, not a peak-RSS measurement or a before/after regression proof. A useful follow-up direction is reusing an immutable column-binding environment between candidates; there is no evidence here requiring a new global cache/framework or blocking this repair. The previously demonstrated quadratic full-CHECK cloning is removed. Existing scoped temporal execution measurements remain applicable; no universal throughput/nonregression claim is made.
Reproduce the new probe by extending the existing BenchmarkAnalyzeTableDumpBindingsChecks with column counts 2/100/1000, retaining 100 CHECKs, and using the repository CGo wrapper with -run '^$' -bench ... -benchtime=1x -benchmem -count=1. The task-only diagnostic source was removed after completion; no production/test delivery files changed in this review.
Evidence and limits
Revalidated the terminal evidence: owning-package UT from unchanged closures; system normal/race plus 100 watcher race repetitions; committed-source frontend QA normal/race and configured incremental SCA; strict two-run BVT of DUMP 96/96, temporal 267/267 and original issue 140/140. Correctly hashed malformed payloads reject without a replacement or target mutation; boundary cases test both the allowed cap and cap+1. Local coverage remains 551/725=76.00% with the unchanged CI parser, filters and >75% gate.
No unnecessary new asynchronous layer or framework was found. Per-expression binding preservation needs the documented v2 format; standalone SHOW CREATE replay is explicitly outside that contract. Real mixed 96/97 deployment and universal peak-memory/throughput claims are not established by mock/wire tests or these local probes. CI #463 remains a separate unmerged collection guard with a runtime rollout dependency. Remote CI on this exact head is still in progress; approval does not mean those pending checks have passed.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Decision: APPROVE — no remaining confirmed blocking finding
Reviewed all 79 changed files at 58058cf406f4fbcdc37c942ced4efa60453f8fe5 against base/merge-base 3f0a68bd8034d37e098765920f328985c308c9ff, plus all 17 preceding reviews, 15 inline comments and 2 issue comments. Review used an isolated, clean worktree; no production or test files were changed. The latest commit adds only the frontend declaration-validation test matrix; production is unchanged from 5ef74a001ef3b03a01fdd181c0d6a19bba9f3f27.
Historical comment closure
The 15 inline findings have corresponding fixes in the current code. All 15 GitHub threads still show unresolved; this is a code-closure assessment, not a claim that those threads were administratively resolved.
| Original comment | Current closure |
|---|---|
| Decimal256 alignment/rounding can wrap or hang | Headroom checks select a bounded wide fallback; shift-boundary, sign and SQL witnesses are covered. |
| Declared precision exceeded | Decimal256 division uses the checked batch path; declared-precision and rounding-carry errors are tested, including failed CTAS publication. |
| Per-row big.Int allocation on scale reduction | Ordinary negative adjustment scales the divisor in fixed-width storage; scale-cap benchmarks retain zero allocations for their measured kernels. |
| Negative 2^64 divisor panic | The shared absolute-value fit predicate excludes that boundary; DIV and integer-DIV shapes are covered. |
| Temporal division ignores the setting | Temporal operands enter the same precision calculation and lossless cast path; direct, prepared and CTAS cases cover the setting changes. |
| Temporal operands unnecessarily widen | Precision comes from packed numeric domains, including MatrixOne's full TIME range, rather than the coercion container width. |
| New plans sent to old CNs | Protocol 97 placement and send-time admission cover decimal division; unknown workers fall back. Persisted expressions also have authoring admission. Real two-version deployment remains a validation limit. |
| Raw result type has the wrong Size | Canonical types.New physical layout is used; the direct-result vector serialization round trip is tested. |
| Generated/CHECK binders lose session precision | DDL binders receive the common precision-bearing context. |
| CTAS rebinds inherited row defaults | Compatible inherited expressions retain their binding; actual type changes and explicitly authored defaults rebind. Alias, order, lineage, nullability and future-row controls are covered. |
| DUMP/LOAD loses stored bindings | v2 preserves bindings per expression, validates them against ordinary DDL binding and installs them transactionally. Mixed-precision forward/reverse cases and future INSERT/UPDATE pass. Standalone SHOW CREATE replay is an explicit separate design limitation. |
| Partial internal resolvers regress unrelated variables | A complete frontend delegate stays authoritative; the process-only fallback is restricted to division precision. |
| Generated-column positions break DUMP | Binding uses all visible columns with explicit self-reference rejection; first/middle/last positions are covered. |
| Quadratic full-schema CHECK cloning | Candidate CHECK binding uses a local table header without cloning the accumulated CHECK list. |
| Redundant nil guard fails SCA | Guard removed; current SCA passes. |
The earlier review-only intermediate-scaling overflow is also covered by the adjusted division fallback. The subsequent coverage/shutdown blocker is now closed by current remote evidence, not just the previous local reconstruction.
Current CI evidence
Run 36224193070 identifies this exact head. Ubuntu UT, Linux/arm64 SCA, shared build, UT coverage, proxy BVT, pessimistic multi-CN BVT, aggregate coverage and CI Required all pass. Skipped Darwin/upgrade jobs are not counted as successful compatibility tests.
Downloaded artifacts independently confirm:
- UT checkpoint selection includes system/frontend/protobuf/compile and function packages with
-race -tags matrixone_test; the corresponding stages finish with status 0. All eight planner shards finish successfully. - Proxy BVT executes the precision fixture 140/140 and DUMP/LOAD 96/96, with zero failed, ignored or abnormal statements. These fixtures are not selected in the other BVT shard.
- Both BVT coverage manifests name this head and generation
36224193070-1. The compose profile now hits 379/679 blocks in frontend/table_dump.go and 85/155 in table_dump_expressions.go; frontend executor counters are present in both profiles. This directly contradicts the previously observed missing SQL-serving CN counters. - The aggregate coverage job reports 571/725 modified lines, 78.76%, passing the unchanged strict >75% gate.
This review reuses those current CI runs and the preceding focused fault/performance evidence where the relevant production and fixtures are unchanged. It does not claim a fresh local server run or new local benchmark.
Design, unhappy paths and engineering quality
The arithmetic correction is an ordinary bug fix. The persistent DUMP v2 extension warrants a design gate: approved revision 10 is unchanged, SHA256 89d3587ff02d50cee59a442cd7eff6ba311e24ea045598550d67b6537a570868.
| Closure / risk | Ownership, consumer and failure review |
|---|---|
| Arithmetic, result metadata and vector layout — R3 hot path | Binding computes the logical result type; execution consumes its stored scale. The review covers rounding once, fixed-width overflow/headroom, logical precision, NULL/zero/masked rows, physical serialization and legacy tagged plans. Goldens follow the changed arithmetic contract. |
| Session cache, prepared generations and retry — R3 state | Setting changes use existing invalidation/rebuild ownership; plan and metadata publish together. Previously verified publication failure/retry and cross-session probes remain applicable. |
| DDL/CTAS/ALTER/LIKE — R3 catalog | Existing bindings survive compatible copying; new declarations/type domains rebind. Source expressions are cloned, references remapped, dependencies checked and protocol authoring admission retained. Replay context restores with defer. |
| DUMP writer → v1/v2 reader → catalog/object install — R3 persistence/security | A digest does not authorize executable trees: recovered expressions must match ordinary legal DDL binding. Preflight bounds size/depth/field/count before decoding; declarations and types must match. Preparation uses a private definition. Guarded replacement preserves identity/ownership and participates in the existing statement transaction and reverse-order restoration. Object installation/protection keeps existing file/GC owners. |
| Capability placement → remote send/receive — R3 distributed | Old/unknown destinations cannot silently consume the new scale contract. Responses have explicit release ownership; waits inherit cancellation and the shared deadline. |
| Linux watcher startup → cancellation → Stop — R3 lifecycle | Partial initialization and failed task admission close the descriptor. Nonblocking inotify participates in Go's poller; cancellation closes the file and unblocks idle reads. An active callback finishes before Stop completes. Repeated close is idempotent. |
| Regression/manual evidence and design artifacts — R0/R1, linked to owners above | Tests check independent expected values/errors, rejected publication and nearest successful controls. Historical failing TSV is explicitly diagnostic evidence, not an accepted golden. Latest declaration tests check error and non-mutation behavior rather than merely inflating execution count. |
Q1 resource ownership, Q2 wait termination and Q3 bounded growth were traced where applicable. Bounds include the fixed watcher buffer, SQL scale limits, 32 MiB expression payload, wire depth/field counts and bounded candidate work. The statement rollback path restores table definitions and clears relation caches. Existing fault evidence includes failure after replacement followed by COMMIT without retaining the failed statement's binding.
The added complexity has a concrete requirement: one session SET cannot encode differently authored expressions in the same table. v2 reuses existing DDL binding and transaction mechanisms. I found no unnecessary asynchronous framework, global cache or plugin abstraction. Index algorithm/ISCP/plugin rules are not triggered by this change.
Performance and accepted limits
Previously verified, unchanged-source measurements show the 100-CHECK case improving from about 430 ms / 856 MB cumulative allocation to 36.7 ms / 18.4 MB. The temporal 1,024-row probe retains 32,784 B, versus 81,952 B before its correction. These compare earlier PR implementations and the stated workloads; they are not universal main-versus-head benchmarks.
The width challenge still allocated about 135 MB cumulatively at 1,000 columns / 100 CHECKs. That is a bounded administrative-path cost and is not a peak-RSS measurement. Decimal256 promotion and per-destination capability RPCs also have costs. No universal throughput, peak-memory or high-fanout nonregression claim is established.
Accepted boundaries remain explicit: old readers reject v2, so reader rollout precedes use; copied SHOW CREATE SQL alone does not preserve mixed per-expression precision; real protocol-96/97 topology, high-fanout RPC latency, uncast nested prepared metadata equivalence and physical compile-failure injection are not fully established by these tests. The separate CI collection guard is outside this PR. None is newly demonstrated here as a blocking defect.
This branch was successfully deployed
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #28594
What this PR does / why we need it:
div_precision_incrementsetting to exact DECIMAL division type derivation and evaluation, including prepared statements and CTAS metadata.Validation on rebased head
aafa3abeac(base71c415705c):pkg/sql/plan/functionandpkg/sql/planUT: pass. Focused frontend prepared-plan and plan-cache UT: pass.make build-with-prebuilt-native: pass. Strict metadata BVT on the rebased service:issue_28594_div_precision.test70/70 andTimeToInt.sql267/267, both with zero failures.time.testpassed 82/82,datetime.test149/149, andtimestamp.test126/126.go vetpassed; golangci-lint v2.6.2 reported 0 issues.Mixed-version and direct execution follow-up (
9d0519ba54)Validation on this commit: full
pkg/pb/plan,pkg/sql/plan/function,pkg/sql/plan, andpkg/sql/compileunit tests pass; focused mixed-version tests andgo vetpass.make build-with-prebuilt-nativepasses. Strict #28594 BVT on the new service passes twice on the same instance, 70/70 each run, with empty error reports. Independent review found no remaining confirmed blocker. New-head CI is pending.Rolling-upgrade note: legacy views executed locally on a v96 CN keep their previous decimal division semantics until that CN is upgraded. Newly authored views using the revised semantics wait for the v97 catalog admission floor.
DDL expression and QA follow-up (
3903e9cbf5)div_precision_incrementsetting to CREATE/ALTER generated columns, defaults, ON UPDATE, CHECK constraints, and CTAS defaults. Internal COPY ALTER SQL inherits the session variable resolver.Validation on this commit: full
pkg/sql/planandpkg/sql/compileUT passed;go veton both packages passed; golangci-lint v2.6.2 reported 0 issues; nativemo-servicebuild passed. Strict metadata BVT on the rebuilt service passed twice, 117/117 statements per run. Existinggenerated_column.sqlpassed 334/334 with historical metadata differences ignored;check_constraints.sqlpassed 20/20 andcreate_table_like.sql25/25 under the same comparison mode. The new case used strict metadata comparison. The local SQL probe independently confirmed retained 12-digit values after column reorder while current direct division used 6 digits.Root-cause lesson: earlier tests covered operator inputs and precision values but omitted the DDL consumers and persisted-expression lifecycle. This change adds a consumer × setting-change × schema-change matrix, including negative CHECK cases, so rebinding mistakes surface as value or metadata mismatches.
CTAS inherited DEFAULT precision repair (
cb7025e3a3)Validation on this commit: full
pkg/sql/planUT passed;go vet ./pkg/sql/planpassed with repository CGo paths; golangci-lint v2.6.2 reported 0 issues. Rebuiltmo-service; strictissue_28594_div_precision.testBVT passed twice on one service, 140/140 per run with zero failed, ignored, or abnormal statements. Existingexpression_default_column_reference.sqlBVT passed 107/107 using its historical metadata-ignored mode. All manual probe databases and BVT database were removed. New-head CI is pending.Why repeated review was needed: the first planner mock omitted the catalog
Type.Tablelineage marker. That made a type comparison appear correct in UT while the SQL service still rebound inherited defaults. The regression test now models that marker, and public SQL verifies the persisted behavior. The new reuse path also required an explicit protocol-admission test; adding it caught a second omission before submission. The case inventory intest/manual/issue28594/README.mdrecords 74 coverage/gap entries and 10 remaining separate gaps.Table DUMP/LOAD binding repair (rebased head
08495b29d8)A table can contain persistent expressions authored under different
div_precision_incrementvalues. The v1 DUMP manifest carries text and a schema hash, but no per-expression binding. LOAD could accept a matching target created under another setting, then evaluate newly inserted rows at the target's precision rather than the source's. A real mixed DEFAULT=10/GENERATED=4 table reproduced this mismatch.Unmarshal: at most 16,384 columns/checks, 100,000 fields, depth 64, and a 32 MiB payload. Reject duplicate JSON fields and incompatible declarations. Validate relation/object/auto-increment metadata before one guarded catalog replacement, preserving the target's owner and creation time. An ambiguous v1 dump fails with a new-dump remedy before mutation.SHOW CREATEcopied independently into a new session is a separate SQL contract: the current CREATE syntax cannot express different increments per expression. Snapshot/PITR ordinary clone already carries bound expressions and is unaffected by this manifest change.Design:
docs/design/20260925-table-dump-expression-binding.md, revision 10, independent review PASS (SHA25689d3587ff02d50cee59a442cd7eff6ba311e24ea045598550d67b6537a570868). Final independent implementation review: PASS on clean head08495b29d8, with no remaining blocker.Validation on rebased production commit
9c18076b98(base3f0a68bd80) plus test-only commit08495b29d8:make build: PASS. Fullpkg/sql/planandpkg/frontendUT: PASS (17.211 s and 98.044 s). New exporter-cap test targeted: PASS. Incrementalgo veton both changed packages with repository CGo flags: PASS.table_dump_load.sqlBVT on the rebuilt service: 83/83 twice, zero failures, ignored, or abnormal cases. Covers both mixed-precision directions, old and new rows, and generated-column UPDATE results. White-box tests cover 0/4/10/30, nested/folded division, tampered executable tree, parser mode differences, producer/reader resource limits, and immutable target-definition preparation.ReplaceDef; after an explicitCOMMIT, the target still bound(1,3)at its original 4-digit setting. A downgraded v1 manifest was rejected before mutation. A service restart preserved the correctly loaded source precision and the rolled-back target precision.Compatibility: older LOAD readers reject v2; upgrade readers before producing v2 dumps. v1 remains for insensitive tables. The dump source must be kept when reverting a reader that cannot load v2. Remote CI on the new head is pending.
DUMP binding and resolver repair (
414c2d8f49)div_precision_increment. This restores the old nil/default behavior for unrelated variables such asforeign_key_checks, which caused 27 exact-head UT failures.Validation: full
pkg/sql/compile,pkg/sql/plan, andpkg/frontendunit suites passed locally; golangci-lint v2.6.2 incremental analysis of these packages reported 0 issues. The rebuilt service passed stricttable_dump_load.sqltwice on one instance (96/96 each), including old rows and future inserts with generated columns at first and middle positions; strictissue_28594_div_precision.testpassed 140/140. The 100-CHECK local probe measured about 856 MB cumulative allocation and 430 ms on the previous head; the final benchmark measured 18.4 MB/op and 36.7 ms/op on the same host. These are local administrative-path measurements, not a peak-RSS or throughput claim. Independent final review: PASS. New-head remote CI is pending.Why this needed another pass: earlier tests put generated columns only after their operands, so a shifted column coordinate escaped. The work budget counted AST visits but missed full-schema copies. Plain
go vetdid not cover the CInilnessanalyzer. This repair adds first/middle/last position cases, a count-scaling benchmark, an explicit schema-immutability assertion, and the CI-configured linter check.CN shutdown and adversarial QA follow-up (
5ef74a001e)Local validation: system package normal/race tests pass; focused watcher race tests pass 100 repetitions; new frontend QA normal/race tests pass; changed-package configured SCA reports 0 issues. Strict source-built two-CN BVT passes DUMP/LOAD 96/96 twice, TimeToInt 267/267 twice, and #28594 division 140/140 twice after normal catalog admission. Both CNs exit 0 and write counters in the validated teardown. The latest production binary is
ec6aa1a1f6;5ef74a001eand58058cf406add tests only.Coverage update at
58058cf406: the prior local 551/725 (76.00%) calculation included a locally controlled BVT teardown. The actual remote CI run for5ef74a001ereported 532/725 (73.38%), although DUMP/LOAD BVT ran 96/96; its collected CN profiles recorded zero hits in the DUMP binding files. The new frontend declaration-matrix UT directly executes the restore and rejection paths. Merging its focused UT coverage with that exact failed CI merged profile using the unchanged parser, filters, and strict >75% gate yields 550/725 (75.86%), an 18-block gain with the same 725 changed blocks. This is a local replay of the CI gate, pending the new-head remote result. The separate collector integrity fix remains in matrixorigin/CI#463.Performance evidence from the unchanged SQL repair remains: 100 CHECKs measured 36.7 ms / 18.4 MB allocated per operation versus the previous reproducer's 430 ms / 856 MB; the 1,024-row temporal-division probe reduced retained bytes from 81,952 to 32,784. The watcher adds bounded process-level state with no per-row work. These are scoped local measurements, not universal throughput or peak-RSS guarantees.