add check constraints - #26785
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? |
…zhen/matrixone into 0807-add-check-constraints
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for 2 P1 and 2 P2 findings at head f3c4991.
[P1] Gate the new information_schema views behind a fresh protocol generation after rebasing latest main.
This PR persists CHECK_CONSTRAINTS and TABLE_CONSTRAINTS definitions that reference the new mo_check_constraints table function:
matrixone/pkg/util/sysview/predefined.go
Lines 431 to 440 in f3c4991
matrixone/pkg/bootstrap/versions/v4_0_6/tenant_upgrade_list.go
Lines 127 to 150 in f3c4991
The upgrade framework explicitly permits a same-version CN with a lower version offset to remain in the cluster:
matrixone/pkg/bootstrap/service_upgrade.go
Lines 194 to 197 in 45a917d
That older binary has no mo_check_constraints dispatch and returns table function not supported:
matrixone/pkg/sql/plan/query_builder.go
Lines 10309 to 10332 in 45a917d
After this offset upgrade, mixed-cluster queries routed to an older CN will fail. Rebase latest main, preserve its existing protocol assignments, allocate a fresh MORPCVersion13 for this view/function contract, and make the tenant upgrade wait until the common protocol reaches v13. Add a mixed-CN boundary regression.
[P1] Do not parse non-table rel_createsql payloads as legacy CREATE TABLE SQL.
The catalog scan includes all non-temporary objects:
matrixone/pkg/sql/colexec/table_function/check_constraints.go
Lines 47 to 51 in f3c4991
The fallback then parses any payload containing the substring CHECK:
matrixone/pkg/sql/colexec/table_function/check_constraints_legacy.go
Lines 37 to 48 in f3c4991
Generic external tables store JSON in rel_createsql. A valid filepath such as stage://bucket/check.csv therefore enters the SQL parser, returns an error, and aborts the whole metadata stream. Both CHECK_CONSTRAINTS and TABLE_CONSTRAINTS then fail for that tenant because of an unrelated external table. Select/filter relkind and only run legacy parsing for eligible base tables; add external envelope, view, and source regressions.
[P2] Preserve the SQL mode of legacy CHECK definitions.
Legacy rel_createsql preserves the creating session SQL text, but the fallback always parses with the empty/default SQL mode:
matrixone/pkg/sql/colexec/table_function/check_constraints_legacy.go
Lines 37 to 48 in f3c4991
For create table source_t(a int, check ("a" > 0)), default mode and ANSI_QUOTES produce different CHECK clauses. The current code silently reports the default interpretation, so legacy valid tables can expose incorrect CHECK_CLAUSE metadata. Use source-preserving extraction or explicit SQL-mode ambiguity handling; do not silently select one mode. Cover ANSI_QUOTES, NO_BACKSLASH_ESCAPES, and PIPES_AS_CONCAT.
[P2] Avoid blocking full scans for LIMIT queries.
The catalog query imposes a global ORDER BY before streaming:
matrixone/pkg/sql/colexec/table_function/check_constraints.go
Lines 47 to 51 in f3c4991
and fillBatch always builds up to 8192 rows without honoring TableFunction.Limit:
matrixone/pkg/sql/colexec/table_function/check_constraints.go
Lines 240 to 270 in f3c4991
Thus SELECT ... FROM information_schema.check_constraints LIMIT 1 may still scan the whole tenant before returning. Remove the unnecessary sort and honor pushed limits so the producer can be cancelled early.
The focused parser counterexamples and git diff --check pass. The full table-function package was blocked by an unchanged Darwin CGo baseline failure; the same command failed at the exact PR base, so this is not attributed to the PR.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head f3c4991 after the successful CI rollup.
[P1] pkg/sql/colexec/table_function/check_constraints.go:220: stopStreaming cancels and drains only streamCh before waiting for streamDone. The streaming SQL executor can publish an error to errCh and return it, after which this producer publishes the same error again. Because errCh has capacity 1, reset/free after cancellation can leave the first error buffered and block the producer on the second send; streamCh is then never closed and stopStreaming waits forever. Give the error channel explicit producer ownership and drain it while shutting down, or otherwise ensure the producer can never block on error publication. Please add a deterministic executor-error plus cancellation/reset lifecycle regression.
读取 mo_tables.rel_createsql,当旧表没有 SchemaExtra.Checks 时,从 legacy CREATE TABLE SQL 解析 CHECK 定义,避免升级后旧 CHECK 约束丢失。
新增/使用 MORPCVersion13,CHECK_CONSTRAINTS 和 TABLE_CONSTRAINTS 的 upgrade entry 会等所有服务协议版本达到 v13 后再创建视图;planner 构建 mo_check_constraints 时也检查本机协议版本,避免 rolling upgrade 混合集群中老 CN 无法识别 table function。
catalog 查询限制 relkind='r',decode 层也跳过 external/view/source 等非 ordinary table,避免这些对象的 rel_createsql payload 含 CHECK 时被当作 CREATE TABLE 解析并中断查询。
legacy CHECK 解析会尝试 parser 相关 SQL mode 组合;如果不同 SQL mode 得到不同 CHECK clause,返回明确 ambiguity error,不再静默按 default mode 解释。
移除 catalog 查询里的 ORDER BY,支持把 plain LIMIT 下推到 mo_check_constraints table function;但不再把 LIMIT 拼到 mo_tables 源查询,而是在输出 CHECK 行层面计数,达到 LIMIT 后 cancel stream,避免前面无 CHECK 表导致漏结果。
RunStreamingSql 改用内部 error channel,外层 producer 只向 public errCh 非阻塞发布一次错误;stopStreaming 在 cancel/reset/free 时 drain error channel,避免 executor error 后 reset/free 卡死。 |
读取 mo_tables.rel_createsql,当旧表没有 SchemaExtra.Checks 时,从 legacy CREATE TABLE SQL 解析 CHECK 定义,避免升级后旧 CHECK 约束丢失。
新增/使用 MORPCVersion13,CHECK_CONSTRAINTS 和 TABLE_CONSTRAINTS 的 upgrade entry 会等所有服务协议版本达到 v13 后再创建视图;planner 构建 mo_check_constraints 时也检查本机协议版本,避免 rolling upgrade 混合集群中老 CN 无法识别 table function。
catalog 查询限制 relkind='r',decode 层也跳过 external/view/source 等非 ordinary table,避免这些对象的 rel_createsql payload 含 CHECK 时被当作 CREATE TABLE 解析并中断查询。
legacy CHECK 解析会尝试 parser 相关 SQL mode 组合;如果不同 SQL mode 得到不同 CHECK clause,返回明确 ambiguity error,不再静默按 default mode 解释。
移除 catalog 查询里的 ORDER BY,支持把 plain LIMIT 下推到 mo_check_constraints table function;但不再把 LIMIT 拼到 mo_tables 源查询,而是在输出 CHECK 行层面计数,达到 LIMIT 后 cancel stream,避免前面无 CHECK 表导致漏结果。
RunStreamingSql 改用内部 error channel,外层 producer 只向 public errCh 非阻塞发布一次错误;stopStreaming 在 cancel/reset/free 时 drain error channel,避免 executor error 后 reset/free 卡死。 |
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 9a1a9f9.
[P1] Make new-tenant information_schema initialization protocol-aware.
InitInformationSchemaSysTables now unconditionally contains both view definitions that reference mo_check_constraints() (pkg/util/sysview/sysview.go:58,67). Every CREATE ACCOUNT executes this list (pkg/frontend/authenticate.go:10489-10497), but building either view reaches buildCheckConstraints, whose protocol gate rejects a deployment-wide version below v14.
During a v13/v14 rolling upgrade, account creation routed to an upgraded CN therefore aborts when it reaches these views, while account creation routed to an old CN installs the legacy schema. UpgradeEntry.RequiredProtocolVersion protects existing-tenant upgrades but does not cover this initialization path.
Make tenant initialization version-aware: preserve or omit the old definitions while the common protocol is below v14, and guarantee that tenants created during that window receive the v14 views after rollout. Add a mixed-version CREATE ACCOUNT regression.
The previous legacy recovery, relation filtering, SQL-mode ambiguity, LIMIT, and stream-shutdown findings are addressed. I also closed the streaming ownership, wait, and buffer audit with no additional lifecycle finding.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 9a1a9f9.
[P1] Make new-tenant information_schema initialization protocol-aware.
InitInformationSchemaSysTables unconditionally includes CHECK_CONSTRAINTS and the new TABLE_CONSTRAINTS definition (pkg/util/sysview/sysview.go:58,67), and every CREATE ACCOUNT executes the full list (pkg/frontend/authenticate.go:10489-10497). Both views reference mo_check_constraints(), whose planner path rejects a deployment protocol below v14 (pkg/sql/plan/check_constraints.go:76-89 and pkg/sql/plan/build_constraint_util.go:205-223).
The RequiredProtocolVersion on the v4.0.6 upgrade entries protects existing-tenant upgrades only. During a v13/v14 rolling upgrade, CREATE ACCOUNT on a v14 CN therefore fails while planning these views; routing it to an older CN creates the legacy schema instead. The result depends on routing and the newly created tenant is not guaranteed to receive the v14 definitions after rollout.
Gate initial schema construction on the common protocol and ensure tenants created during the mixed-version window are upgraded after v14 becomes common. Please add a mixed-version CREATE ACCOUNT regression. The previous legacy recovery, relation filtering, SQL-mode ambiguity, LIMIT, and streaming shutdown findings are addressed on this head.
|
|
… 0807-add-check-constraints
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed the complete current diff and all review history. The latest head closes the prior blocking gaps: legacy CHECK metadata remains recoverable without parsing non-table payloads, SQL-mode-dependent legacy definitions fail explicitly, LIMIT stops the streaming catalog scan, result/error/cancel ownership is bounded and reset-safe, and protocol v15 gates both planning and mixed-version tenant view installation/upgrade. I also checked the latest main merge (clean), ran the focused reset race case 16x, the full table_function package under race, and the affected upgrade/frontend/planner/sysview packages; all passed. No blocking correctness, lifecycle, compatibility, or general-case performance issue found.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head 698efde with all 26 checks terminal and passing. The previous blocker is closed: new-tenant information_schema initialization now selects legacy definitions below protocol v15, both upgraded views are gated on v15, and the upgrade entries detect and replace mixed-window legacy or missing definitions after rollout. The earlier legacy metadata, relation filtering, SQL-mode, LIMIT, and streaming shutdown fixes remain intact. No remaining correctness, compatibility, lifecycle, or boundedness blocker found.
… 0807-add-check-constraints
LeftHandCold
left a comment
There was a problem hiding this comment.
Reviewed exact head d91ee27. No blocking issue found. Rechecked the protocol-v16 gate after the latest main merge, legacy metadata recovery, account/relation/temp filtering, SQL-mode ambiguity handling, LIMIT-driven stream cancellation, Reset/Free ownership, protocol-aware tenant initialization, and idempotent v4.0.6 view upgrades. Focused upgrade/sysview/planner tests and build/vet passed. Full local table_function/frontend validation hits the same pkg/common/docfilter CGo compile failure on exact base d01c859, so it is not introduced by this PR.
Merge Queue Status
This pull request spent 47 minutes 31 seconds in the queue, including 47 minutes 4 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #24730
What this PR does / why we need it:
主要完成 information_schema.CHECK_CONSTRAINTS 的兼容支持: