Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: type-checking code should not ignore reject flags #108166

Closed
mgartner opened this issue Aug 4, 2023 · 0 comments · Fixed by #108188
Closed

sql: type-checking code should not ignore reject flags #108166

mgartner opened this issue Aug 4, 2023 · 0 comments · Fixed by #108188
Assignees
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@mgartner
Copy link
Collaborator

mgartner commented Aug 4, 2023

There's several TypeCheck methods that replace the semaCtx's reject flags with new ones, causing the original reject flags set by the caller to be ignored. For example:

semaCtx.Properties.Require("CASE", RejectGenerators)

This can cause subtle bugs where expressions that should be rejected in certain contexts are not rejected when wrapped in expressions whose TypeCheck method replaces the flags.

As one example, aggregate functions are not allowed in ORDER BY clauses of DELETE and UPDATE as of #107641. But the rejection fails and we get an internal error if we wrap the aggregate function in a COALESCE:

CREATE TABLE t (a INT);

DELETE FROM t WHERE a > 0 ORDER BY COALESCE(sum(a), 1) LIMIT 1;
-- ERROR: internal error: top-level relational expression cannot have outer columns: (9
-- )
-- SQLSTATE: XX000
-- DETAIL: stack trace:
-- github.com/cockroachdb/cockroach/pkg/sql/opt/xform/optimizer.go:280: Optimize()
-- github.com/cockroachdb/cockroach/pkg/sql/plan_opt.go:592: buildExecMemo()
-- github.com/cockroachdb/cockroach/pkg/sql/plan_opt.go:240: makeOptimizerPlan()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1969: makeExecPlan()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:1485: dispatchToExecu
-- tionEngine()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:965: execStmtInOpenState()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:142: func1()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:2991: execWithProfiling()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor_exec.go:141: execStmt()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2220: func1()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2225: execCmd()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:2142: run()
-- github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:954: ServeConn()
-- github.com/cockroachdb/cockroach/pkg/sql/pgwire/conn.go:244: processCommands()
-- github.com/cockroachdb/cockroach/pkg/sql/pgwire/server.go:996: func3()
-- GOROOT/src/runtime/asm_arm64.s:1172: goexit()

Jira issue: CRDB-30341

@mgartner mgartner added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Aug 4, 2023
@mgartner mgartner self-assigned this Aug 4, 2023
@github-project-automation github-project-automation bot moved this to Triage in SQL Queries Aug 4, 2023
@mgartner mgartner moved this from Triage to Active in SQL Queries Aug 4, 2023
mgartner added a commit to mgartner/cockroach that referenced this issue Aug 7, 2023
This commit fixes a bug introduced in cockroachdb#105582 that caused
SemaRejectFlags to be restored during semantic analysis, preventing the
analysis from detecting some forms of invalid expressions.

Fixes cockroachdb#108166

There is no release note because the related bug does not exist in any
releases.

Release note: None
craig bot pushed a commit that referenced this issue Aug 7, 2023
108188: sql: refactor semantic analysis and fix some bugs r=mgartner a=mgartner

#### sql/sem/tree: simplify SemaCtx reject flag checks

Release note: None

#### sql/sem/tree: split derived SemaContext properties from contextual info

Properties derived about expressions during semantic analysis are
communicated to callers via ScalarProperties. Prior to this commit, this
type was also used to provide contextual information while traversing
sub-expressions during semantic analysis. For example, it would indicate
whether the current expression is a descendent of a window function
expression.

These two types of information, derived and contextual, are
fundamentally different. Derived properties bubble up from the bottom of
the tree to the top, while context propagates downward into
sub-expressions. This difference made it difficult to maintaining them
correctly in a single type and difficult to reason about. This commit
introduces the ScalarScene type which is used for providing internal
contextual information during semantic analysis.

Release note: None

#### sql/sem/tree: do not Restore SemaRejectFlags during semantic analysis

This commit fixes a bug introduced in #105582 that caused
SemaRejectFlags to be restored during semantic analysis, preventing the
analysis from detecting some forms of invalid expressions.

Fixes #108166

There is no release note because the related bug does not exist in any
releases.

Release note: None

#### sql: do not allow subqueries to be cast to enums in views and UDFs

This commit is a follow-up to #106868 after additional reproductions of
the original bug were found. For now, we disallow any CAST expressions
that contain a subquery in the input and the target type is an ENUM.
I've created #108184 to track this limitation.

Fixes #107654

There is no release note because the release note from #106868 should be
sufficient.

Release note: None

#### sql/randgen: fix typo in comment

Release note: None


Co-authored-by: Marcus Gartner <[email protected]>
@craig craig bot closed this as completed in 82109df Aug 7, 2023
@github-project-automation github-project-automation bot moved this from Active to Done in SQL Queries Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant