Skip to content

fix: align INSERT/UPDATE string width enforcement with MySQL sql_mode (#25200) (cherry-pick #25262)#25312

Open
ck89119 wants to merge 18 commits into
matrixorigin:mainfrom
ck89119:issue-25200-main
Open

fix: align INSERT/UPDATE string width enforcement with MySQL sql_mode (#25200) (cherry-pick #25262)#25312
ck89119 wants to merge 18 commits into
matrixorigin:mainfrom
ck89119:issue-25200-main

Conversation

@ck89119

@ck89119 ck89119 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #25200

What this PR does / why we need it:

Cherry-pick of PR #25262 from 3.0-dev to main.

Rework CHAR/VARCHAR width enforcement on DML assignment paths so it matches MySQL's sql_mode semantics:

  • sql_mode gating (runtime): add a new cast_assign function used by DML assignment paths for CHAR/VARCHAR targets. It reads sql_mode at runtime (strict -> reject 1406, non-strict -> truncate), avoiding plan-cache staleness.
  • trailing-space exemption: over-length values whose excess is only trailing spaces are accepted (truncated) even under strict mode, matching MySQL.
  • INSERT IGNORE downgrade: under IGNORE, over-length writes are truncated instead of rejected.

Notes

🤖 Generated with Claude Code

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@matrix-meow matrix-meow added the size/XXL Denotes a PR that changes 2000+ lines label Jul 1, 2026
@matrix-meow matrix-meow removed the size/XXL Denotes a PR that changes 2000+ lines label Jul 1, 2026
ck89119 and others added 3 commits July 3, 2026 11:37
…matrixorigin#25200)

PR matrixorigin#25162 routed all CHAR/VARCHAR assignment paths through the internal
cast_strict, which rejects over-length writes unconditionally regardless
of sql_mode. This diverged from MySQL and from the LOAD DATA path (which
already honors sql_mode).

Introduce a runtime, sql_mode-gated assignment cast and wire the DML
assignment paths to it:

- New cast_assign function (volatile, so prepared statements resolve
  sql_mode at execution time): strict sql_mode rejects over-length
  CHAR/VARCHAR writes; non-strict truncates. Over-length values whose
  excess is only trailing spaces are accepted (truncated) even in strict
  mode, matching MySQL.
- INSERT/UPDATE/INSERT...SELECT/ON DUPLICATE KEY UPDATE now use
  cast_assign; INSERT IGNORE downgrades to the lenient cast (truncation)
  regardless of sql_mode.
- DDL column DEFAULT/ON UPDATE keep cast_strict (the DDL-layer width
  check is not relaxed by sql_mode in MySQL).
- Relax the plan-time width check in SetInsertValueString for
  CHAR/VARCHAR so the runtime cast_assign is the single source of truth.

Not included: emitting a 1265 truncation warning (issue item matrixorigin#4). MO has
no plumbing from vectorized execution to the session warning stack; that
needs separate infrastructure.

Adds unit tests (func_cast_width_test.go) and a BVT
(insert_string_width_sqlmode) covering sql_mode gating, the trailing-space
exemption, INSERT IGNORE downgrade, prepared-statement execution-time
resolution, INSERT...SELECT/UPDATE/ON DUP, and the DDL DEFAULT case.
Updates existing BVT results whose over-length CHAR/VARCHAR literal error
message moved from the plan-time to the runtime cast form.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- mysql_cmd_executor.go:2692: use Error (zap args) instead of Errorf
  (printf-style) for structured log message without format directives
- cdc_test.go:3721: fix format string %q -> %d for int64 type

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…main

# Conflicts:
#	pkg/frontend/mysql_cmd_executor.go
#	pkg/sql/plan/function/func_cast.go

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes.

The cherry-pick fixes the local/plan-level blockers I raised on #25262: INSERT IGNORE is now threaded through the non-literal VALUES path, prepared literal VALUES now route through a volatile runtime cast instead of freezing the plan-time decision, VARBINARY is no longer relaxed by the CHAR/VARCHAR constant path, and the fallback buildInsert initializes isInsertIgnore. CI is green and the new BVT covers the main single-CN SQL matrix.

I still see one correctness blocker in the current design: cast_assign reads sql_mode from the local process.Process resolver at execution time, but remote-run processes do not carry that resolver or an encoded sql_mode value. That means the same assignment cast can behave as strict on a remote CN even when the user session is non-strict.

Relevant paths:

  • pkg/sql/plan/function/func_cast.go:458-459: NewAssignCast calls isStrictSqlMode(proc) at function execution time.
  • pkg/sql/plan/function/func_cast.go:467-469: if proc.GetResolveVariableFunc() is nil, isStrictSqlMode returns true.
  • pkg/sql/compile/remoterunClient.go:182-195: remote run sends the encoded scope plus encoded process info.
  • pkg/vm/process/process_codec.go:92-102: encoded pipeline.SessionInfo includes user/host/db/version/timezone/query id/lock timeout, but not sql_mode and not a resolver.
  • pkg/sql/compile/remoterunServer.go:398-415: the remote CN rebuilds a fresh process.Process, restores SessionInfo, and does not install any ResolveVariableFunc.

The existing remote var-expression mechanism does not close this gap: foldVarExprsInRemoteRunScope only folds explicit plan.Expr_V variables before encoding (pkg/sql/compile/remote_expr.go:51-118). cast_assign hides the sql_mode dependency inside the function implementation, so the scope does not contain a var expr to fold.

This matters for assignment paths that can be executed inside remote scopes, e.g. distributed INSERT ... SELECT / UPDATE plans where the projection contains the new cast_assign. In non-strict mode, local execution truncates, while a remote CN with nil resolver will take the strict fallback and return 1406. The current BVT covers the SQL semantics, but not this remote-process boundary.

Suggested fix: make the strictness input explicit and portable across remote execution. For example, either encode the session sql_mode into the remote ProcessInfo / SessionInfo and have isStrictSqlMode fall back to that, or model sql_mode as an explicit expression/input to the assignment cast so the existing remote var-folding path can capture the current execution value. Please also add coverage for the remote/multi-CN path: non-strict INSERT ... SELECT with over-width CHAR/VARCHAR should truncate on a remote execution path, and strict mode should still reject.

Local validation I ran on the current PR head:

go test -ldflags="-extldflags '-L$(pwd)/cgo -lmo -L$(pwd)/thirdparties/install/lib -Wl,-rpath,@executable_path/lib'" \
  -v -count=1 -timeout 120s ./pkg/sql/plan/function \
  -run 'Test(StrToStrWidthEnforcement|OverLenIsAllTrailingSpaces|IsStrictSqlMode|NewAssignCastHonorsSqlMode|strToStr_StrictStringWidth|strToStr_TextToCharVarchar)'
PASS

go test -ldflags="-extldflags '-L$(pwd)/cgo -lmo -L$(pwd)/thirdparties/install/lib -Wl,-rpath,@executable_path/lib'" \
  -v -count=1 -timeout 120s ./pkg/sql/plan \
  -run 'Test(InsertSelectVarcharFromTextUsesAssignmentCast|OnDuplicateUpdateVarcharFromTextUsesAssignmentCast|MakeInsertValueConstExprGeometry)'
PASS

ck89119 and others added 7 commits July 6, 2026 16:25
P1: Propagate strictStringWidth to bool/bit cast paths so
bool/varchar and bit/varchar width enforcement is sql_mode-gated.

P2: Capture sql_mode in SessionInfo and serialize through
pipeline proto so distributed CNs do not incorrectly assume
strict mode when the resolver is unavailable.

P3: Replace formatCastError (internal error) with
formatDataTruncationError (1406 / ER_DATA_TOO_LONG) for all
cast width violations, matching MySQL error semantics.

Co-Authored-By: Claude <noreply@anthropic.com>
…main

# Conflicts:
#	pkg/sql/util/eval_expr_util.go
P1-1: Use EmptySqlModeSentinel to distinguish explicitly-empty
(non-strict) sql_mode from an unset field. Fixes remote CNs
incorrectly defaulting to strict when the source session is
non-strict (sql_mode='').

P1-2: Add sql_mode field to proto/pipeline.proto SessionInfo
message and regenerate pipeline.pb.go.

P1-3: Propagate strictStringWidth through yearToOthers/yearToStr
so YEAR->VARCHAR width enforcement respects sql_mode.

P2: Correct SQLSTATE comment in formatDataTruncationError from
22001 to HY000 (matches actual moerr/error.go registration).

Co-Authored-By: Claude <noreply@anthropic.com>
…add width check to yearToStr

P1: Add strictStringWidth propagation to uuidToOthers/uuidToStr,
tsToOthers/tsToStr, jsonToOthers/jsonToStr, and enumToOthers/enumToStr.
This ensures uuid/json/enum/ts->VARCHAR(N) width enforcement honors
sql_mode (truncate in non-strict, reject in strict).

P1: Add width check after truncateCastBytesResult in yearToStr.
Previously strict mode would silently accept over-wide YEAR values
(e.g. YEAR 2026 into VARCHAR(2)) without reporting 1406.

Co-Authored-By: Claude <noreply@anthropic.com>
P1: Replace byte-level truncateCastBytesResult in jsonToStr with
rune-count-based truncation (truncateStringByRunes) for CHAR/VARCHAR
targets, preventing mid-character truncation of multi-byte JSON output.

P2: Use moerr.NewErrInvalidDefault (1067 ER_INVALID_DEFAULT) instead
of formatDataTruncationError (1406) when cast_strict rejects an
over-long DDL default/on-update value, matching MySQL semantics.

Update test expectations and BVT result file accordingly.

Co-Authored-By: Claude <noreply@anthropic.com>
…main

# Conflicts:
#	pkg/sql/plan/function/function_id_test.go
ck89119 and others added 8 commits July 8, 2026 14:33
Add ErrCastWidthExceeded error code (MySQL 1406/ER_DATA_TOO_LONG)
with the same message format as the old formatCastError ("internal
error: Can't cast..."), keeping existing BVT result files compatible
while still returning the correct MySQL error code.

Revert hand-edited BVT result files to upstream format since the
error messages now match the old formatCastError output.

Also update insert_string_width_sqlmode.result to match the new
message format (DML errors use 1406 via ErrCastWidthExceeded, DDL
default errors use 1067 via NewErrInvalidDefault).

Co-Authored-By: Claude <noreply@anthropic.com>
getPkValueExpr was calling forceAssignmentCastExpr (no ignore flag)
instead of forceAssignmentCastExprWithIgnore, causing INSERT IGNORE
with over-long CHAR/VARCHAR primary key values to fail with 1406 in
strict sql_mode instead of truncating and ignoring the duplicate.

Add BVT coverage: INSERT IGNORE + varchar primary key + over-length
value under STRICT_TRANS_TABLES.

Co-Authored-By: Claude <noreply@anthropic.com>
ErrCastWidthExceeded was mapped to ER_DATA_TOO_LONG (MySQL 1406), causing
the JDBC driver used by mo-tester to wrap it as java.sql.DataTruncation
and prepend 'Data truncation: ' to the message. This broke BVT result
matching across all CI jobs.

Change the mapping to ER_UNKNOWN_ERROR (1105), matching the old
formatCastError behavior that uses NewInternalError -> ErrInternal ->
ER_UNKNOWN_ERROR, which does not trigger JDBC prefix.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Change the const-level error format in formatCastError and
formatDataTruncationError from 'Can't cast X from TYPE1 type to
TYPE2 type' to 'Can't cast X to TYPE2 type', matching the format
expected by BVT result files.

Also update result files that still used the old 'from...to' format:
- insert_string_width_sqlmode.result
- update_text_coalesce_cast.result
- binary.result
- fulltext2.result

Co-Authored-By: Claude <noreply@anthropic.com>
Generated columns go through NewCast (strict, non-assignment path) which
produces ErrInvalidDefault (1067) for over-width values, not the DML cast
error (formatDataTruncationError). Update the result file to match the
actual error behavior.

Co-Authored-By: Claude <noreply@anthropic.com>
…main

# Conflicts:
#	test/distributed/cases/dtype/binary.result
…main

Resolve function_id conflicts: renumber CAST_STRICT=541, CAST_ASSIGN=542,
DATE_TRUNC=543, JSON_CONTAINS=544, JSON_REMOVE=545, JSON_CONTAINS_PATH=546,
FUNCTION_END_NUMBER=547.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement kind/test-ci size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants