partialidx: fix panic with two var comparison within conjunctions #53222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit fixes a panic that is produced when trying to prove
implication with a disjunction of two-variable comparisons. The panic
only occurs when the comparisons are equivalent, but not exactly the
same. For example:
a > b OR c > d
=>
b < a OR c < d
Release note: None
|
This change is |
RaduBerinde
approved these changes
Aug 22, 2020
|
bors r=RaduBerinde |
|
Build succeeded: |
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Aug 26, 2020
This commit adds a typed set for tracking filter expressions that are exact matches of predicate expressions. This will help prevent a common developer mistake of not checking whether or not the set it `nil` before manipulating it, such as the bug fixed by cockroachdb#53222. Benchmarks show performance improvements of the Implicator for some cases as a result of this change. name old time/op new time/op delta Implicator/single-exact-match-16 81.2ns ± 1% 77.8ns ± 1% -4.24% (p=0.008 n=5+5) Implicator/single-inexact-match-16 916ns ± 1% 914ns ± 0% ~ (p=0.254 n=5+5) Implicator/range-inexact-match-16 2.40µs ± 0% 2.46µs ± 1% +2.56% (p=0.008 n=5+5) Implicator/single-exact-match-extra-filters-16 313ns ± 0% 305ns ± 1% -2.74% (p=0.008 n=5+5) Implicator/single-inexact-match-extra-filters-16 3.85µs ± 1% 3.85µs ± 1% ~ (p=0.952 n=5+5) Implicator/multi-column-and-exact-match-16 90.0ns ± 1% 84.3ns ± 1% -6.29% (p=0.008 n=5+5) Implicator/multi-column-and-inexact-match-16 1.99µs ± 0% 2.02µs ± 1% +1.53% (p=0.016 n=4+5) Implicator/multi-column-or-exact-match-16 82.3ns ± 3% 78.1ns ± 1% -5.10% (p=0.008 n=5+5) Implicator/multi-column-or-exact-match-reverse-16 1.72µs ± 1% 1.74µs ± 2% ~ (p=0.397 n=5+5) Implicator/multi-column-or-inexact-match-16 2.23µs ± 2% 2.31µs ± 3% +3.74% (p=0.008 n=5+5) Implicator/and-filters-do-not-imply-pred-16 3.61µs ± 1% 3.63µs ± 4% ~ (p=0.548 n=5+5) Implicator/or-filters-do-not-imply-pred-16 916ns ± 2% 905ns ± 2% ~ (p=0.095 n=5+5) Implicator/many-columns-exact-match10-16 305ns ± 1% 278ns ± 1% -8.97% (p=0.008 n=5+5) Implicator/many-columns-inexact-match10-16 12.9µs ± 1% 12.9µs ± 1% ~ (p=0.841 n=5+5) Implicator/many-columns-exact-match100-16 21.3µs ± 1% 19.2µs ± 2% -9.94% (p=0.008 n=5+5) Implicator/many-columns-inexact-match100-16 502µs ± 1% 503µs ± 1% ~ (p=0.421 n=5+5) Release justification: Low risk update to new functionality, partial indexes. Release note: None
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Aug 26, 2020
This commit adds a typed set for tracking filter expressions that are exact matches of predicate expressions. This will help prevent a common developer mistake of not checking whether or not the set it `nil` before manipulating it, such as the bug fixed by cockroachdb#53222. Benchmarks show performance improvements of the Implicator for some cases as a result of this change. name old time/op new time/op delta Implicator/single-exact-match-16 81.2ns ± 1% 77.8ns ± 1% -4.24% (p=0.008 n=5+5) Implicator/single-inexact-match-16 916ns ± 1% 914ns ± 0% ~ (p=0.254 n=5+5) Implicator/range-inexact-match-16 2.40µs ± 0% 2.46µs ± 1% +2.56% (p=0.008 n=5+5) Implicator/single-exact-match-extra-filters-16 313ns ± 0% 305ns ± 1% -2.74% (p=0.008 n=5+5) Implicator/single-inexact-match-extra-filters-16 3.85µs ± 1% 3.85µs ± 1% ~ (p=0.952 n=5+5) Implicator/multi-column-and-exact-match-16 90.0ns ± 1% 84.3ns ± 1% -6.29% (p=0.008 n=5+5) Implicator/multi-column-and-inexact-match-16 1.99µs ± 0% 2.02µs ± 1% +1.53% (p=0.016 n=4+5) Implicator/multi-column-or-exact-match-16 82.3ns ± 3% 78.1ns ± 1% -5.10% (p=0.008 n=5+5) Implicator/multi-column-or-exact-match-reverse-16 1.72µs ± 1% 1.74µs ± 2% ~ (p=0.397 n=5+5) Implicator/multi-column-or-inexact-match-16 2.23µs ± 2% 2.31µs ± 3% +3.74% (p=0.008 n=5+5) Implicator/and-filters-do-not-imply-pred-16 3.61µs ± 1% 3.63µs ± 4% ~ (p=0.548 n=5+5) Implicator/or-filters-do-not-imply-pred-16 916ns ± 2% 905ns ± 2% ~ (p=0.095 n=5+5) Implicator/many-columns-exact-match10-16 305ns ± 1% 278ns ± 1% -8.97% (p=0.008 n=5+5) Implicator/many-columns-inexact-match10-16 12.9µs ± 1% 12.9µs ± 1% ~ (p=0.841 n=5+5) Implicator/many-columns-exact-match100-16 21.3µs ± 1% 19.2µs ± 2% -9.94% (p=0.008 n=5+5) Implicator/many-columns-inexact-match100-16 502µs ± 1% 503µs ± 1% ~ (p=0.421 n=5+5) Release justification: Low risk update to new functionality, partial indexes. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Aug 27, 2020
53507: partialidx: reduce remaining filters of semantically equivalent expressions r=rytaft a=mgartner
#### partialidx: reduce remaining filters of semantically equivalent expressions
This commit reduces the remaining filters returned from the Implicator
in cases where expressions in the filter are semantically equivalent to
expression in the predicate, but not syntactically equivalent.
For example:
(a::INT > 17)
=>
(a::INT >= 18)
Prior to this commit, `(a > 17)` would remain as a filter to be applied
after a partial index scan. However, this filter is unnecessary, because
there are no integers between `17` and `18`. With this change,
`(a > 17)` is removed from the remaining filters.
Release justification: Low risk update to new functionality, partial
indexes.
Release note (performance improvement): The optimizer reduces filters
applied after partial index scans in more cases where the filters are
implicitly applied by the partial index predicate. This could lead to
more efficient query plans in some cases.
#### partialidx: add safer set operations for tracking exact matches
This commit adds a typed set for tracking filter expressions that are
exact matches of predicate expressions. This will help prevent a common
developer mistake of not checking whether or not the set it `nil` before
manipulating it, such as the bug fixed by #53222.
Benchmarks show performance improvements of the Implicator for some
cases as a result of this change.
name old time/op new time/op delta
Implicator/single-exact-match-16 81.2ns ± 1% 77.8ns ± 1% -4.24% (p=0.008 n=5+5)
Implicator/single-inexact-match-16 916ns ± 1% 914ns ± 0% ~ (p=0.254 n=5+5)
Implicator/range-inexact-match-16 2.40µs ± 0% 2.46µs ± 1% +2.56% (p=0.008 n=5+5)
Implicator/single-exact-match-extra-filters-16 313ns ± 0% 305ns ± 1% -2.74% (p=0.008 n=5+5)
Implicator/single-inexact-match-extra-filters-16 3.85µs ± 1% 3.85µs ± 1% ~ (p=0.952 n=5+5)
Implicator/multi-column-and-exact-match-16 90.0ns ± 1% 84.3ns ± 1% -6.29% (p=0.008 n=5+5)
Implicator/multi-column-and-inexact-match-16 1.99µs ± 0% 2.02µs ± 1% +1.53% (p=0.016 n=4+5)
Implicator/multi-column-or-exact-match-16 82.3ns ± 3% 78.1ns ± 1% -5.10% (p=0.008 n=5+5)
Implicator/multi-column-or-exact-match-reverse-16 1.72µs ± 1% 1.74µs ± 2% ~ (p=0.397 n=5+5)
Implicator/multi-column-or-inexact-match-16 2.23µs ± 2% 2.31µs ± 3% +3.74% (p=0.008 n=5+5)
Implicator/and-filters-do-not-imply-pred-16 3.61µs ± 1% 3.63µs ± 4% ~ (p=0.548 n=5+5)
Implicator/or-filters-do-not-imply-pred-16 916ns ± 2% 905ns ± 2% ~ (p=0.095 n=5+5)
Implicator/many-columns-exact-match10-16 305ns ± 1% 278ns ± 1% -8.97% (p=0.008 n=5+5)
Implicator/many-columns-inexact-match10-16 12.9µs ± 1% 12.9µs ± 1% ~ (p=0.841 n=5+5)
Implicator/many-columns-exact-match100-16 21.3µs ± 1% 19.2µs ± 2% -9.94% (p=0.008 n=5+5)
Implicator/many-columns-inexact-match100-16 502µs ± 1% 503µs ± 1% ~ (p=0.421 n=5+5)
Release justification: Low risk update to new functionality, partial
indexes.
Release note: None
53510: kv: split rate limits and metrics for read and write requests r=nvanbenschoten a=nvanbenschoten
Fixes #53483.
This commit splits the existing request rate limit into two categories, read requests and write requests. Experimentation has shown that the fixed cost of a request is dramatically different between these two categories, primarily because write requests need to go through Raft while read requests do not. By splitting the limits and metrics along this dimension, we expect to be able to more accurately model the cost of KV traffic and more effectively tune rate limits.
In making the split, the commit replaces the existing metric:
```
kv.tenant_rate_limit.requests_admitted
```
with the following two new metrics:
```
kv.tenant_rate_limit.read_requests_admitted
kv.tenant_rate_limit.write_requests_admitted
```
The commit also replaced the existing two settings:
```
kv.tenant_rate_limiter.requests.rate_limit
kv.tenant_rate_limiter.request.burst_limit
```
with the following four new settings:
```
kv.tenant_rate_limiter.read_requests.rate_limit
kv.tenant_rate_limiter.read_requests.burst_limit
kv.tenant_rate_limiter.write_requests.rate_limit
kv.tenant_rate_limiter.write_requests.burst_limit
```
Release justification: Low-risk, high benefit change.
53517: opt: better error message for multiple arbiters with CONFLICT DO UPDATE r=mgartner a=mgartner
This commit improves the error message and adds telemetry for INSERT ON
CONFLICT DO UPDATE statements that find multiple matching arbiter
indexes.
Release justification: This is a low-risk update to new functionality,
partial indexes.
Release note: None
53544: roachtest: skip and reown tests r=nvanbenschoten,petermattis a=tbg
cc @petermattis, let me know if you're opposed to owning the disk-stalled roachtests. I saw while triaging that you and Bilal were discussing fixing the charybdefs setup and I also saw talk about disk stall work in the weekly notes.
Release justification: testing only
Release note: None
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit fixes a panic that is produced when trying to prove
implication with a disjunction of two-variable comparisons. The panic
only occurs when the comparisons are equivalent, but not exactly the
same. For example:
Release note: None