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

release-20.2: sql: shrink SampleReservoir capacity on memory exhaustion #67059

Merged
merged 3 commits into from Jun 30, 2021

Conversation

michae2
Copy link
Collaborator

@michae2 michae2 commented Jun 30, 2021

Backport 3/3 commits from #65491.

/cc @cockroachdb/release

Backport note: execinfrapb changes were removed from the second commit to allow backporting without incrementing execinfrapb.Version. This means we are always using a hardcoded 200 as MinSampleSize instead of picking MinSampleSize based on MaxHistogramBuckets. For most stats this behavior is identical (MaxHistogramBuckets is usually 200). For stats on non-key columns MaxHistogramBuckets is 2, so when collecting those stats this will be slightly more strict about sample size than necessary. But it seems unlikely that we will hit the memory limit of 64 MiB at < 200 samples anyway.


sql, stats: shrink sampleAggregator capacity to match child capacity

To avoid bias, sampleAggregator must always use a capacity <= the
capacity of each child samplerProcessor feeding it. This was always
implicitly true before, as every samplerProcessor and sampleAggregator
used the same fixed capacity. But the next few commits will give
sampleProcessors (and sampleAggregators) the ability to dynamically
shrink capacity when out of memory, meaning we now sometimes need to
resize sampleAggregator to keep this invariant true.

(Resizing sampleAggregator really means resizing the underlying
SampleReservoir, so most of the changes are there.)

This commit does not yet change any behavior, because the capacities of
samplerProcessors are still static. Next few commits will change that.

Also factor the giant closure out of TestSampleAggregator into an
external function, to make table-driven testing easier.

Release note: None


sql, stats: shrink SampleReservoir capacity on memory exhaustion

Fixes: #62206

Instead of returning an error when out of memory, make
SampleReservoir.SampleRow dynamically reduce capacity and retry. Fewer
samples will result in a less accurate histogram, but less accurate is
probably still better than no histogram at all, up to a point.

If the number of samples falls below a minimum threshold, then give up
and disable histogram collection, as we were doing originally, rather
than using a wildly inaccurate histogram.

Also fix some memory accounting in SampleReservoir.copyRow.

Release note (performance improvement): continue to generate histograms
when table statistics collection reaches memory limits, instead of
disabling histogram generation.


sql, stats: shrink sampleAggregator capacity before histogram generation

SampleAggregator can also run out of memory when gathering the datums
for histogram generation. Push this datum gathering down into
SampleReservoir so that we can use the same dynamic capacity-shrinking
strategy as used in SampleRow to reclaim some memory and try again.

Release note: None

To avoid bias, sampleAggregator must always use a capacity <= the
capacity of each child samplerProcessor feeding it. This was always
implicitly true before, as every samplerProcessor and sampleAggregator
used the same fixed capacity. But the next few commits will give
sampleProcessors (and sampleAggregators) the ability to dynamically
shrink capacity when out of memory, meaning we now sometimes need to
resize sampleAggregator to keep this invariant true.

(Resizing sampleAggregator really means resizing the underlying
SampleReservoir, so most of the changes are there.)

This commit does not yet change any behavior, because the capacities of
samplerProcessors are still static. Next few commits will change that.

Also factor the giant closure out of TestSampleAggregator into an
external function, to make table-driven testing easier.

Release note: None
Fixes: cockroachdb#62206

Instead of returning an error when out of memory, make
SampleReservoir.SampleRow dynamically reduce capacity and retry. Fewer
samples will result in a less accurate histogram, but less accurate is
probably still better than no histogram at all, up to a point.

If the number of samples falls below a minimum threshold, then give up
and disable histogram collection, as we were doing originally, rather
than using a wildly inaccurate histogram.

Also fix some memory accounting in SampleReservoir.copyRow.

Release note (performance improvement): continue to generate histograms
when table statistics collection reaches memory limits, instead of
disabling histogram generation.
@michae2 michae2 requested a review from a team as a code owner June 30, 2021 07:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

SampleAggregator can also run out of memory when gathering the datums
for histogram generation. Push this datum gathering down into
SampleReservoir so that we can use the same dynamic capacity-shrinking
strategy as used in SampleRow to reclaim some memory and try again.

Release note: None
@michae2 michae2 requested review from a team and rytaft June 30, 2021 07:13
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r1, 8 of 8 files at r2, 4 of 4 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

@michae2
Copy link
Collaborator Author

michae2 commented Jun 30, 2021

TFTR!

@michae2 michae2 merged commit 0c0e0d0 into cockroachdb:release-20.2 Jun 30, 2021
@michae2 michae2 deleted the backport20.2-65491 branch June 30, 2021 16:16
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.

None yet

3 participants