Skip to content

Add query comment to ignore max memory row limit#6430

Merged
deepthi merged 2 commits intovitessio:masterfrom
tinyspeck:ignore-max-memory-rows
Jul 23, 2020
Merged

Add query comment to ignore max memory row limit#6430
deepthi merged 2 commits intovitessio:masterfrom
tinyspeck:ignore-max-memory-rows

Conversation

@spark4
Copy link
Copy Markdown
Contributor

@spark4 spark4 commented Jul 9, 2020

Overview

We currently have warn and max thresholds for the number of rows that can be returned by a given query. However, there exists no mechanism for ignoring the max_memory_rows limit for a given query when this flag is enabled. This PR introduces a new query comment directive that will allow us to ignore the max_memory_rows limit, similar to that for max_payload_size in this PR.

Relevant Issue: #6213
Relevant PR: #6143

Implementation

  1. Define a new comment directive DirectiveIgnoreMaxMemoryRows.
  2. Extract the DirectiveIgnoreMaxMemoryRows value at the executor step and pass that into vcursor for later validation.
  3. Skip MaxMemoryRows validation if DirectiveIgnoreMaxMemoryRows is set.

Testing

Testing was done via unit testing and manual testing. The following manual test cases were covered:

  • Inserts/updates/deletes continue to work as expected
  • Selects within the max_memory_rows limit continue to work as expected
  • Selects that exceed the max_memory_rows limit fail as expected
  • Selects that exceed the max_memory_rows limit that have the DirectiveIgnoreMaxMemoryRows query comment will succeed as expected

@spark4 spark4 requested a review from sougou as a code owner July 9, 2020 20:54
@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Jul 18, 2020

@spark4 can you resolve the conflicts?

@deepthi deepthi requested review from harshit-gangal and systay July 18, 2020 20:45
@spark4 spark4 force-pushed the ignore-max-memory-rows branch from b5e1167 to ca3cbed Compare July 20, 2020 18:32
spark4
spark4 commented Jul 20, 2020
View reviewed changes
@spark4 spark4 force-pushed the ignore-max-memory-rows branch from ca3cbed to 666223d Compare July 20, 2020 18:48
deepthi
deepthi reviewed Jul 21, 2020
View reviewed changes
systay
systay reviewed Jul 22, 2020
View reviewed changes
systay
systay reviewed Jul 22, 2020
View reviewed changes
go/vt/vtgate/engine/join_test.go Outdated
Copy link
Copy Markdown
Collaborator

@systay systay Jul 22, 2020

Choose a reason for hiding this comment

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

nice expansion of the test cases

systay
systay reviewed Jul 22, 2020
View reviewed changes
@spark4
add query comment to ignore max memory row limit
e7f01e8 
Signed-off-by: Serry Park <me@serry.co>
@spark4 spark4 force-pushed the ignore-max-memory-rows branch from 666223d to 09cbcb1 Compare July 23, 2020 00:05
spark4
spark4 commented Jul 23, 2020
View reviewed changes
go/vt/vtgate/executor.go Outdated
Copy link
Copy Markdown
Contributor Author

@spark4 spark4 Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Calling this out since this may be easy to miss and the other comment is no longer relevant -- setting this explicitly to true since this is being called within StreamExecute

@spark4
update tests, update vcursor within getPlan(), remove extraneous sqlp…
bce9c2a 
…arser calls

Signed-off-by: Serry Park <me@serry.co>
@spark4 spark4 force-pushed the ignore-max-memory-rows branch from 09cbcb1 to bce9c2a Compare July 23, 2020 00:32
deepthi
deepthi approved these changes Jul 23, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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

@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Jul 23, 2020

Unit tests are failing as you can see here: https://github.com/vitessio/vitess/actions?query=branch%3Aignore-max-memory-rows
A few more changes are needed in legacy_scatter_conn_test.go. We can merge once those are fixed.

@spark4
Copy link
Copy Markdown
Contributor Author

spark4 commented Jul 23, 2020

@deepthi the unit test failures should have been resolved by the last commit. Is there an easy way to restart tests that have been stuck in queued? It looks like some have been stuck in this state since last night.

@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Jul 23, 2020

@deepthi the unit test failures should have been resolved by the last commit. Is there an easy way to restart tests that have been stuck in queued? It looks like some have been stuck in this state since last night.

You can click on Details and Re-run. Let me know if you don't have the permissions to re-run and I can fix that.
I just re-ran unit 🤞

@deepthi deepthi merged commit d37b51a into vitessio:master Jul 23, 2020
@spark4 spark4 mentioned this pull request Jul 23, 2020
Merged
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
@ameetkotian ameetkotian mentioned this pull request Aug 20, 2020
Merged
@harshit-gangal harshit-gangal mentioned this pull request Nov 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@systay systay systay left review comments

@sougou sougou Awaiting requested review from sougou

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal

+1 more reviewer

@deepthi deepthi deepthi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@spark4 @deepthi @systay