Skip to content
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

vtgate sql: enforce a max row count limit #4930

Merged
merged 1 commit into from
Jun 16, 2019

Conversation

sougou
Copy link
Contributor

@sougou sougou commented Jun 14, 2019

I've been claiming that vtgate had a max row count limit.
It turns out that I never actually implemented it.
So, here's the implementation.

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

I've been claiming that vtgate had a max row count limit.
It turns out that I never actually implemented it.
So, here's the implementation.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
@sougou sougou requested review from deepthi and rafael June 14, 2019 15:37
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sougou sougou merged commit 640588c into vitessio:master Jun 16, 2019
@sougou sougou deleted the ss-max-rows branch June 16, 2019 03:55
Copy link
Member

@demmer demmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backwards compatibility with production deployments this needs to have a much higher default, ideally infinite but at least 1 million.

As is this would break some slack workloads if deployed into production like this. Of course we can be diligent and override this default as part of the deployment, but as a general rule changes like this one that might affect some production deployments should really be disabled by default.

@sougou
Copy link
Contributor Author

sougou commented Jun 16, 2019

This was loosely based the fact hat vttablets already limit the max count to 10K. Are there workloads that pull under 10K from each vttablet, that still add up to over 30K?

@zmagg
Copy link
Contributor

zmagg commented Jun 17, 2019

This was loosely based the fact hat vttablets already limit the max count to 10K. Are there workloads that pull under 10K from each vttablet, that still add up to over 30K?

Definitely. We have some scatter queries that absolutely hit that. I just checked. :)

@sougou
Copy link
Contributor Author

sougou commented Jun 17, 2019

Can you check if they were using OLTP, and not OLAP? Note that it's a bad idea to do it this way. We've always said that one should use OLAP if fetching more than 10K rows.

The main motivation behind this limit is to prevent site-wide outage from a rogue query. If it pulls too many rows, it can OOM all the vtgates. Ideally, we should have a global limit. But that's hard to track and enforce. So, this is some kind of a start.

Will 100K be reasonable?

@demmer
Copy link
Member

demmer commented Jun 17, 2019

Slack doesn't use OLAP anywhere.

Yes we realize there are probably better ways to do this... but in the global scheme of tradeoffs what we have is the least-bad.

systay pushed a commit that referenced this pull request Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants