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: recent regression on planning comparisons #95633

Closed
knz opened this issue Jan 20, 2023 · 4 comments · Fixed by #97554
Closed

sql: recent regression on planning comparisons #95633

knz opened this issue Jan 20, 2023 · 4 comments · Fixed by #97554
Assignees
Labels
A-sql-execution Relating to SQL execution. A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. regression Regression from a release. T-sql-queries SQL Queries Team

Comments

@knz
Copy link
Contributor

knz commented Jan 20, 2023

Some time in the past 5 days, the planning / execution engine started to fail on the following query:

SELECT d.datname,
       pg_catalog.pg_get_userbyid(d.datdba)
   FROM pg_catalog.pg_database d;
ERROR: internal error: lookup for ComparisonExpr ((@18)[oid] = (('defaultdb')[string])[name])[bool]'s CmpOp failed

stack trace:

github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:491: MemoizeComparisonExprOp()
github.com/cockroachdb/cockroach/pkg/sql/sem/tree/expr.go:400: NewTypedComparisonExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder/scalar.go:257: buildComparison()
github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder/scalar.go:107: buildScalar()
github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder/builder.go:325: BuildScalar()
github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder/format.go:54: fmtInterceptor()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:1060: formatScalarWithLabel()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:920: formatScalar()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:230: formatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:1082: formatScalarWithLabel()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:920: formatScalar()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:230: formatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:915: formatRelational()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:232: formatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:915: formatRelational()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:232: formatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:915: formatRelational()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:232: formatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:915: formatRelational()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:232: formatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:218: FormatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/memo/expr_format.go:156: FormatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/xform/optimizer.go:1156: FormatExpr()
github.com/cockroachdb/cockroach/pkg/sql/opt/exec/execbuilder/scalar.go:889: func2()
github.com/cockroachdb/cockroach/pkg/sql/routine.go:66: func2()
github.com/cockroachdb/cockroach/pkg/sql/routine.go:95: EvalRoutineExpr()
github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:635: EvalRoutineExpr()
github.com/cockroachdb/cockroach/bazel-out/freebsd-fastbuild/bin/pkg/sql/sem/tree/eval_expr_generated.go:346: Eval()
github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:174: EvalCaseExpr()
github.com/cockroachdb/cockroach/bazel-out/freebsd-fastbuild/bin/pkg/sql/sem/tree/eval_expr_generated.go:91: Eval()
github.com/cockroachdb/cockroach/pkg/sql/sem/eval/expr.go:26: Expr()
github.com/cockroachdb/cockroach/pkg/sql/execinfrapb/pkg/sql/execinfrapb/expr.go:248: Eval()

cc @yuzefovich for triage

Jira issue: CRDB-23642
Epic: CRDB-23454

@knz knz added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-optimizer SQL logical planning and optimizations. A-sql-execution Relating to SQL execution. regression Regression from a release. labels Jan 20, 2023
@blathers-crl blathers-crl bot added the T-sql-queries SQL Queries Team label Jan 20, 2023
@knz
Copy link
Contributor Author

knz commented Jan 20, 2023

This is blocking #88061 from merging.

@yuzefovich
Copy link
Member

I think this is 61233f0, cc @mgartner

@knz
Copy link
Contributor Author

knz commented Jan 20, 2023

I have also just bisected this to #94797. I'm not sure that PR is causing the issue but it revealed it (probably because pg_get_userbyid is a UDF now)

@DrewKimball
Copy link
Collaborator

I'm going to take a looks at this now since Marcus is OOO and it's blocking another project.

@DrewKimball DrewKimball assigned DrewKimball and unassigned mgartner Feb 23, 2023
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Feb 23, 2023
During planning for a render node, the physical planning logic checks
whether the last created processor rendered its own expressions, adds its
render expressions to the processor if not. The check is necessary to handle
the case when expressions in a parent render reference those from a child
render.

However, render nodes also handle projecting input columns: both removing
them and reordering them. Correct planning of the processors depends on the
columns at each step being in a particular order. Since physical planning
didn't check whether the existing processor projected its columns in some
way, the parent render could be combined with its child. This could lead
to panics in the parent render expressions when the columns weren't projected
as expected. This patch fixes the issue by adding the projection check
before allowing the render planning code to add to the previous processor.

Fixes cockroachdb#95633

Release note (bug fix): Fixed a rare bug existing since before 22.1 that
could cause a projected expression to replace column references with the
wrong values.
craig bot pushed a commit that referenced this issue Feb 23, 2023
97554: physicalplan: don't combine render nodes into one processor r=DrewKimball a=DrewKimball

During planning for a render node, the physical planning logic checks whether the last created processor rendered its own expressions, adds its render expressions to the processor if not. The check is necessary to handle the case when expressions in a parent render reference those from a child render.

However, render nodes also handle projecting input columns: both removing them and reordering them. Correct planning of the processors depends on the columns at each step being in a particular order. Since physical planning didn't check whether the existing processor projected its columns in some way, the parent render could be combined with its child. This could lead to panics in the parent render expressions when the columns weren't projected as expected. This patch fixes the issue by adding the projection check before allowing the render planning code to add to the previous processor.

Fixes #95633

Release note (bug fix): Fixed a rare bug existing since before 22.1 that
could cause a projected expression to replace column references with the
wrong values.

Co-authored-by: Drew Kimball <[email protected]>
@craig craig bot closed this as completed in 7672be3 Feb 23, 2023
blathers-crl bot pushed a commit that referenced this issue Feb 23, 2023
During planning for a render node, the physical planning logic checks
whether the last created processor rendered its own expressions, adds its
render expressions to the processor if not. The check is necessary to handle
the case when expressions in a parent render reference those from a child
render.

However, render nodes also handle projecting input columns: both removing
them and reordering them. Correct planning of the processors depends on the
columns at each step being in a particular order. Since physical planning
didn't check whether the existing processor projected its columns in some
way, the parent render could be combined with its child. This could lead
to panics in the parent render expressions when the columns weren't projected
as expected. This patch fixes the issue by adding the projection check
before allowing the render planning code to add to the previous processor.

Fixes #95633

Release note (bug fix): Fixed a rare bug existing since before 22.1 that
could cause a projected expression to replace column references with the
wrong values.
craig bot pushed a commit that referenced this issue Feb 24, 2023
88061: clisqlshell: new infrastructure for describe commands r=rafiss,ZhouXing19 a=knz

Fixes #95320.
Epic: CRDB-23454

The SQL shell (`cockroach sql`, `demo`) now
supports the client-side commands `\l`, `\dn`, `\d`, `\di`, `\dm`,
`\ds`, `\dt`, `\dv`, `\dC`, `\dT`, `\dd`, `\dg`, `\du`, `\df` and `\dd` in a
way similar to `psql`, including the modifier flags `S` and `+`, for
convenience for users migrating from PostgreSQL.

A notable difference is that when a pattern argument is specified, it
should use the SQL "LIKE" syntax (with `%` representing the wildcard
character) instead of PostgreSQL's glob-like syntax (with `*`
representing wildcards).

Issues discovered:

- [x] join bug:  #88096
- [x] semi-join exec error #91012
- [x] `pg_table_is_visible` should return true when given a valid index OID and the index is valid.  #88097
- [x] missing pkey column in pg_index:  #88106
- [x] missing stored columns in pg_index: #88107 
- [x] pg_statistic_ext has problems #88108
- [x] missing view def on materialized views  #88109
- [x] missing schema comments: #88098
- [x] missing pronamespace for functions #94952
- [x] broken pg_function_is_visible for UDFs #94953
- [x] generated columns #92545
- [x] indnullsnotdistinct #92583
- [x] missing prokind #95288
- [x] missing function comments in obj_description #95292
- [x] planning regression #95633

96397: builtins: mark some pg_.* builtins as strict r=DrewKimball a=mgartner

Builtins defined using the UDF `Body` field will be wrapped in a `CASE`
expression if they are strict, i.e., `CalledOnNullInput=false`. When the
builtin is inlined, the `CASE` expression prevents decorrelation,
leaving a slow apply-join in the query plan. This caused a significant
regression of some ORM introspection queries.

Some of these builtins have filters that cause the SQL body to return no rows
if any of the arguments is NULL. In this case, the builtin will have the same
behavior whether or not it is defined as being strict. We can safely optimize
these builtins by setting `CalledOnNullInput=true`.

The following conditions are sufficient to prove that `CalledOnNullInput` can
be set for a builtin function with a SQL body:

  1. The WHERE clause of the SQL query *null-rejects* every argument of the
     builtin. Operators like `=` and `<` *null-reject* their operands because
     they filter rows for which an operand is NULL.

  2. The arguments are not used elsewhere in the query. This is not strictly
     necessary, but simplifies the proof because it ensures NULL arguments will
     not cause the builtin to error.

Examples of SQL statements that would allow `CalledOnNullInput` to be set:
```
SELECT * FROM tab WHERE $1=1 AND $2='two';

SELECT * FROM tab WHERE $1 > 0;
```

Fixes #96218
Fixes #95569

Epic: None

Release note: None


97585: cli: don't scope TLS client certs to a specific tenant by default r=stevendanna a=knz

Epic: CRDB-23559
Fixes: #97584

This commit changes the default for `--tenant-scope` from "only the system tenant" to "cert valid for all tenants".

Note that the scoping is generally useful for security, and it is used in CockroachCloud. However, CockroachCloud does not use our CLI code to generate certs and sets its cert tenant scopes on its own.

Given that our CLI code is provided for convenience and developer productivity, and we don't expect certs generated here to be used in multi-tenant deployments where tenants are adversarial to each other, defaulting to certs that are valid on every tenant is a good choice.

Release note: None


Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
DrewKimball added a commit that referenced this issue Apr 3, 2023
During planning for a render node, the physical planning logic checks
whether the last created processor rendered its own expressions, adds its
render expressions to the processor if not. The check is necessary to handle
the case when expressions in a parent render reference those from a child
render.

However, render nodes also handle projecting input columns: both removing
them and reordering them. Correct planning of the processors depends on the
columns at each step being in a particular order. Since physical planning
didn't check whether the existing processor projected its columns in some
way, the parent render could be combined with its child. This could lead
to panics in the parent render expressions when the columns weren't projected
as expected. This patch fixes the issue by adding the projection check
before allowing the render planning code to add to the previous processor.

Fixes #95633

Release note (bug fix): Fixed a rare bug existing since before 22.1 that
could cause a projected expression to replace column references with the
wrong values.
DrewKimball added a commit to DrewKimball/cockroach that referenced this issue Apr 3, 2023
During planning for a render node, the physical planning logic checks
whether the last created processor rendered its own expressions, adds its
render expressions to the processor if not. The check is necessary to handle
the case when expressions in a parent render reference those from a child
render.

However, render nodes also handle projecting input columns: both removing
them and reordering them. Correct planning of the processors depends on the
columns at each step being in a particular order. Since physical planning
didn't check whether the existing processor projected its columns in some
way, the parent render could be combined with its child. This could lead
to panics in the parent render expressions when the columns weren't projected
as expected. This patch fixes the issue by adding the projection check
before allowing the render planning code to add to the previous processor.

Fixes cockroachdb#95633

Release note (bug fix): Fixed a rare bug existing since before 22.1 that
could cause a projected expression to replace column references with the
wrong values.
@mgartner mgartner moved this to Done in SQL Queries Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-execution Relating to SQL execution. A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. regression Regression from a release. T-sql-queries SQL Queries Team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants