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

mysql: do not allocate in parseOKPacket #15067

Merged
merged 1 commit into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 5 additions & 7 deletions go/mysql/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -1525,15 +1525,13 @@ type PacketOK struct {
sessionStateData string
}

func (c *Conn) parseOKPacket(in []byte) (*PacketOK, error) {
func (c *Conn) parseOKPacket(packetOK *PacketOK, in []byte) error {
data := &coder{
data: in,
pos: 1, // We already read the type.
}
packetOK := &PacketOK{}

fail := func(format string, args ...any) (*PacketOK, error) {
return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, format, args...)
fail := func(format string, args ...any) error {
return vterrors.Errorf(vtrpcpb.Code_INTERNAL, format, args...)
}

// Affected rows.
Expand Down Expand Up @@ -1578,7 +1576,7 @@ func (c *Conn) parseOKPacket(in []byte) (*PacketOK, error) {
if !ok || length == 0 {
// In case we have no more data or a zero length string, there's no additional information so
// we can return the packet.
return packetOK, nil
return nil
}

// Alright, now we need to read each sub packet from the session state change.
Expand Down Expand Up @@ -1615,7 +1613,7 @@ func (c *Conn) parseOKPacket(in []byte) (*PacketOK, error) {
}
}

return packetOK, nil
return nil
}

// isErrorPacket determines whether or not the packet is an error packet. Mostly here for
Expand Down
12 changes: 7 additions & 5 deletions go/mysql/conn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,8 @@ func TestBasicPackets(t *testing.T) {
require.NotEmpty(data)
assert.EqualValues(data[0], OKPacket, "OKPacket")

packetOk, err := cConn.parseOKPacket(data)
var packetOk PacketOK
err = cConn.parseOKPacket(&packetOk, data)
require.NoError(err)
assert.EqualValues(12, packetOk.affectedRows)
assert.EqualValues(34, packetOk.lastInsertID)
Expand All @@ -272,7 +273,7 @@ func TestBasicPackets(t *testing.T) {
require.NotEmpty(data)
assert.EqualValues(data[0], OKPacket, "OKPacket")

packetOk, err = cConn.parseOKPacket(data)
err = cConn.parseOKPacket(&packetOk, data)
require.NoError(err)
assert.EqualValues(23, packetOk.affectedRows)
assert.EqualValues(45, packetOk.lastInsertID)
Expand All @@ -295,7 +296,7 @@ func TestBasicPackets(t *testing.T) {
require.NotEmpty(data)
assert.True(cConn.isEOFPacket(data), "expected EOF")

packetOk, err = cConn.parseOKPacket(data)
err = cConn.parseOKPacket(&packetOk, data)
require.NoError(err)
assert.EqualValues(12, packetOk.affectedRows)
assert.EqualValues(34, packetOk.lastInsertID)
Expand Down Expand Up @@ -690,7 +691,8 @@ func TestOkPackets(t *testing.T) {
cConn.Capabilities = testCase.cc
sConn.Capabilities = testCase.cc
// parse the packet
packetOk, err := cConn.parseOKPacket(data)
var packetOk PacketOK
err := cConn.parseOKPacket(&packetOk, data)
if testCase.expectedErr != "" {
require.Error(t, err)
require.Equal(t, testCase.expectedErr, err.Error())
Expand All @@ -699,7 +701,7 @@ func TestOkPackets(t *testing.T) {
require.NoError(t, err, "failed to parse OK packet")

// write the ok packet from server
err = sConn.writeOKPacket(packetOk)
err = sConn.writeOKPacket(&packetOk)
require.NoError(t, err, "failed to write OK packet")

// receive the ok packet on client
Expand Down
25 changes: 12 additions & 13 deletions go/mysql/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,9 @@

// ReadQueryResult gets the result from the last written query.
func (c *Conn) ReadQueryResult(maxrows int, wantfields bool) (*sqltypes.Result, bool, uint16, error) {
var packetOk PacketOK
// Get the result.
colNumber, packetOk, err := c.readComQueryResponse()
colNumber, err := c.readComQueryResponse(&packetOk)
if err != nil {
return nil, false, 0, err
}
Expand Down Expand Up @@ -441,8 +442,7 @@
more = (statusFlags & ServerMoreResultsExists) != 0
result.StatusFlags = statusFlags
} else {
packetOk, err := c.parseOKPacket(data)
if err != nil {
if err := c.parseOKPacket(&packetOk, data); err != nil {
vmg marked this conversation as resolved.
Show resolved Hide resolved
return nil, false, 0, err
}
warnings = packetOk.warnings
Expand Down Expand Up @@ -497,35 +497,34 @@
}
}

func (c *Conn) readComQueryResponse() (int, *PacketOK, error) {
func (c *Conn) readComQueryResponse(packetOk *PacketOK) (int, error) {
data, err := c.readEphemeralPacket()
if err != nil {
return 0, nil, sqlerror.NewSQLError(sqlerror.CRServerLost, sqlerror.SSUnknownSQLState, "%v", err)
return 0, sqlerror.NewSQLError(sqlerror.CRServerLost, sqlerror.SSUnknownSQLState, "%v", err)
}
defer c.recycleReadPacket()
if len(data) == 0 {
return 0, nil, sqlerror.NewSQLError(sqlerror.CRMalformedPacket, sqlerror.SSUnknownSQLState, "invalid empty COM_QUERY response packet")
return 0, sqlerror.NewSQLError(sqlerror.CRMalformedPacket, sqlerror.SSUnknownSQLState, "invalid empty COM_QUERY response packet")

Check warning on line 507 in go/mysql/query.go

View check run for this annotation

Codecov / codecov/patch

go/mysql/query.go#L507

Added line #L507 was not covered by tests
}

switch data[0] {
case OKPacket:
packetOk, err := c.parseOKPacket(data)
return 0, packetOk, err
return 0, c.parseOKPacket(packetOk, data)
case ErrPacket:
// Error
return 0, nil, ParseErrorPacket(data)
return 0, ParseErrorPacket(data)
case 0xfb:
// Local infile
return 0, nil, vterrors.Errorf(vtrpc.Code_UNIMPLEMENTED, "not implemented")
return 0, vterrors.Errorf(vtrpc.Code_UNIMPLEMENTED, "not implemented")

Check warning on line 518 in go/mysql/query.go

View check run for this annotation

Codecov / codecov/patch

go/mysql/query.go#L518

Added line #L518 was not covered by tests
}
n, pos, ok := readLenEncInt(data, 0)
if !ok {
return 0, nil, sqlerror.NewSQLError(sqlerror.CRMalformedPacket, sqlerror.SSUnknownSQLState, "cannot get column number")
return 0, sqlerror.NewSQLError(sqlerror.CRMalformedPacket, sqlerror.SSUnknownSQLState, "cannot get column number")

Check warning on line 522 in go/mysql/query.go

View check run for this annotation

Codecov / codecov/patch

go/mysql/query.go#L522

Added line #L522 was not covered by tests
}
if pos != len(data) {
return 0, nil, sqlerror.NewSQLError(sqlerror.CRMalformedPacket, sqlerror.SSUnknownSQLState, "extra data in COM_QUERY response")
return 0, sqlerror.NewSQLError(sqlerror.CRMalformedPacket, sqlerror.SSUnknownSQLState, "extra data in COM_QUERY response")

Check warning on line 525 in go/mysql/query.go

View check run for this annotation

Codecov / codecov/patch

go/mysql/query.go#L525

Added line #L525 was not covered by tests
}
return int(n), &PacketOK{}, nil
return int(n), nil
}

//
Expand Down
3 changes: 2 additions & 1 deletion go/mysql/streaming_query.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ func (c *Conn) ExecuteStreamFetch(query string) (err error) {
}

// Get the result.
colNumber, _, err := c.readComQueryResponse()
var packetOk PacketOK
colNumber, err := c.readComQueryResponse(&packetOk)
if err != nil {
return err
}
Expand Down
Loading