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

PrepareStatement got incorrect paramsCount in response packet from mock mysql server. #1793

Closed
win5do opened this issue May 24, 2023 · 5 comments · Fixed by dolthub/vitess#241

Comments

@win5do
Copy link

win5do commented May 24, 2023

I use this lib to write some tests with https://github.com/jinzhu/gorm@v1.9.16 and https://github.com/go-sql-driver/mysql@v1.6.0.

A panic "sql: expected 0 arguments, got 1" occurred in grom's AutoMigrate func.

But there is no problem when I changed to a real mysql@5.7 started by docker.

After comparison, it is found that the paramsCount returned by the server is inconsistent.

// Param count [16 bit uint]
stmt.paramCount = int(binary.LittleEndian.Uint16(data[7:9]))

The SQL is:

SHOW TABLES FROM `gotest` WHERE `Tables_in_gotest` = ?

I write a test based on the code in dolthub/vitess:

https://github.com/dolthub/vitess/blob/73e284f11fe80760f9ef4e6afaddc00ddeaaefa5/go/mysql/conn.go#L1116

func TestStmtPrepare(t *testing.T) {
	statement, err := sqlparser.Parse("SHOW TABLES FROM `gotest` WHERE `Tables_in_gotest` = ?")
	require.NoError(t, err)
	paramsCount := uint16(0)
	_ = sqlparser.Walk(func(node sqlparser.SQLNode) (bool, error) {
		switch node := node.(type) {
		case *sqlparser.SQLVal:
			if strings.HasPrefix(string(node.Val), ":v") {
				paramsCount++
			}
		}
		return true, nil
	}, statement)
	assert.Equal(t, uint16(1), paramsCount)
}

Got unexpected output:

        	Error:      	Not equal: 
        	            	expected: 0x1
        	            	actual  : 0x0
@timsehn
Copy link
Sponsor Contributor

timsehn commented May 24, 2023

This is a good bug and repro. We've seen some bugs with native prepared implementation in Dolt so this may be the cause. We will fix.

@win5do
Copy link
Author

win5do commented May 25, 2023

@jfontan

Thanks for your work, this SQL is indeed not a very common case.

I temporarily solved the problem with go-mysql-driver's “interpolateParams=true” parameter.

@fulghum
Copy link
Contributor

fulghum commented May 30, 2023

Hey @win5do, thanks for opening this issue and for providing the nice repro case. I debugged through this and you're absolutely right that the param count is being calculated in the vitess layer by looking for types of sqlparser.SQLVal; unfortunately the grammar we use doesn't consistently use sqlparser.SQLVal in every place where a prepared param value is valid, so that's why the go-mysql-server layer is detecting 1 param and the Vitess layer is detecting 0 params.

I'm going to try out a few ideas to fix this...

  • One idea is updating our grammar to consistently use sqlparser.SQLVal in every spot where a prepared parameter is valid. This feels like it'll be tricky to get 100% correct and to enforce as we continue to grow the grammar.
  • A second idea is changing the interface between GMS and Vitess so that GMS can just tell Vitess how many prepared params it found, instead of having code in Vitess and GMS that calculates the param count differently. This feels like a more invasive change, but also seems like a faster path to having this calculation be correct and stay correct over time.

Either way, we'll get this prepared query working correctly. Glad to hear you have a good workaround with interpolateParams=true in the meantime.

@fulghum
Copy link
Contributor

fulghum commented May 31, 2023

I debugged through this a little deeper and realized that we actually do have the right sqlparser.SQLVal instances in the parsed statement, but... our code to recursively walk the SHOW TABLES statement wasn't following those struct members, so that's why the code in conn.go never found those params.

I've got a fix for this statement coded up locally and passing the repro test you provided. I'm going to tidy it up a little bit, do another round of debugging and testing to make sure I didn't miss anything, and then get the fix out for review.

Thanks again for taking the time to report this so we could fix it! 🙏

@fulghum
Copy link
Contributor

fulghum commented May 31, 2023

@win5do – the fix for this issue is now available in main for go-mysql-server. Thank you for taking the time to report this and for providing such an easy repro case! 🙏 Let us know if you hit any other snags with go-mysql-server and we'll be happy to investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants