feat: removing short cicuit in client use db

[vanhtuan0409]

During implementation of a SQL proxy. I encountered a rare bug due to a short circuit condition in client/conn.UseDB

The proxy implementation is quite simple

type SQLProxy struct {
	conn   *client.Conn
	logger zerolog.Logger
}

func (p *SQLProxy) UseDB(dbName string) error {
	p.logger.Debug().Str("db", dbName).Msg("handle use db")

	return p.conn.UseDB(dbName)
}

func (p *SQLProxy) HandleQuery(query string) (*mysql.Result, error) {
	p.logger.Debug().Str("query", query).Msg("handle query")

	return p.conn.Execute(query)
}
...

I tested with SQL script as follow

select database();
drop database proxier;
create database if not exists proxier;
use proxier;
select database();

during the drop database command, upstream MySQL automatically resets the connection selected db back to none
but SQLProxy.conn still track the old deleted db. During use proxier command, p.conn.UseDB got short circuit because the selected db is not changed

I propose to add a new function as ForceUseDB to bypass the short circuit check

[lance6716]

I think it's better that we reset the inuse DB when DROP DATABASE, or we remove the short circuit. Because even if this PR adds a new function, other developers don't know it and still meet the problem. Can you use tcpdump/wireshark to learn the behaviour of official MySQL client? Does it also have short circuit? If not, the easier way is we also remove the short circuit.

[vanhtuan0409]

based in the implementation of mariadb client. It seems they cross-check with server using select database() command
https://github.com/MariaDB/server/blob/main/client/mysql.cc#L4847

I agree that we can just remove the short circuit altogether with a little cost of extra network traffic. This PR was created at first to maintain the current behaviour and avoid breaking changes

So how do you say @lance6716 . Are we accepting changes in behaviour?

[lance6716]

I agree that we can just remove the short circuit altogether with a little cost of extra network traffic

LGTM. PTAL @dveeden @atercattus

[lance6716]

PTAL @dveeden

[vanhtuan0409]

can we merge this @lance6716 @dveeden ?

[dveeden]

I'll check this today.

[dveeden]

I've tested this and this seems correct to me.

The only issue I can see with this is that there is no comment or test added, which might mean we could accidentally break this in the future.

[lance6716]

Hi @vanhtuan0409 can you add a test in client/conn_test.go? You can find examples in that file that executes SQL and call methods on Conn.

[dveeden]

I tried something that is related to this:

Small change in go-mysql:

diff --git a/client/auth.go b/client/auth.go
index 1d2e457..2d3c15e 100644
--- a/client/auth.go
+++ b/client/auth.go
@@ -218,7 +218,7 @@ func (c *Conn) writeAuthHandshake() error {
                c.ccaps&mysql.CLIENT_MULTI_STATEMENTS | c.ccaps&mysql.CLIENT_MULTI_RESULTS |
                c.ccaps&mysql.CLIENT_PS_MULTI_RESULTS | c.ccaps&mysql.CLIENT_CONNECT_ATTRS |
                c.ccaps&mysql.CLIENT_COMPRESS | c.ccaps&mysql.CLIENT_ZSTD_COMPRESSION_ALGORITHM |
-               c.ccaps&mysql.CLIENT_LOCAL_FILES
+               c.ccaps&mysql.CLIENT_LOCAL_FILES | c.ccaps&mysql.CLIENT_SESSION_TRACK
 
        capability &^= c.clientExplicitOffCaps
 

proxy:

package main

import (
	"net"

	"github.com/go-mysql-org/go-mysql/client"
	"github.com/go-mysql-org/go-mysql/mysql"
	"github.com/go-mysql-org/go-mysql/server"
	"github.com/rs/zerolog"
)

type SQLProxy struct {
	server.EmptyHandler
	conn   *client.Conn
	logger zerolog.Logger
}

func (p SQLProxy) UseDB(dbName string) error {
	p.logger.Debug().Str("db", dbName).Msg("handle use db")

	return p.conn.UseDB(dbName)
}

func (p SQLProxy) HandleQuery(query string) (*mysql.Result, error) {
	p.logger.Debug().Str("query", query).Msg("handle query")

	return p.conn.Execute(query)
}

func main() {
	l, err := net.Listen("tcp", "127.0.0.1:4000")
	if err != nil {
		panic(err)
	}
	c, err := l.Accept()
	if err != nil {
		panic(err)
	}

	h := SQLProxy{}
	h.conn, err = client.Connect("127.0.0.1:3306", "root", "", "", func(conn *client.Conn) error {
		conn.SetCapability(mysql.CLIENT_SESSION_TRACK)
		return nil
	})

	if err != nil {
		panic(err)
	}
	defer h.conn.Quit()

	conn, err := server.NewConn(c, "root", "", h)
	if err != nil {
		panic(err)
	}
	for {
		if err := conn.HandleCommand(); err != nil {
			panic(err)
		}
	}
}

And a MariaDB instance (could have been MySQL as well):

podman run --rm --name mariadb -e MARIADB_ALLOW_EMPTY_ROOT_PASSWORD=1 -p 3306:3306 mariadb:11.8 --log-bin=mariadb-log --binlog-format=ROW --binlog-legacy-event-pos

And then I get this in Wireshark (between proxy and server, port 3306/tcp):

(the capability is set)

(schema tracking info in OK packet)

(schema tracking info in OK packet after running drop schema test)

This is the server config for session tracking:

mysql-11.8.5-MariaDB-ubu2404-log [(none)]> SHOW GLOBAL VARIABLES LIKE 'session\_track\_%';
+--------------------------------+-------------------------------------------------------------------------------------------------------+
| Variable_name                  | Value                                                                                                 |
+--------------------------------+-------------------------------------------------------------------------------------------------------+
| session_track_schema           | ON                                                                                                    |
| session_track_state_change     | OFF                                                                                                   |
| session_track_system_variables | autocommit,character_set_client,character_set_connection,character_set_results,redirect_url,time_zone |
| session_track_transaction_info | OFF                                                                                                   |
+--------------------------------+-------------------------------------------------------------------------------------------------------+
4 rows in set (0.002 sec)

[vanhtuan0409]

let's me add some test 1st. It can help to verify both the short circuit and the client session tracking

[dveeden]

Note that for this PR you can basically ignore the session tracking. The session tracking probably should be a separate PR.

[lance6716]

@vanhtuan0409 If you have some difficulty in adding unit test, I think we can merge this PR and let me add unit test later. Or you can ask us for help.

[vanhtuan0409]

@vanhtuan0409 If you have some difficulty in adding unit test, I think we can merge this PR and let me add unit test later. Or you can ask us for help.

That would be great. I've been busy recently with all the development

[lance6716]

@dveeden I have pushed a commit to add test for "use db" feature. Let's leave "session tracking" for another issue.