MariaDB Metadata skipping #1708
Conversation
WalkthroughThis change introduces explicit support for MariaDB's extended capability flags, particularly focusing on the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Connector
participant Server
Client->>Connector: Connect()
Connector->>Server: Send handshake request
Server-->>Connector: Send handshake packet (includes capabilities)
Connector->>Connector: Parse server capabilities
Connector->>Connector: Compute client capabilities (including MariaDB extended)
Connector->>Server: Send handshake response (with capabilities)
Server-->>Connector: Authenticate/OK
Note over Connector: Use extended capabilities to decide on metadata caching
Connector->>Server: Prepare/Query/Exec
Server-->>Connector: Send result set header (may include metadata cache flag)
alt clientCacheMetadata enabled
Connector->>Connector: Cache/reuse column metadata
else
Connector->>Connector: Read/skip columns as usual
end
Connector-->>Client: Return results
Possibly related issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
const.go (1)
76-86: Well-structured implementation of MariaDB extended capabilities.The introduction of
extendedCapabilityFlagtype and associated constants properly encapsulates MariaDB-specific extended capabilities, particularly theclientCacheMetadataflag that enables metadata skipping functionality.Consider adding a brief comment for each extended capability flag to document their purpose, especially for the
clientCacheMetadataflag which is central to this PR's implementation.rows.go (1)
159-159: Updated function call to handle new metadata caching information.The function call now correctly unpacks the additional boolean return value from
readResultSetHeaderPacket()which likely indicates metadata caching presence.The unused variable (represented by
_) indicates metadata caching information that's not needed in this context. Consider adding a brief comment explaining why this information is discarded here but might be used elsewhere.auth_test.go (1)
137-1346: Comprehensive test updates for authentication methods with capability flags.All authentication-related tests have been consistently updated to match the new function signature, passing zero values for capability flags which is appropriate for these tests since they're not testing capability negotiation specifically.
Consider adding at least one test case that uses non-zero values for capability flags to explicitly test how capability negotiation affects the authentication process, particularly for metadata caching.
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 293-293: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 665-665: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 868-868: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
[warning] 1301-1301: MinVersion
is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures(missing-ssl-minversion-go)
benchmark_test.go (2)
402-407: Populateargsslice from index 0 to keep invariant obviousBecause the first
forstarts at index 1,args[0]is assigned only in the nested loop below. The code is correct, but the two-phase “odd indices first, even indices later” pattern is a bit non-obvious to future readers.- for i := 1; i < 200; i += 2 { + for i := 0; i < 200; i += 2 {…and swap the later writer loop to fill the odd indices. Functionality is unchanged while intent becomes clearer.
459-512: BenchmarkReceiveMetadata: tighten correctness & reduce noise
db.SetMaxIdleConns(0)immediately followed bydb.SetMaxIdleConns(1)cancels itself.rows.Next()’s boolean return is ignored; if the single row is unexpectedly absent,Scantriggers “row.Scan called without Next”.- The benchmark never checks
rows.Err(); silent I/O errors could be missed.- db.SetMaxIdleConns(0) - db.SetMaxIdleConns(1) + db.SetMaxIdleConns(1) // one stable idle conn is enough - rows := tb.checkRows(stmt.Query()) - rows.Next() - // Scan the row - err := rows.Scan(valuePtrs...) - tb.check(err) - rows.Close() + rows := tb.checkRows(stmt.Query()) + if !rows.Next() { + rows.Close() + b.Fatalf("unexpectedly empty result set") + } + tb.check(rows.Scan(valuePtrs...)) + tb.check(rows.Close()) + tb.check(rows.Err())These tweaks make the benchmark more robust without affecting the performance signal.
connector.go (1)
133-143: TLS enforcement check is welcome – but message could be clearerThe error text is surfaced to application code; clarifying that the server capability flags are missing helps operators:
- return nil, fmt.Errorf("TLS is required, but server doesn't support it") + return nil, fmt.Errorf("TLS requested in DSN but server did not advertise CLIENT_SSL capability")Minor wording tweak improves diagnosability.
packets.go (3)
282-318: *The helper receives a Config but then ignores it
initClientCapabilitiestakescfg *Config, yet most of the checks (compress,TLS,MultiStatements,DBName) usemc.cfgdirectly. Either:
- Drop the parameter and rely on
mc.cfg, or- Use the
cfgthat is passed in to avoid confusion.-func (mc *mysqlConn) initClientCapabilities(serverCapabilities capabilityFlag, cfg *Config) capabilityFlag { +func (mc *mysqlConn) initClientCapabilities(serverCapabilities capabilityFlag) capabilityFlag { ... - if cfg.ClientFoundRows { + if mc.cfg.ClientFoundRows { ... - if cfg.compress { + if mc.cfg.compress { ... - if n := len(cfg.DBName); n > 0 { + if n := len(mc.cfg.DBName); n > 0 {This will also simplify the call-site (
mc.initClientCapabilities(serverCapabilities)).
573-604: Variable name ‑lenshadows the built-in
Usinglenas a local variable hides the predeclaredlen()function and can trip up future readers (and some IDE/go-analysis tooling).- num, _, len := readLengthEncodedInteger(data) + num, _, bytesRead := readLengthEncodedInteger(data) ... - return int(num), data[len] == 0x01, nil + return int(num), data[bytesRead] == 0x01, nilNo behavioural change – just clearer code.
921-928: Blindly discarding a packet may swallow protocol errors
skipEofreads (and discards) one packet whenclientDeprecateEOFis not set but never checks that the packet is actually an EOF/OK packet. A malformed server response would pass unnoticed.-if mc.clientCapabilities&clientDeprecateEOF == 0 { - if _, err := mc.readPacket(); err != nil { - return err - } +if mc.clientCapabilities&clientDeprecateEOF == 0 { + pkt, err := mc.readPacket() + if err != nil { + return err + } + if pkt[0] != iEOF && pkt[0] != iOK { + return fmt.Errorf("protocol error: expected EOF/OK, got 0x%02x", pkt[0]) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
auth_test.go(14 hunks)benchmark_test.go(3 hunks)connection.go(5 hunks)connector.go(3 hunks)const.go(2 hunks)packets.go(15 hunks)packets_test.go(1 hunks)rows.go(3 hunks)statement.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
connection.go (1)
dsn.go (1)
Config(37-84)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (ubuntu-latest, 1.23, 9.0)
- GitHub Check: test (macos-latest, 1.24, mariadb-10.11)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.6)
- GitHub Check: test (windows-latest, 1.24, mariadb-10.11)
- GitHub Check: test (macos-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, 8.0)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.1)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.4)
- GitHub Check: test (windows-latest, 1.24, 5.7)
- GitHub Check: test (windows-latest, 1.24, mariadb-11.2)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.5)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-10.11)
- GitHub Check: test (ubuntu-latest, 1.24, 9.0)
- GitHub Check: test (macos-latest, 1.24, 9.0)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.4)
- GitHub Check: test (ubuntu-latest, 1.24, 5.7)
- GitHub Check: test (ubuntu-latest, 1.24, 8.4)
- GitHub Check: test (ubuntu-latest, 1.24, mariadb-11.1)
- GitHub Check: test (ubuntu-latest, 1.24, 8.0)
🔇 Additional comments (14)
const.go (2)
46-46: Improved type name enhances code clarity.Renaming
clientFlagtocapabilityFlagbetter reflects its purpose as it represents capabilities of both client and server, not just client flags.
49-49: Renamed constant maintains consistent naming with its type.Renaming
clientLongPasswordtoclientMySQLaligns with the type name change and better represents its meaning as the first capability flag.packets_test.go (2)
335-335: Updated function call signature to match implementation changes.The test correctly adapts to the modified
readHandshakePacketfunction signature that now returns additional capability flags.
340-346: Good test coverage for server capabilities.The added assertions verify that both standard and extended server capabilities are correctly parsed from the handshake packet, which is essential for testing the MariaDB metadata caching feature.
rows.go (2)
116-116: More precise method for skipping result set rows.Replacing
readUntilEOF()withskipResultSetRows()improves code semantics and likely handles metadata caching more efficiently when closing rows.
146-146: Consistent replacement of EOF reading with specialized method.This change aligns with the previous one, ensuring consistent handling of result sets with potential metadata caching.
auth_test.go (1)
92-92: Updated function signature for handshake response packet.The call to
writeHandshakeResponsePacketnow includes the server capability parameters needed for capability negotiation during authentication.benchmark_test.go (1)
132-147: Minor loop-formatting change is fineOnly whitespace was touched here; functionality and performance stay identical.
statement.go (2)
120-131: Skip-EOF logic may mis-handle MariaDB “metadata skipped” pathWhen
metadataFollows == false, MariaDB does not send an EOF packet; the first packet after the header is already the first row. Callingmc.skipEof()risks consuming that row and corrupting the stream.Please verify with the MariaDB protocol docs; if confirmed, gate the EOF skip:
- if err = mc.skipEof(); err != nil { + if mc.mc.isMySQL() { // or similar capability check + if err = mc.skipEof(); err != nil { return nil, err } }Failing to do so will surface as “invalid sequence” or “unexpected field count” on the next read.
73-83: Exec: fast-path skips — good changeReplacing
readUntilEOFwith explicitskipColumnsandskipResultSetRowsavoids parsing work and aligns with capability-aware handling. Nice!connector.go (2)
160-161: HandshakeResponse now forwards server capability info – goodForward-propagating both capability bitsets keeps the client/server negotiation symmetric and maintainable.
174-177: Compression gating uses negotiated client flags – correctSwitching from
mc.flagstomc.clientCapabilitiesfixes false-negatives when the initial capability mask differed. Looks good.connection.go (1)
223-242: Conditional metadata path looks solid – just ensure compatibility downstream
The new branch that caches column metadata whenclientCacheMetadatais negotiated is well-structured and will save network round-trips. Please double-check every call-site that later inspectsstmt.columns; when metadata is skipped, it will now benil, which may not have been the case before.packets.go (1)
945-960: EOF detection may mis-classify large data packets
skipResultSetRowstreats any packet whose first byte is0xFEand whose total length < 16 MB (0xffffff) as EOF/OK. According to the protocol, row packets can also legitimately start with0xFEwhen the first column’s length-encoded string happens to use the 0xFE marker and the row is small (< 8 bytes/field).A safer check is “length < 9” (EOF packet’s minimum size) which MySQL itself and other clients use.
Implement MariaDB metadata skipping
This is in replacement for #1692, for a simplier (still requiring lots of changes).
See MariaDB metadata skipping. With this change, MariaDB server won't send metadata when they have not changed, saving client parsing metadata and network.
Even if metadata skipping change is not so big, this rely on big changes :
A benchmark BenchmarkReceiveMetadata has been added to show the difference.
On a local server, difference gain is only a few percent.
on a distant server, difference is huge. Example here with a server next to client :
goos: linux
goarch: amd64
pkg: github.com/go-sql-driver/mysql
cpu: 11th Gen Intel(R) Core(TM) i9-11900K @ 3.50GHz
BenchmarkReceiveMetadata
BenchmarkReceiveMetadata/query
before :
BenchmarkReceiveMetadata/query-16 138 7849971 ns/op 90379 B/op 1015 allocs/op
after change:
BenchmarkReceiveMetadata/query-16 362 2974155 ns/op 33337 B/op 16 allocs/op
Checklist
Summary by CodeRabbit
New Features
Improvements
Bug Fixes