New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: Do not panic when using an unexpected cursor type #26165

Open

devanbenz wants to merge 15 commits into main-2.x from db/panic-at-the-cursor

devanbenz commented Mar 19, 2025

This PR is used to alleviate the erroneous panic we are seeing corresponding with #26142 There should not be a panic and instead we should be throwing an error.


          fix: Do not panic when using an unexpected cursor type

336f728

This PR is used to alleviate the erroneous panic we are seeing
corresponding with #26142.
There should not be a panic and instead we should be throwing an error.

davidby-influx requested changes

View reviewed changes

Contributor

davidby-influx left a comment

Still some panics left in the code, and error messages are cryptic.

storage/reads/array_cursor.gen.go Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg:  fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

unreachable is a bad error message. Help the user out with something like unsupported array cursor type

storage/reads/array_cursor.gen.go Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg:  fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

As above for error message.

storage/reads/array_cursor.gen.go Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg:  fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

As above for error message

storage/reads/array_cursor.gen.go Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg:  fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

As above.

storage/reads/array_cursor.gen.go

-              		panic(fmt.Sprintf("unsupported for aggregate min: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg:  fmt.Sprintf("unsupported for aggregate min: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

Yes, a better error message!

storage/reads/array_cursor.gen.go.tmpl Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg: fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

Better error message, see above.

storage/reads/array_cursor.gen.go.tmpl Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg: fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

Better error message, see suggestions above.

storage/reads/array_cursor.gen.go.tmpl Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg: fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

Better message

storage/reads/array_cursor.gen.go.tmpl Outdated

-              		panic(fmt.Sprintf("unreachable: %T", cur))
+              		return nil, &errors2.Error{
+              			Code: errors2.EInvalid,
+              			Msg: fmt.Sprintf("unreachable: %s", arrayCursorType(cur)),

Contributor

davidby-influx Mar 19, 2025

Better error message.

storage/reads/array_cursor.go

               	case datatypes.Aggregate_AggregateTypeMax:
-              		return newWindowMaxArrayCursor(cursor, window), nil
+              		return newWindowMaxArrayCursor(cursor, window)
               	case datatypes.Aggregate_AggregateTypeMean:
               		return newWindowMeanArrayCursor(cursor, window)
               	default:

Contributor

davidby-influx Mar 19, 2025

Don't panic in this default statement

Contributor

davidby-influx Mar 19, 2025

See also panic on line 143. We should not panic for bad flux.


          fix: Remove various panic's

91f532d

davidby-influx assigned devanbenz

davidby-influx added kind/bug area/storage area/flux team/edge labels


          fix: remove panics

6a1cc9f

devanbenz marked this pull request as draft

March 20, 2025 15:54

Author

devanbenz commented Mar 20, 2025

Converting to draft as I make changes here.


          fix: Bubble up errors for cursors

c8ee812

devanbenz force-pushed the db/panic-at-the-cursor branch from 72977bc to c8ee812 Compare

March 20, 2025 16:42

devanbenz added 5 commits

March 20, 2025 12:01


          feat: Add error handling to flux table advanceCursor

1f8fa62


          feat: bubble up error for handleRead

c050d9d


          feat: Modify return values so we return r.err for cursors

ced6938


          feat: do not shadow cur!

fe208e5


          feat: modify windowAggregateResultSet to use c.err for Cursor return

a6ccc8b

davidby-influx reviewed

View reviewed changes

storage/flux/reader.go Outdated

               		gc    storage.GroupCursor
               		cur   cursors.Cursor
               		table storageTable
+              		err   error

Contributor

davidby-influx Mar 24, 2025

Should be a separate declaration, because it is not a resource that much be closed if not nil on return.

davidby-influx reviewed

View reviewed changes

storage/reads/group_resultset.go

@@ @@ -121,7 +121,10 @@ func (g *groupResultSet) Next() GroupCursor { @@
               // the time range of the query.
               func (g *groupResultSet) seriesHasPoints(row *SeriesRow) bool {
               	// TODO(sgc): this is expensive. Storage engine must provide efficient time range queries of series keys.
-              	cur := g.arrayCursors.createCursor(*row)
+              	cur, err := g.arrayCursors.createCursor(*row)

Contributor

davidby-influx Mar 24, 2025

So this is a lost error; unreported, never makes it out of this method scope. I'm not sure how to handle this, but because there is also a panic on line 148, you may need to return a (bool, error) from this method to get this error and the (soon-to-be-eliminated) panic out to the user.

davidby-influx requested changes

View reviewed changes

Contributor

davidby-influx left a comment

A few notes on this draft.

storage/reads/group_resultset.go

-              	cur = c.arrayCursors.createCursor(seriesRow)
+              	cur, err = c.arrayCursors.createCursor(seriesRow)
+              	if err != nil {
+              		return nil, err

Contributor

davidby-influx Mar 24, 2025

The other returns are (cur, err). Should we hand back the cur here (in case there is some clean-up necessary on it)? Or are we certain it is always nil if createCursor fails (which would be better).

Author

devanbenz Mar 24, 2025

cur here will be nil if the createCursor call fails.
This switch statement gets called during createCursor

func newWindowAggregateArrayCursor(ctx context.Context, agg *datatypes.Aggregate, window interval.Window, cursor cursors.Cursor) (cursors.Cursor, error) {
	if cursor == nil {
		return nil, nil
	}

	switch agg.Type {
	case datatypes.Aggregate_AggregateTypeCount:
		return newWindowCountArrayCursor(cursor, window)
	case datatypes.Aggregate_AggregateTypeSum:
		return newWindowSumArrayCursor(cursor, window)
	case datatypes.Aggregate_AggregateTypeFirst:
		return newWindowFirstArrayCursor(cursor, window)
	case datatypes.Aggregate_AggregateTypeLast:
		return newWindowLastArrayCursor(cursor, window)
	case datatypes.Aggregate_AggregateTypeMin:
		return newWindowMinArrayCursor(cursor, window)
	case datatypes.Aggregate_AggregateTypeMax:
		return newWindowMaxArrayCursor(cursor, window)
	case datatypes.Aggregate_AggregateTypeMean:
		return newWindowMeanArrayCursor(cursor, window)
	default:
		return nil, &errors2.Error{
			Code: errors2.EInvalid,
			Msg:  fmt.Sprintf("unsupported window aggregate cursor: %s", agg.Type),
		}
	}
}

Each of the newWindow...Cursor calls also has a switch statement that will bubble up nil for the cursor value as well.

storage/reads/store_test.go

               	for rs.Next() {
               		fmt.Fprint(wr, "series: ")
               		tagsToString(wr, rs.Tags())
-              		cur := rs.Cursor()
+              		cur, _ := rs.Cursor()

Contributor

davidby-influx Mar 24, 2025

Hmmm, not sure how important this is to fix....

At least add a comment.

Author

devanbenz Mar 24, 2025

Sounds good can do - I was actually planning on going back and adding in this error while I work on some tests for these changes to test capturing errors.

Author

devanbenz Mar 26, 2025

Taking a look at this code it looks like the tests that resultSetToString gets called in all have a nil cursor so the error will never bubble up in these tests. The continue here

influxdb/storage/reads/store_test.go

Line 125 in e862501

continue

is called in every test. I've added a comment.

v1/services/storage/store.go

               	rs := reads.NewFilteredResultSet(ctx, mqAttrs.start, mqAttrs.end, cur)
               	for rs.Next() {
               		func() {
-              			c := rs.Cursor()
+              			c, err := rs.Cursor()

Contributor

davidby-influx Mar 24, 2025

Return the error out of this anonymous function; the top level function has an error return, so the plumbing is only local.

The rs.Next loop should halt on error (as opposed to continuing as is should for c == nil when there is no data.

v1/services/storage/store.go

               	rs := reads.NewFilteredResultSet(ctx, mqAttrs.start, mqAttrs.end, cur)
               	for rs.Next() {
               		func() {
-              			c := rs.Cursor()
+              			c, err := rs.Cursor()

Contributor

davidby-influx Mar 24, 2025

Same comments as in the similar code above.

v1/services/storage/store.go

               	rs := reads.NewFilteredResultSet(ctx, start, end, cur)
               	for rs.Next() {
               		func() {
-              			c := rs.Cursor()
+              			c, err := rs.Cursor()
+              			if err != nil {

Contributor

davidby-influx Mar 24, 2025

As above.

devanbenz added 3 commits

March 24, 2025 14:05


          feat: Modify seriesHasPoints to include error returns for groupResultSet

e627dec

move error var declaration outside of closed vars in flux reader


          feat: Modify lambdas to return err values

a42e611


          feat: Working on adding tests

6366b01

devanbenz mentioned this pull request

panic: unsupported for aggregate min: *reads.stringMultiShardArrayCursor #26142

Open


          feat: cursor is always nil, don't use error

6a701e8

devanbenz marked this pull request as ready for review

March 26, 2025 20:41

devanbenz added 2 commits

March 26, 2025 15:45


          feat: removing more panics from cursor switches

dc37295


          Merge branch 'main-2.x' into db/panic-at-the-cursor

9a9c1e6

devanbenz requested a review from davidby-influx

March 26, 2025 22:13

# for free to join this conversation on GitHub. Already have an account? # to comment

Labels

area/flux area/storage kind/bug team/edge