-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: severe regression in ORM query performance {django} #96218
Comments
Adding more info -- I tried on my single-region gceworker with the latest commit on master (
and with 22.2.2:
|
I tried with 6fd5b04 (the commit before #95234 was merged), and observed that the apply join is no longer planned. On a local single node cluster with 0 tables:
|
I'll take a look. Nothing immediately stands out to me - that PR should have only affected queries that were erroring with |
I tried with commit 2f81142 (from #93218), and that also does not have the apply join.
|
My bad. I thought I had tested 61233f0, which was why I tagged you. Thanks for taking on the bisect! |
This is the commit where the apply-join gets added: c2c6239. The One workaround is to mark the builtin (and the ones similar to it) as NOT strict. I believe the behavior of the SELECT n.nspname = any current_schemas(true)
FROM pg_catalog.pg_class c
INNER LOOKUP JOIN pg_catalog.pg_namespace n
ON c.relnamespace = n.oid
WHERE c.oid=$1 LIMIT 1 The I'll go ahead and put up a PR. |
Nice find! I suppose we should make the same change for the other
is there a concern that preventing decorrelation would be bad for other performance of other functions? |
@mgartner do you think it would be possible to have a rule that will remove the |
This patch adds a normalization rule `SimplifyLeakproofBooleanCase` that replaces a leak-proof boolean CASE statement with an equivalent boolean expression built using `AND` and `OR` operators. This is possible when the following conditions are satisfied: 1. The CASE statement returns a boolean value. 2. The CASE statement is leak-proof. 3. The CASE condition is a *constant* boolean value (True, False, NULL). 4. Each WHEN branch returns a *constant* boolean value. Example: ``` CASE WHEN a THEN False WHEN b THEN True ELSE c END => ((a IS NOT True) AND (b OR c)) ``` See the `ConvertCaseToCondition` comment for details. This is useful for strict UDFs, which implement the argument NULL check using a CASE statement. Informs cockroachdb#96218 Release note: None
Yes, that seems valuable, but I don't think it helps in this case because these builtins are stable, non-leakproof functions. |
We can still hoist stable functions a lot of the time, right? After function hoisting, the CASE becomes leakproof (at least in this case) |
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]>
I was investigating ORM query performance in CRDB 22.2.2 as well as latest (g69dd453d0e) and noticed some major regressions for Django's table introspection query when testing on Roachprod.
Steps to repro:
g69dd453d0e
) - takes 694 secondsJira issue: CRDB-24010
The text was updated successfully, but these errors were encountered: