ExecuteFetch: error on multiple result sets

[shlomi-noach]

Description

ExecuteFetch now returns an error when faced with multiple result sets. It should only be used when a single result set is expected. See #14948

The main change in this PR is in go/mysql/query.go. The rest of the PR is adaptation of many tests to the new logic, as well as fixing the logic of some tests, that were missing error responses due to the previous nature of ExecuteFetch behavior.

This PR should not be backported.

Related Issue(s)

Fixes #14948

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[shlomi-noach]

Question: should we iterate and consume all results?

[dbussink]

I think we must here, otherwise we leave the connection in an invalid state. And a subsequent query on the same connection would see the previous result here.

[shlomi-noach]

Done.

[dbussink]

@shlomi-noach are there other places in the code base where we pass in 0 incorrectly and do want to consume all the results?

[shlomi-noach]

I'm not sure. ExecuteFetchMulti potentially? But then, this bugs me, because we should be able to pass maxrows = 17 in any place, so why would the draining in ExecuteFetch necessarily have to use -1? And yet, it does, as per #14949 (comment). I'm not sure if this is again limited to stored procedure behavior. I don't think it is.

[shlomi-noach]

There's not other explicit c.ReadQueryResult(0, ...) call in the code, FWIW.

[shlomi-noach]

@harshit-gangal further edited the ExecuteFetch/drain logic to fix potential leaks, and consolidated the draining logic. I think we should be good now.

[shlomi-noach]

As mostly expected, we get a bunch of CI errors. We should start tackling them one by one.

[shlomi-noach]

I think it the procedure above that is corrupting the connection.

Let me look into that. It's not supposed to happen now that we drain the connection. If anything, it shows that what we're trying to fix here is a nasty bug.

[shlomi-noach]

I see the problem, and I have a simple patch to solve it, which is a bit of a cheat, and I'd like to understand this better.

As @harshit-gangal suggested, the reason the test was failing was that the connection had result sets from the previous (multi result-set) test. Which surprised me, because the very essence of this PR is to drain all result sets in ExecuteFetch, so how could there be any leftover result sets?

I found the the way QueryExecutor drains results is different than how ExecuteFetch drains results... And I'm not sure why the QueryExecutor technique works where ExecuteFetch does not. Is it related specifically to how stored procedures work?

The QueryExecutor way:

func (qre *QueryExecutor) drainResultSetOnConn(conn *connpool.Conn) error {
	more := true
	for more {
		qr, err := conn.FetchNext(qre.ctx, int(qre.getSelectLimit()), true)
		if err != nil {
			return err
		}
		more = qr.IsMoreResultsExists()
	}
	return nil
}

The ExecuteFetch way:

func (c *Conn) ExecuteFetch(query string, maxrows int, wantfields bool) (result *sqltypes.Result, err error) {
	var more bool
	result, more, err = c.ExecuteFetchMulti(query, maxrows, wantfields)
	if more {
		// Multiple results are unexpected. Prioritize this "unexpected" error over whatever error we got from the first result.
		err = errors.Join(ErrExecuteFetchMultipleResults, err)
	}
	// Even though we do not allow multiple result sets, we still prefer to drain them so as to clean the connection, as well as
	// exhaust any further possible error.
	for more {
		var moreErr error
		_, more, _, moreErr = c.ReadQueryResult(0, false)
		if err != nil {
			err = errors.Join(err, moreErr)
		}
	}
	return result, err
}

My solution, BTW, which is a bit of a cheat, is to close the connection when multipel result sets are found, like so:

	qr, err := qre.execDBConn(conn.Conn, sql, true)
	if errors.UnwrappedIs(err, mysql.ErrExecuteFetchMultipleResults) {
		conn.Close()
		return nil, vterrors.New(vtrpcpb.Code_UNIMPLEMENTED, "Multi-Resultset not supported in stored procedure")
	}

It works, but I suspect there's better way to make this work.

[shlomi-noach]

Found it! The difference was that ExecuteFetch was running:

_, more, _, moreErr = c.ReadQueryResult(0, false)

Using 0 maxrows. Which means some reault rows were left in the pipe. When changing to -1 (unlimited) the problem goes away.

[shlomi-noach]

Good to review!

[dbussink]

@shlomi-noach is the 0 here correct or could that result in a similar issue?

[shlomi-noach]

It's correct, because the first query runs through ReadQueryResult which drains the results. Then, ExecuteFetchMultiDrain actively drains the results of the remaining queries. What;'s important here is that we don't do double-draining on the same result, which is what happened previously and was fixed both my my change and then by @harshit-gangal 's change on top.

[shlomi-noach]

We can set -1 for visual consistency with other calls, too. That would be fine.

[dbussink]

Isn’t 0 then much more efficient since we can avoid loading a whole bunch of data and allocating objects for it which we then drop here?

And does that imply we should be using 0 more in the case where we only care about if we get an error or not and not the actual result?

Dunno if we have many more of those cases in the code base though.

[shlomi-noach]

I'm trying to make this work but keep getting the double-drain problem. @harshit-gangal is there a way for us to use 0 so that we don't read excessive rows into memory, and still drain correctly?

[shlomi-noach]

Hmmm actually I still have errors.

[shlomi-noach]

OK, this passes tests:

diff --git a/go/mysql/query.go b/go/mysql/query.go
index 18e9872752..d0f68ec35c 100644
--- a/go/mysql/query.go
+++ b/go/mysql/query.go
@@ -39,6 +39,13 @@ var (
        ErrExecuteFetchMultipleResults = vterrors.Errorf(vtrpc.Code_INTERNAL, "unexpected multiple results. Use ExecuteFetchMulti instead.")
 )

+const (
+       // Use as `maxrows` in `ExecuteFetch` and related functions, to indicate no rows should be fetched.
+       // This is different than specifying `0`, because `0` means "expect zero results", while this means
+       // "do not attempt to read any results into memory".
+       FETCH_NO_ROWS = math.MinInt
+)
+
 //
 // Client side methods.
 //
@@ -322,7 +329,7 @@ func (c *Conn) ExecuteFetch(query string, maxrows int, wantfields bool) (result
 // caring for any results. The function returns an error if any of the statements fail.
 // The function drains the query results of all statements, even if there's an error.
 func (c *Conn) ExecuteFetchMultiDrain(query string) (err error) {
-       _, more, err := c.ExecuteFetchMulti(query, 0, false)
+       _, more, err := c.ExecuteFetchMulti(query, FETCH_NO_ROWS, false)
        return c.drainMoreResults(more, err)
 }

@@ -331,7 +338,7 @@ func (c *Conn) ExecuteFetchMultiDrain(query string) (err error) {
 func (c *Conn) drainMoreResults(more bool, err error) error {
        for more {
                var moreErr error
-               _, more, _, moreErr = c.ReadQueryResult(-1, false)
+               _, more, _, moreErr = c.ReadQueryResult(FETCH_NO_ROWS, false)
                err = errors.Join(err, moreErr)
        }
        return err
@@ -451,6 +458,9 @@ func (c *Conn) ReadQueryResult(maxrows int, wantfields bool) (*sqltypes.Result,
                if err != nil {
                        return nil, false, 0, sqlerror.NewSQLError(sqlerror.CRServerLost, sqlerror.SSUnknownSQLState, "%v", err)
                }
+               if maxrows == FETCH_NO_ROWS {
+                       return result, more, warnings, nil
+               }

                if c.isEOFPacket(data) {
                        defer c.recycleReadPacket()

[shlomi-noach]

Committed as a30c804, let's see how the entire CI reacts.

[shlomi-noach]

CI looks happy. @harshit-gangal what do you think?

[harshit-gangal]

It looks good to me

[shlomi-noach]

This PR is looking for two approvals!

[shlomi-noach]

Actually, I'm adding a couple more unit tests here. Something I noticed.

[shlomi-noach]

OK, I further modified the FETCH_NO_ROWS behavior: https://github.com/vitessio/vitess/pull/14949/files/a30c8045ff79dcd5fede6a27a57dc7ac48327411..db5de4f30e2afc701dd5bc1d9d0097e427fdd4a1

I added a couple unit tests that showed that ExecuteFetchMultiDrain left the connection in invalid state, and followup queries were failing. This commit now plays it very safe: it does iterate the packet for resulting rows, it just does nothing with them. It is more wasteful in that it iterates all results.

[shlomi-noach]

Looking for another review.

[vmg]

Looks well thought out to me.

[vmg]

@shlomi-noach I pushed a small commit to revert your UnwrappedIs helper. If you look at the implementation of errors.Is in the standard library, you'll notice you've re-implemented the same function!

https://cs.opensource.google/go/go/+/refs/tags/go1.22.0:src/errors/wrap.go;l=68-73

Also notably, if you change your unit test to use errors.Is, it's also still green. So I removed that unit test too. :)

[shlomi-noach]

@vmg if I replace is := UnwrappedIs with is := errors.Is then there actually is a test failure, but it's for testing whether nil error is nil, which returns true for errors.Is and false for UnwrappedIs. I'm perfectly happy with returning true so the change is good.

[vmg]

Right, I did find that out. It seems like errors.Is(nil, nil) == true is very sensible behavior.