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

fix: copy names from mmapped memory before closing iterator #21999

Closed
wants to merge 2 commits into from

Conversation

davidby-influx
Copy link
Contributor

@davidby-influx davidby-influx commented Jul 30, 2021

Ensure that when looking up measurement names all
pointers to mmapped memory are copied to heap memory
before the mmapped iterators are closed.

closes #22000

@@ -1330,7 +1330,8 @@ func (is IndexSet) MeasurementNamesByExpr(auth query.FineAuthorizer, expr influx
if err != nil {
return nil, err
}
return slices.CopyChunkedByteSlices(names, 1000), nil
// Requires that names is a copy of the names from the mmapped file
return names, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems quite different than before because we now allocate more memory for this part:

		case influxql.OR, influxql.AND:
			lhs, err := is.measurementNamesByPredicate(auth, e.LHS)
			if err != nil {
				return nil, err
			}
			rhs, err := is.measurementNamesByPredicate(auth, e.RHS)
			if err != nil {
				return nil, err
			}

Also what about the Expr versions - e.g. measurementNamesByTagFilter ?

Copy link
Contributor

@lesam lesam left a comment

Choose a reason for hiding this comment

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

See comments

Ensure that when looking up measurement names all
pointers to mmapped memory are copied to heap memory
before the mmaped iterators are closed.

Closes influxdata/EAR#2371
@davidby-influx davidby-influx deleted the DSB_infor_panic branch August 4, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copying measurement names from mmapped memory can cause a panic
2 participants