MariaDB metadata skipping + test stability improvement #1692
Conversation
* ensuring performance schema is enabled when testing some performance schema results * Added logic to check if the default collation is overridden by the server character_set_collations * ensure using IANA timezone in test, since tzinfo depending on system won't have deprecated tz like "US/Central" and "US/Pacific"
Refactor handshake packet handling to support extended capabilities Updated the readHandshakePacket and writeHandshakeResponsePacket functions to include server capabilities and extended capabilities. Adjusted related tests and connection logic to accommodate these changes, ensuring compatibility with MariaDB and improved handling of client capabilities.
* correct packet readLengthEncodedString that was returning byte array to readLengthEncodedBytes * have an readLengthEncodedString that effectively return string * faster column parsing: MariaDB/MySQL have an identifier limitation of 64 characters (https://dev.mysql.com/doc/refman/8.4/en/identifier-length.html) before: BenchmarkReceiveMetadata-16 1846 650394 ns/op 138776 B/op 3024 allocs/op after: BenchmarkReceiveMetadata-16 1772 639809 ns/op 138776 B/op 3024 allocs/op
… but will permit further enhancement)
WalkthroughThis pull request introduces multiple updates across the MySQL driver codebase. A new author entry is added in the AUTHORS file. Test files are modified to update handshake response calls to include additional parameters and adjust collation/timezone checks. The handshake and connection processes have been refactored by introducing new capability flag types and updating method signatures, while result set and metadata handling has been improved using skip methods. A new benchmark function for metadata retrieval is added, and utility functions for processing length-encoded strings are updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Connector
participant MySQLConn
participant Server
Connector->>MySQLConn: Connect(ctx)
MySQLConn->>Server: readHandshakePacket()
Server-->>MySQLConn: Returns handshake packet (data, serverCapabilities, serverExtendedCapabilities, plugin)
MySQLConn->>Connector: Return handshake info
Connector->>MySQLConn: writeHandshakeResponsePacket(authResp, serverCapabilities, serverExtendedCapabilities, plugin)
MySQLConn->>Server: Send handshake response
sequenceDiagram
participant Stmt as mysqlStmt
participant MySQLConn
Stmt->>MySQLConn: readResultSetHeaderPacket()
MySQLConn-->>Stmt: Return header and metadata flag
alt Metadata Follows
Stmt->>MySQLConn: readColumns(...)
else No Metadata
Stmt->>MySQLConn: skipEof()/skipResultSetRows()
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
rusher
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packets.go (1)
177-268: 💡 Verification agent🧩 Analysis chain
Refactor boundary checks and consider fallback safety in
readHandshakePacket
[Lines 230-234] Potential out-of-bounds risk during extended capabilities extraction
When reading the additional 4 bytes forserverExtendedCapabilities, there is no explicit check ensuringpos+4 <= len(data). A malformed or truncated packet could cause a slice out-of-range error.if serverCapabilities&clientMySQL == 0 {
if pos+4 > len(data) {return nil, serverCapabilities, 0, "", fmt.Errorf("malformed packet: insufficient bytes for extended capabilities")} serverExtendedCapabilities = extendedCapabilityFlag(binary.LittleEndian.Uint32(data[pos : pos+4]))
[Lines 211-216] Fallback to plaintext
The fallback mechanism (if mc.cfg.AllowFallbackToPlaintext) is logically correct. However, confirm that a downgrade from TLS to plaintext meets your security requirements, especially in production environments.Overall
Returning zero values for(serverCapabilities, serverExtendedCapabilities, plugin)upon errors (e.g., line 184) is reasonable. The updated signature properly propagates capabilities outward for subsequent usage. Aside from the boundary check issue, the rest of the handshake logic looks consistent.
Update safety checks and verify TLS fallback in
readHandshakePacket
Boundary Check for Extended Capabilities [Lines ~230-234]:
There is currently no explicit check that ensurespos+4is within bounds before reading the next 4 bytes forserverExtendedCapabilities. This omission can lead to a slice out-of-range error with malformed or truncated packets. Please add a boundary verification similar to the diff snippet below:if serverCapabilities&clientMySQL == 0 { + if pos+4 > len(data) { + return nil, serverCapabilities, 0, "", fmt.Errorf("malformed packet: insufficient bytes for extended capabilities") + } serverExtendedCapabilities = extendedCapabilityFlag(binary.LittleEndian.Uint32(data[pos : pos+4])) }Fallback to Plaintext:
The mechanism that allows downgrading from TLS to plaintext (viamc.cfg.AllowFallbackToPlaintext) is functionally correct. However, please verify that this fallback behavior aligns with your overall security requirements, especially in production environments.
🏁 Script executed:
#!/bin/bash # Check for the usage of 'Uint32' on data slice without a boundary check rg "Uint32\\(data\\[pos : pos\\+4\\]\\)" -n packets.go # Check if there's any boundary check for 'pos+4' before the extended capabilities extraction rg "if\\s+pos\\+4\\s*>\\s*len\\(data\\)" -n packets.goLength of output: 517
Critical: Fix out-of-bound risk & confirm TLS fallback behavior in
readHandshakePacket
Boundary Check for Extended Capabilities (around line 232):
The code extracts 4 bytes forserverExtendedCapabilitiesusingserverExtendedCapabilities = extendedCapabilityFlag(binary.LittleEndian.Uint32(data[pos : pos+4]))without verifying that
pos+4is within the bounds ofdata. This could lead to a slice out-of-range error if the packet is malformed. Please add a boundary check as in the following diff:if serverCapabilities&clientMySQL == 0 { + if pos+4 > len(data) { + return nil, serverCapabilities, 0, "", fmt.Errorf("malformed packet: insufficient bytes for extended capabilities") + } serverExtendedCapabilities = extendedCapabilityFlag(binary.LittleEndian.Uint32(data[pos : pos+4])) }TLS Fallback Behavior:
The fallback mechanism that disables TLS whenmc.cfg.AllowFallbackToPlaintextis enabled appears functionally correct. However, please verify that downgrading from TLS to plaintext complies with your overall security requirements, especially in production.
🧹 Nitpick comments (3)
benchmark_test.go (2)
479-484: Consider deriving column count dynamically.Instead of hardcoding 1000 for the slice sizes, consider using
rows.Columns()to dynamically determine the number of columns in the result set. This would make the code more maintainable if the table structure changes.- values := make([]interface{}, 1000) - valuePtrs := make([]interface{}, 1000) - for j := range values { - valuePtrs[j] = &values[j] - } + cols, err := rows.Columns() + tb.check(err) + values := make([]interface{}, len(cols)) + valuePtrs := make([]interface{}, len(cols)) + for j := range values { + valuePtrs[j] = &values[j] + }
465-465: Consider using a helper function for generating repeated values.The string repetition for generating placeholder values could be improved with a helper function, especially if similar patterns are used elsewhere.
- strings.Repeat("0,", 999)+"0)" + strings.Repeat("0,", 999)+"0)"Alternatively, a more reusable approach:
func generatePlaceholders(count int, value string) string { if count <= 0 { return "" } values := make([]string, count) for i := range values { values[i] = value } return strings.Join(values, ",") } // Usage "INSERT INTO large_integer_table VALUES (" + generatePlaceholders(1000, "0") + ")"const.go (1)
76-86: Good addition of extended capability flagsThe introduction of
extendedCapabilityFlagtype and its constants supports MariaDB's extended capabilities model. TheclientCacheMetadataflag is particularly important as it's used in the connection code to determine column reading behavior.Consider adding documentation comments for each new flag explaining its purpose and behavior, especially for
clientCacheMetadatawhich is used in conditional logic elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
AUTHORS(1 hunks)auth_test.go(14 hunks)benchmark_test.go(1 hunks)connection.go(5 hunks)connector.go(3 hunks)const.go(2 hunks)driver_test.go(4 hunks)packets.go(18 hunks)packets_test.go(1 hunks)rows.go(3 hunks)statement.go(4 hunks)utils.go(4 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packets.go (2)
errors.go (2)
ErrOldProtocol(27-27)ErrNoTLS(22-22)dsn.go (1)
Config(37-84)
connection.go (1)
dsn.go (1)
Config(37-84)
🔇 Additional comments (41)
AUTHORS (1)
40-40: LGTM! Proper author addition.The new author has been correctly added in alphabetical order, following the file's guidelines.
benchmark_test.go (1)
444-492: Well-structured benchmark for metadata reception performance.This benchmark provides a good way to measure metadata reception performance, which aligns with the PR objectives of improving packet parsing and metadata handling. The function creates a table with 1000 columns, inserts a single row, and measures the performance of retrieving that row's metadata.
driver_test.go (4)
1633-1658: Good improvement to handle collation overrides by server settingsThis change adds a more robust mechanism to handle cases where the default collation is overridden by server character set collations. By querying
@@character_set_collationsand mapping charset prefixes to their collations, the test can now properly validate expectations even when server-specific overrides are in place.
1721-1721: Replaced deprecated timezone identifiers with IANA standard onesGood change to replace the deprecated timezone identifiers (
US/Central,US/Pacific) with IANA standard ones (America/New_York,Asia/Hong_Kong). This improves future compatibility as older timezone identifiers might be removed in newer systems.
1729-1730: Updated timezone references in test implementationProperly updated the timezone references within the test implementation to use the new IANA standard timezone (
America/New_Yorkinstead ofUS/Central), and updated the error message to match.Also applies to: 1749-1749
3577-3585: Added check to ensure performance schema is enabledThis is an important improvement that ensures tests dependent on performance schema are only run when the feature is actually enabled in the MySQL server. This prevents false test failures when running against servers where performance schema is disabled.
statement.go (6)
23-23: Added field to store column metadata.This new field allows the statement to retain column information, which is necessary for implementing the
clientDeprecateEOFprotocol feature. This will enable reusing column metadata across subsequent result sets.
67-68: Signature update to support the deprecateEOF protocol.The function call has been updated to capture the new
metadataFollowsreturn value fromreadResultSetHeaderPacket(), which is important for determining whether column metadata follows in the result set.
74-76: Changed to useskipColumnsmethod instead of previous approach.Updated to use a more descriptive method name that better represents the intent of the code. This improves code clarity and is part of the larger change to support the clientDeprecateEOF flag.
79-80: Changed to useskipResultSetRowsmethod instead of previous approach.Similar to the above change, this improves code clarity by using a method name that better describes the operation being performed.
110-111: Updated to support metadata follows flag.The method call now captures the additional
metadataFollowsreturn value which indicates whether column metadata follows in the result set.
119-130: Implemented conditional column handling based on metadata flag.This is a key part of implementing the clientDeprecateEOF feature. When
metadataFollowsis true, it reads the columns from the connection. When false, it skips the EOF packet and reuses the previously stored columns, potentially reducing network traffic.auth_test.go (1)
92-92: Updated function signature to include server capabilities parameters.The
writeHandshakeResponsePacketmethod now accepts two additional integer parameters for server capabilities and server extended capabilities. This is consistent with changes to the handshake protocol implementation to support extended capabilities, particularly for MariaDB.Also applies to: 137-137, 179-179, 235-235, 287-287, 347-347, 403-403, 462-462, 505-505, 547-547, 596-596, 644-644, 681-681, 1346-1346
packets_test.go (2)
335-335: Updated to capture additional server capability information.The function call now captures
serverCapabilitiesandserverExtendedCapabilitiesfrom the handshake packet, which is necessary for proper protocol handling, especially with MariaDB.
340-346: Added assertions for server capability values.These new assertions verify that the server capabilities and extended capabilities are correctly parsed from the handshake packet. This helps ensure the protocol implementation is working as expected.
rows.go (3)
116-116: Updated to use the newskipResultSetRowsmethod.Changed from
mc.readUntilEOF()tomc.skipResultSetRows(), which improves code clarity and is part of the implementation of the clientDeprecateEOF protocol feature. This method better describes the intent of the operation.
146-146: Updated to use the newskipResultSetRowsmethod.Changed from
rows.mc.readUntilEOF()torows.mc.skipResultSetRows(), consistent with the change in theClose()method above, to better represent the intended operation.
159-159: Updated to handle additional metadata flag.The
readResultSetHeaderPacket()call now returns three values instead of two, with the middle value (likelymetadataFollows) being discarded with_. This is consistent with the protocol changes for clientDeprecateEOF support.connection.go (7)
26-51: Addition of new capability fields enhances connection managementThe introduction of
clientCapabilitiesandclientExtCapabilitiesfields to themysqlConnstruct aligns well with the PR objective of implementing theclientDeprecateEOFflag. These fields provide a clear structure for managing client capabilities.
227-241: Good optimization based on client capabilitiesThe conditional logic based on
clientExtCapabilitiesallows the driver to either cache or skip column metadata based on capabilities. This creates a performance optimization path when metadata caching isn't needed.
382-383: Update to handle result set header readingThis change is part of adapting the code to handle the EOF deprecation model, updating how result set headers are processed.
389-396: Improved column and row handlingReplacing
readUntilEOF()withskipColumns()andskipResultSetRows()methods is a good refactoring that supports the EOF deprecation model while potentially improving performance by avoiding unnecessary data reads.
431-432: Consistent update for query methodThis change maintains consistency with the updates made to the exec method, ensuring a uniform approach to handling result set headers.
465-466: Consistent implementation for system variablesThe change to
getSystemVarfollows the same pattern as other methods, ensuring consistency throughout the codebase.
473-480: Efficient handling of system variable resultsGood implementation of column skipping and result set row handling that maintains consistency with other similar methods.
const.go (2)
46-47: Type renaming improves clarityRenaming
clientFlagtocapabilityFlagbetter represents the purpose of this type, as it handles both client and server capabilities.
49-50: Updated constant name for better semanticsRenaming
clientLongPasswordtoclientMySQLbroadens the scope of this constant, which appears to be the first flag in the capability flags enumeration.connector.go (3)
134-135: Enhanced handshake packet readingThe updated method signature now captures additional server capability information, which aligns with the PR objective of improving the handshake process to support extended capabilities.
156-157: Improved handshake response with capabilitiesThe handshake response now includes server capabilities information, allowing for better communication of supported features between client and server.
170-171: More explicit capability checkingThe condition for enabling compression has been updated to check if
clientCompressis present inclientCapabilitiesusing a greater-than comparison instead of a bitwise AND. This is functionally equivalent but more aligned with how capability flags are conventionally checked.utils.go (4)
527-541: Renamed method preserves original behaviorThe original
readLengthEncodedStringhas been renamed toreadLengthEncodedByteswhile maintaining its functionality of returning a byte slice. This rename clarifies the method's purpose.
543-560: Added string-specific helper functionThe new
readLengthEncodedStringfunction efficiently returns a string directly instead of a byte slice, avoiding unnecessary conversions when a string is needed. This aligns with the PR objective of improving packet parsing.
586-588: Simplified handling of small integersThe optimization for handling values less than 251 reduces the overhead of going through the switch statement for common cases. This is a good performance enhancement.
603-605: Consolidated handling for large integersThe removal of the explicit case for 254 and using a default handler simplifies the code without changing functionality. All values that would have hit the 254 case now go through the default path, which is correct.
packets.go (7)
270-306: ValidateinitClientCapabilitieslogic for newly addedclientDeprecateEOFYour approach of initializing default client flags (lines 273-285), then masking them against the server’s supported flags (line 305) is appropriate to ensure only mutually supported capabilities are in effect. The inclusion of
clientDeprecateEOF(line 285) matches the rest of your new logic. This portion looks good.
308-426: Examine extended capabilities handling withinwriteHandshakeResponsePacket
[Lines 311-314] Initialization of client capabilities
PassingserverCapabilitiesintomc.initClientCapabilities(...)and selectively enablingmc.clientExtCapabilitiesis straightforward. If future extended capabilities need partial enablement, you may consider a more granular approach than a single “&” operation (line 313).[Lines 363-374] Writing extended capabilities
The current code zeroes out a 23-byte filler region for standard MySQL, or writes a 4-byte extended capability field for MariaDB. This is consistent with the handshake specification. Just ensure the filler usage remains valid if future protocol changes require more than 4 bytes for extended flags.Overall, the handshake response logic, including SSL handling and user credential packing, appears correct.
559-590: Verify new boolean return inreadResultSetHeaderPacketfor additional metadata checks
- The return signature now includes a
bool(lines 559-560) to denote whether more results exist, or if cached metadata applies (lines 585-587).- [Lines 585-589]: When
clientCacheMetadatais set, thedata[len]check determines the “more results” boolean. Otherwise, the function defaults totrue. Confirm that this logic correctly covers all server responses to avoid erroneously perpetuating more-results states.
709-783:readColumnsupdates to useskipEof()for deprecating EOF
- [Lines 778-780]: The call to
mc.skipEof()ensures EOF is read or skipped depending on theclientDeprecateEOFflag. This maintains backward compatibility with servers that still send traditional EOF packets.- The rest of the column-reading logic (lines 714-776) remains consistent, managing length-encoded strings for table/column names.
No functional issues noted.
800-815: Text-row reading logic aligned withclientDeprecateEOF
- [Lines 800-805]: Properly differentiates between actual EOF vs. “OK packet with an 0xFE header” when
clientDeprecateEOFis set, avoiding confusion for older servers.- The short-circuit with
return io.EOFat line 813 is aligned with typical row iteration patterns.
884-935: Skip logic methods (skipPackets,skipEof,skipColumns,skipResultSetRows)
- [Lines 893-900]:
skipEof()gracefully checks theclientDeprecateEOFflag, only reading a packet if EOF is not deprecated.- [Lines 902-907]:
skipColumns()callsskipPackets(resLen)and thenskipEof(), centralizing logic for skipping column metadata.- [Lines 909-935]:
skipResultSetRows()similarly checks for iERR or iEOF and updatesmc.statusaccordingly. This looks consistent and keeps the code DRY.
1249-1272: Check for EOF or Error inbinaryRows.readRow
- [Lines 1250-1269]: The changed logic ensures that an iEOF packet is interpreted according to whether
clientDeprecateEOFis set (line 1252). The code properly updatesrows.rs.done, setsrows.mcto nil if no next result set is pending, and returnsio.EOF.- The error handling at lines 1268-1271 is standard, returning
mc.handleErrorPacket(data)if the packet indicates iERR.All changes are consistent with your deprecate-EOF approach.
There was a problem hiding this comment.
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- AUTHORS: Language not supported
Comments suppressed due to low confidence (3)
packets.go:229
- The change in reserved bytes from 11 to 7 (plus an additional 4 bytes for extended capabilities) should be verified against the protocol specification to ensure correct packet parsing.
// reserved (all [00]) [10 bytes]
packets.go:722
- Directly using data[pos] as the length for field values may not handle multi-byte length encodings correctly; verify that this approach conforms to the protocol's length-encoded string specification.
pos += int(data[pos]) + 1
auth_test.go:92
- [nitpick] Passing static zeros for serverCapabilities and serverExtendedCapabilities in authentication tests might not validate the full handshake response logic; consider adding tests with non-zero values to ensure comprehensive coverage.
err = mc.writeHandshakeResponsePacket(authResp, 0, 0, plugin)
| @@ -348,9 +360,18 @@ func (mc *mysqlConn) writeHandshakeResponsePacket(authResp []byte, plugin string | |||
|
|
|||
| // Filler [23 bytes] (all 0x00) | |||
| pos := 13 | |||
| for ; pos < 13+23; pos++ { | |||
| for ; pos < 13+19; pos++ { | |||
There was a problem hiding this comment.
Since clientMySQL is unconditionally set in initClientCapabilities, the branch that writes clientExtendedCapabilities (lines 366-370) will never be triggered. Consider revising the condition to allow proper handling of MariaDB extended capabilities.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Description
This PR contains different things, separated in multiple commits.
Test stability improvement.
Implement MariaDB metadata skipping.
Refactor handshake packet handling to support extended capabilities
Updated the readHandshakePacket and writeHandshakeResponsePacket functions to include server capabilities and extended capabilities. Adjusted related tests and connection logic to accommodate these changes, ensuring compatibility with MariaDB and improved handling of client capabilities.
Packet parsing small improvement
before:
BenchmarkReceiveMetadata-16 1846 650394 ns/op 138776 B/op 3024 allocs/op
after:
BenchmarkReceiveMetadata-16 1772 639809 ns/op 138776 B/op 3024 allocs/op
Implement clientDeprecateEOF flag. (not real performance improvement, but will permit further enhancement)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores