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

ExecuteFetch: error on multiple result sets #14949

Merged
merged 48 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
222a174
ExecuteFetch: error on multiple results
shlomi-noach Jan 15, 2024
0125804
more suggestive description
shlomi-noach Jan 15, 2024
48bc86d
exhaust further result sets. Prioritize multi-results error over resu…
shlomi-noach Jan 15, 2024
232da20
break down ExecuteFetch multi-statements
shlomi-noach Jan 15, 2024
a6d97f5
ExecuteFetchMultiDrain
shlomi-noach Jan 15, 2024
4e3544f
remove uneccessary (though unharmful) semicolon
shlomi-noach Jan 15, 2024
a4d0271
fix multi statements
shlomi-noach Jan 15, 2024
7b28d8f
fix multi statements
shlomi-noach Jan 15, 2024
2cde946
fix multi statements
shlomi-noach Jan 15, 2024
7867fb2
fix multi statements
shlomi-noach Jan 16, 2024
0c88aa8
fix multi statements
shlomi-noach Jan 16, 2024
c039998
fix multi statements
shlomi-noach Jan 16, 2024
313487c
fix test rewrite
shlomi-noach Jan 16, 2024
9adac78
collect errors rather than number of errors
shlomi-noach Jan 16, 2024
1690cc0
fix multi statements
shlomi-noach Jan 16, 2024
1003574
useDB rather than 'USE ...' statement
shlomi-noach Jan 16, 2024
7be46ad
Fix failover test for reparenting
dbussink Jan 16, 2024
3f8a010
use QueryTabletMultiple
shlomi-noach Jan 16, 2024
c26b25a
Merge branch 'main' into execute-fetch-error-more
shlomi-noach Jan 18, 2024
245b9bc
fix multi statements
shlomi-noach Jan 22, 2024
8e30a12
log query/queries in error message
shlomi-noach Jan 22, 2024
2ef3fa5
RunSQLs runs queries in single connection
shlomi-noach Jan 22, 2024
ed7e27a
fix multi statements
shlomi-noach Jan 22, 2024
02a39e8
fix multi statements
shlomi-noach Jan 22, 2024
b15e38d
clearer error message
shlomi-noach Jan 22, 2024
953330f
QueryTablet loop -> QueryTabletMultiple with no loop
shlomi-noach Jan 22, 2024
2147a8b
fix multi statements
shlomi-noach Jan 22, 2024
28b42a2
fix multi statements
shlomi-noach Jan 22, 2024
3d05639
fix multi statements
shlomi-noach Jan 22, 2024
b87350e
rewording
shlomi-noach Jan 22, 2024
760c64d
fix multi statements
shlomi-noach Jan 22, 2024
dab41b1
generic fix for multi-statements in NewMySQL()
shlomi-noach Jan 22, 2024
8f07a3b
fix multi statements
shlomi-noach Jan 22, 2024
6e1ec2f
simplify test error check
shlomi-noach Jan 22, 2024
5b1d205
Ignore sqlerror.ERNonExistingGrant
shlomi-noach Jan 22, 2024
262a8da
UnwrappedIs
shlomi-noach Jan 22, 2024
ca09be0
turn ErrExecuteFetchMultipleResults to existing/expected muti-result …
shlomi-noach Jan 22, 2024
fb6f8d4
feat: update vtorc tests to function properly
GuptaManan100 Jan 23, 2024
dcffe94
test both before and after multi-result procs
shlomi-noach Jan 24, 2024
404ddb6
do not limit rows, so that we can consume them all
shlomi-noach Jan 24, 2024
fbb23a6
remove grant relaxation patch
shlomi-noach Jan 24, 2024
4351f12
resolved conflict
shlomi-noach Jan 24, 2024
8e1eb7f
refactor: move drain to it's own little method
harshit-gangal Jan 24, 2024
50c2243
fix test: drop table (previously there was a hidden 'Table 'test_idx'…
shlomi-noach Jan 24, 2024
a30c804
FETCH_NO_ROWS
shlomi-noach Jan 30, 2024
39ab378
multi-drain still fully reads packet rows, just not into memory
shlomi-noach Jan 31, 2024
db5de4f
code comments
shlomi-noach Jan 31, 2024
9620d3c
errors: do not re-implement errors.Is
vmg Feb 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions go/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ limitations under the License.

package errors

import "errors"

// Wrapped is used to unwrap an error created by errors.Join() in Go 1.20
type Wrapped interface {
Unwrap() []error
Expand Down Expand Up @@ -56,13 +54,3 @@ func UnwrapFirst(err error) error {
}
return UnwrapAll(err)[0]
}

// UnwrappedIs returns 'true' if any of the unwrapped error complies with golang's errors.Is(target)
func UnwrappedIs(err, target error) bool {
for _, serr := range UnwrapAll(err) {
if errors.Is(serr, target) {
return true
}
}
return false
}
60 changes: 0 additions & 60 deletions go/errors/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,63 +97,3 @@ func TestUnwrap(t *testing.T) {
})
}
}

func TestUnwrappedIs(t *testing.T) {
var err1 = errors.New("err1")

tcases := []struct {
err error
target error
expect bool
}{
{
err: nil,
expect: false,
},
{
err: nil,
target: errors.New("err1"),
expect: false,
},
{
err: errors.New("err1"),
target: errors.New("err1"),
expect: false,
},
{
err: errors.New("err1"),
target: err1,
expect: false,
},
{
err: err1,
target: err1,
expect: true,
},
{
err: errors.Join(err1, errors.New("err2")),
target: err1,
expect: true,
},
{
err: errors.Join(errors.New("err2"), err1),
target: err1,
expect: true,
},
{
err: errors.Join(errors.New("err2"), errors.Join(errors.New("err3"), err1)),
target: err1,
expect: true,
},
}
for _, tcase := range tcases {
name := "nil"
if tcase.err != nil {
name = tcase.err.Error()
}
t.Run(name, func(t *testing.T) {
is := UnwrappedIs(tcase.err, tcase.target)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

assert.Equal(t, tcase.expect, is)
})
}
}
4 changes: 2 additions & 2 deletions go/vt/vttablet/tabletserver/query_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@

import (
"context"
"errors"
"fmt"
"io"
"strings"
"sync"
"time"

"vitess.io/vitess/go/errors"
"vitess.io/vitess/go/mysql"
"vitess.io/vitess/go/mysql/replication"
"vitess.io/vitess/go/mysql/sqlerror"
Expand Down Expand Up @@ -880,9 +880,9 @@
}

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

Check warning on line 885 in go/vt/vttablet/tabletserver/query_executor.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vttablet/tabletserver/query_executor.go#L885

Added line #L885 was not covered by tests
if err != nil {
return nil, rewriteOUTParamError(err)
}
Expand Down
Loading