opt: don't apply huge cost for full scans of virtual tables #123780
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
yuzefovich
added
backport-23.1.x
yuzefovich
force-pushed
the
full-scan-vt
branch
3 times, most recently
from
b7cd87c
to
e44bc50
Compare
This commit fixes - what I believe is - an oversight where we applied the huge cost to the plans with full scans of virtual tables when `disallow_full_table_scans` variable is set. The intent behind this penalty is to make plans with full scans prohibitively expensive so that other plans (without full scans) would be preferred, and if there is no other plan, then the query would error out. Implementation of this approach was incomplete for virtual tables: namely, we'd apply the cost penalty, but then we would not actually error out the query. Additionally, given that we don't have stats on virtual tables, we would apply the cost penalty to all full scans of virtual tables. As a result, we would either pick a plan without the full scan (that might actually be worse) or would allow the plan with the full scan to get executed still. We would also favor plans with fewer full scans of virtual tables (which might also be worse than had we not applied the cost penalty). This commit adjusts the code to not apply the cost penalty to full scans of virtual tables. This change is gated based on the new session variable `optimizer_apply_full_scan_penalty_to_virtual_tables` which defaults to `false` on master. We can discuss on the backport PRs whether it should be default to `true` there to require users to opt in for this PR to have an effect. Release note: None
There was a problem hiding this comment.
Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @yuzefovich)
-- commits line 28 at r1:
[nit] Should we add a release note since the issue this is fixing has an O-support label?
|
I will look today. |
There was a problem hiding this comment.
Reviewable status:
Previously, DrewKimball (Drew Kimball) wrote…
[nit] Should we add a release note since the issue this is fixing has an O-support label?
My thinking was that the default value for disallow_full_table_scans is false, so this could be considered a bug fix to a non-default configuration, in which I thought we generally don't include the release notes. (I did see that we did include some release notes in the past for this feature though.) For the single customer that ran into this we'll be communicating directly, so I don't think they care whether we include it or not.
I can add a release note if we think it's worth it.
There was a problem hiding this comment.
Reviewable status:
I don't think they care whether we include it or not.
If that's the case, I'm fine with just leaving it out.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
foo
Reviewed 1 of 17 files at r1.
Reviewable status:
pkg/sql/opt/xform/coster.go line 791 at r1 (raw file):
// TODO(#123783): once we start rejecting plans with full scans of> // virtual tables, we should apply the cost penalty here.
Do we want to reject plans with full scans of virtual tables? I thought according to #122708 we decided to not apply disallow_full_table_scans to virtual tables? And it seems like this PR also fixes that issue by removing hugeCost?
I find the purpose of optimizer_apply_full_scan_penalty_to_virtual_tables a little muddy: it brings back the old behavior, but the old behavior was not correct as you point out in #123783. Would it make more sense to add a disallow_full_virtual_table_scans setting which, when enabled has both the hugeCost and the ContainsLargeFullTableScan flag, fixing 123783, and when disabled has neither, fixing 122708?
There was a problem hiding this comment.
sorry, ignore the foo 🤦
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/coster.go line 791 at r1 (raw file):
Do we want to reject plans with full scans of virtual tables?
This is the question that I don't know the answer to. I find this problem quite confusing - I believe that originally "disallow full table scans" feature was designed with user tables in mind, so we probably didn't consider how it should behave for virtual tables. The goal is for DB infra team to force their app developers think better about schema / query design in order to not introduce expensive queries (to protect stability of the cluster), and we use "full scan" or "large scan" as a proxy for being expensive. However, for virtual tables our users have no control over the schema; true, in some cases they could rewrite the queries to avoid full scans of vtables, but I'd guess it's usually not possible - this is due to CRDB missing virtual indexes (in case it is possible to implement those). Do we want to error out such queries without having any workaround? I'm not sure.
This thinking is why I decided to file (rather than fix it right away) #123783 so that we could discuss in case we want to have a way to prohibit full scans of vtables. The current approach allows us to fix the issue for the customer with low risk and have a backportable solution. As a follow-up, I plan to remove this session variable altogether on master (so that this behavior is unconditional), and then it'll be up to us to fix #123783 (where a new session variable would be warranted) separately, and we'll have the session var available on older releases (where I think we should consider defaulting to enabling this patch, and the session var would provide us an escape hatch in case there is a regression).
WDYT?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/coster.go line 791 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Do we want to reject plans with full scans of virtual tables?
This is the question that I don't know the answer to. I find this problem quite confusing - I believe that originally "disallow full table scans" feature was designed with user tables in mind, so we probably didn't consider how it should behave for virtual tables. The goal is for DB infra team to force their app developers think better about schema / query design in order to not introduce expensive queries (to protect stability of the cluster), and we use "full scan" or "large scan" as a proxy for being expensive. However, for virtual tables our users have no control over the schema; true, in some cases they could rewrite the queries to avoid full scans of vtables, but I'd guess it's usually not possible - this is due to CRDB missing virtual indexes (in case it is possible to implement those). Do we want to error out such queries without having any workaround? I'm not sure.
This thinking is why I decided to file (rather than fix it right away) #123783 so that we could discuss in case we want to have a way to prohibit full scans of vtables. The current approach allows us to fix the issue for the customer with low risk and have a backportable solution. As a follow-up, I plan to remove this session variable altogether on master (so that this behavior is unconditional), and then it'll be up to us to fix #123783 (where a new session variable would be warranted) separately, and we'll have the session var available on older releases (where I think we should consider defaulting to enabling this patch, and the session var would provide us an escape hatch in case there is a regression).
WDYT?
IIRC, the customer that ran into #122708 is the same that requested this feature originally. Perhaps we should reach out to them to see what they think with regards to prohibiting full scans of vtables?
There was a problem hiding this comment.
Reviewable status:
|
TFTRs! bors r+ |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 1bb9662 to blathers/backport-release-23.1-123780: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This commit fixes - what I believe is - an oversight where we applied the huge cost to the plans with full scans of virtual tables when
disallow_full_table_scansvariable is set. The intent behind this penalty is to make plans with full scans prohibitively expensive so that other plans (without full scans) would be preferred, and if there is no other plan, then the query would error out. Implementation of this approach was incomplete for virtual tables: namely, we'd apply the cost penalty, but then we would not actually error out the query. Additionally, given that we don't have stats on virtual tables, we would apply the cost penalty to all full scans of virtual tables.As a result, we would either pick a plan without the full scan (that might actually be worse) or would allow the plan with the full scan to get executed still. We would also favor plans with fewer full scans of virtual tables (which might also be worse than had we not applied the cost penalty). This commit adjusts the code to not apply the cost penalty to full scans of virtual tables.
This change is gated based on the new session variable
optimizer_apply_full_scan_penalty_to_virtual_tableswhich defaults tofalseon master. We can discuss on the backport PRs whether it should be default totruethere to require users to opt in for this PR to have an effect.Fixes: #122708.
Release note: None