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.

Sign up for GitHub

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

Jump to bottom

sql: bump workmem size to avoid sqllite flakes on index/1000 tests #90095

Merged
merged 1 commit into from
Oct 18, 2022
Merged

sql: bump workmem size to avoid sqllite flakes on index/1000 tests #90095

merged 1 commit into from
Oct 18, 2022

Conversation

cucaroach
Copy link
Contributor

Below ~70k these tests would flake like so:

pq: streamer budget: memory budget exceeded: 38112 bytes requested, 57344 currently allocated, 76801 bytes in budget

Fixes: #89635

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cucaroach cucaroach requested a review from a team October 17, 2022 21:08
@michae2
Copy link
Collaborator

michae2 commented Oct 17, 2022

I'm worried this will cause logic tests to stop exercising the various operators that spill to disk. Could we just exempt this particular test?

@cucaroach
sql: bump workmem size to avoid sqllite flakes on index/1000 tests
ba4e08f 
Below ~70k these tests would flake like so:

```
pq: streamer budget: memory budget exceeded: 38112 bytes requested, 57344 currently allocated, 76801 bytes in budget
```

Fixes: cockroachdb#89635

Release note: None
cucaroach
cucaroach commented Oct 17, 2022
View reviewed changes
Copy link
Contributor Author

@cucaroach cucaroach left a comment

Choose a reason for hiding this comment

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

Well I think its a whole slew of the "1000" tests but easy enough to limit to the sqlite "Big" tests.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

DrewKimball
DrewKimball approved these changes Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

michae2
michae2 approved these changes Oct 17, 2022
View reviewed changes
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Thank you!

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @cucaroach)

@cucaroach
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Oct 18, 2022

Build succeeded:

@craig craig bot merged commit b2b8d10 into cockroachdb:master Oct 18, 2022
@blathers-crl blathers-crl bot mentioned this pull request Oct 18, 2022
Merged
@cucaroach
Copy link
Contributor Author

@mgartner should I backport this to 22.2.0 as well?

@mgartner
Copy link
Collaborator

It's a test-only change so that seems fine to me. 👍

@cucaroach cucaroach mentioned this pull request Oct 18, 2022
Merged
@yuzefovich
Copy link
Member

My guess is that the reuse of the BatchTruncationHelper between Enqueue calls (added in 75a18a8) that made us also perform the memory accounting for that struct is what rarely would push us over the budget. I don't think it should be happening when workmem limit is in MBs range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DrewKimball DrewKimball DrewKimball approved these changes

@michae2 michae2 michae2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@cucaroach @cockroach-teamcity @michae2 @mgartner @yuzefovich @DrewKimball