builtins: mark some pg_.* builtins as strict#96397
builtins: mark some pg_.* builtins as strict#96397craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
In a One non-obvious side-effect of this change is that these builtins could potentially result in errors when presented with a |
rafiss
left a comment
There was a problem hiding this comment.
this lgtm! i think the same change should be made to the following, since they have the same pattern of using a single WHERE clause filtering by the first parameter:
- makePGGetViewDef
- makePGGetConstraintDef
- pg_get_functiondef
- pg_get_function_arguments
- pg_get_function_result
- pg_get_function_identity_arguments
- pg_get_indexdef
- col_description
- obj_description
- shobj_description
- _pg_index_position
|
Will we be able to merge either this PR or #96545? The performance regression is causing nightlies to fail (see #95569 (comment)), since the latency allows a different bug to manifest (#94337). |
bb130f7 to
7b33861
Compare
|
I've added all the pg_.* builtins that I could prove would return the expected result when called with NULL arguments, and added corresponding tests. |
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 cockroachdb#96218
Fixes cockroachdb#95569
Epic: None
Release note: None
7b33861 to
649f219
Compare
rharding6373
left a comment
There was a problem hiding this comment.
Thanks for wrapping this up Drew!
Reviewable status:
complete! 1 of 0 LGTMs obtained
|
TFTRs! bors r+ |
|
Build succeeded: |
|
Thanks @DrewKimball! |
Builtins defined using the UDF
Bodyfield will be wrapped in aCASEexpression if they are strict, i.e.,
CalledOnNullInput=false. When thebuiltin is inlined, the
CASEexpression 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
CalledOnNullInputcanbe set for a builtin function with a SQL body:
The WHERE clause of the SQL query null-rejects every argument of the
builtin. Operators like
=and<null-reject their operands becausethey filter rows for which an operand is NULL.
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
CalledOnNullInputto be set:Fixes #96218
Fixes #95569
Epic: None
Release note: None