-
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
what exactly does disallow_full_table_scans do? / is it working correctly? #70795
Comments
Hello, I am Blathers. I am here to help you get the issue triaged. Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here. I have CC'd a few people who may be able to assist you:
If we have not gotten back to your issue within a few business days, you can try the following:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Thanks for opening the issue, @davepacheco. No need to apologize. The initial implementation of
This means that
The optimizer produces this plan because (currently) it doesn't know about
Yes, sometimes the optimizer chooses to use a full scan for tiny tables, even if
Yes, this is the current implementation. I think the first one worked because
This is what we probably should be doing, but are not.
You've summed it up well. I might have some good news, we recently changed the implementation a little so that now:
(This change will be released in 21.1.10 with This means that |
Thanks for the quick and detailed reply!
Ah, you're right. I thought I had tried running this without EXPLAIN and seen it execute the query. But that's not what I have in my notes and I can't reproduce it, so I'm probably misremembering.
For what it's worth,
Cool! Yes, this sounds like an improvement in the ability to identify at runtime that a query's going to run badly before you run it. I don't think it quite captures what I'm hoping to do. The case I'd love to be able to catch is where we've got a small database (because we're in development) and we accidentally introduce a query into the application without an associated index to make that query run quickly. With the updated implementation, we won't get a false positive, but we also won't get a true positive because the tables are small. One improvement though is we can load the tables up with at least
Sounds good. From my understanding, that'd be necessary to reliably identify queries that require table scans. I know we can't totally solve the problem of catching all pathological queries. But any assistance from the database in identifying queries that will fall apart at scale (before you get to that scale) would be invaluable for application developers. I'm excited to see a bunch of work around this. (Out of curiosity, is there other tooling in this area, or plans for it? Another example I can think of is when we have an index, but we forget to sort the results in index order, so the database can't just walk it in sorted order but needs to assemble the results first, then sort.) |
I wrote:
Thinking about that more: I think we still have the same problem. The number of rows we'd need to insert in each table is the greater of We could use this mechanism to fail quickly at runtime instead of taking arbitrarily long. That might be an improvement. But for our app, we may want that to be latency-based anyway. |
It's true, this latter number is difficult to know ahead of time, and might even change depending on the table or the version of CRDB. (Anecdotally it's probably always below 1000 rows.) Good news is that in 21.1 the optimizer is less likely to pick full scans for small tables so that should reduce the chance of false positives.
We did recently add some guardrails which:
(These should also be released in 21.1.10 and 21.2.0.) There are plans for more. |
Thanks! |
Notes: @rytaft pointed out that we could use a "hints"-like mechanism to teach the optimizer about this. |
I apologize if this is the wrong venue for this. I can't tell if this behavior is buggy or just not what I expected.
Describe the problem
The
disable_full_table_scans
option seems so close to what I'm looking for, but not quite right.My team is building an application atop CockroachDB with the intent of scaling horizontally. Naturally, we want to avoid using queries that require full scans on tables that might grow arbitrarily large. We're trying to figure out the best way to avoid accidentally introducing such a query. Ideally, we'd like to find out early during development that a query is problematic, not when we go to do large-scale testing later. I was excited to find
disallow_full_table_scans=off
because I thought it might help us identify queries that require a table scan. But it doesn't seem to work quite the way I'd expect in two ways.First, it seems to allow some full scans? Take this query:
There are three
FULL SCAN
nodes there. Why is that allowed withdisallow_full_table_scans=on
?Note that when I ran this, all three of these tables were empty:
If you want the schema to reproduce this:
I suspected that maybe this was allowed because the query planner chose to use a full scan even when it wasn't needed (because it knows the table is tiny -- empty, really). However, I ran into a different case where the query planner chose to do a table scan when an index was available, and that query was disallowed by
disallow_full_table_scans
. This was the second thing I found surprising.This time, I'm using this table:
Again, starting from an empty table:
I was surprised that this query didn't work with disallow_full_table_scans:
It's not surprising that the query planner would choose to do a table scan here, since the table is empty, and sure enough:
So I inserted 500-1000 records using:
and sure enough I get a new plan that uses the index and I can run it even with disallow_full_table_scans:
More on this below, but the fact that this behavior depends on which plan the query planner chooses makes it a lot less useful for what we're trying to do.
To Reproduce
Set up a CockroachDB cluster with default configuration.
Use the above
CREATE TABLE
statements to set up the schema, then run the aboveEXPLAIN
and observe the query plan. (If you need the example where a query was disallowed, even when an index was available, let me know.) Observe it doing a full scan. You can do this again with the second example and observe it disallow the query.Expected behavior
In the first case, given that CockroachDB chose a full scan, I expected it to disallow the query. Instead, it executed the query.
Alternatively, if the expected behavior of
disallow_full_table_scans
is that it only disallows queries that can only be completed with a full scan, then I expected my second query to not produce an error.Additional data / screenshots
See above.
Environment:
Server:
Client:
cockroach sql
:Additional context
Between these two examples, I can't tell what the exactly behavior of
disallow_full_table_scans
is supposed to be. Is it supposed to change nothing about the query plan and simply deny queries that get planned with a full scan? If so, why did the first one work? Or is it supposed to encourage the query planner to avoid a table scan and fail only if it couldn't? If so, why did the second query get denied?Our real goal is to avoid building our application using queries that will clearly require table scans as tables get large. So it's tempting to set
disallow_full_table_scans=on
in the application. That way, if we have good test coverage, we can have confidence that we're not going to run into scaling limitations due to table scans. (Obviously, this isn't a substitute for actual scale testing, but anything we can do to catch this early in development would be a big help.) I'm also excited by #67964, since that would allow us to set this only on tables that we know will be large.But it doesn't seem like this will work. Because
disallow_full_table_scans=on
disallows queries for which the planner happens to have chosen a table scan, not just those that require a table scan, it produces false positives any time a table is small.Thanks for reading this far. If there's a better way to achieve what we're trying to do, please let me know!
The text was updated successfully, but these errors were encountered: