Skip to content

Add opt-in zstd protocol compression - #1823

Merged
methane merged 5 commits into
go-sql-driver:masterfrom
methane:zstd
Oct 12, 2026
Merged

methane merged 5 commits into
go-sql-driver:masterfrom
methane:zstd

Conversation

@methane

@methane methane commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Description

Add optional zstd compression via a blank import of github.com/go-sql-driver/mysql/zstd. With compress=true, connections prefer zstd and fall back to zlib when unsupported. Compression uses a fixed level 3.

Applications that do not import mysql/zstd do not compile or link klauspost/compress.

Validated with MySQL 8.4, TLS, MariaDB fallback, and payloads larger than 16 MiB. Added protocol and dependency-isolation tests; race tests and go vet pass.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8fb691b5-04f3-4537-b535-9915a56a74af

📥 Commits

Reviewing files that changed from the base of the PR and between 9f505db and 24c3d3b.


📒 Files selected for processing (1)
  • const.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.



Walkthrough

The driver adds optional Zstandard compression through the mysql/zstd package. When compression is enabled, it negotiates Zstandard if the codec and server support it. Otherwise, it uses zlib when supported. The change adds bounded codec behavior and tests for negotiation, packet processing, codec errors, and connections.

Changes

Optional Zstandard compression

Layer / File(s) Summary
Codec contract and registration
internal/compression/compression.go, zstd/zstd.go, zstd/zstd_test.go
The internal package defines the codec contract and Zstandard level. The optional mysql/zstd package registers pooled encoders and bounded decoders. Tests cover round trips, invalid input, and the driver dependency boundary.
Capability and handshake negotiation
const.go, packets.go, connector.go, compress_negotiation_test.go, dsn.go, README.md
The driver advertises Zstandard when its codec is available and the server supports it; otherwise, it advertises zlib. The handshake includes the Zstandard level when negotiated. Tests check capability negotiation and handshake data. The README and DSN documentation describe the optional package and fallback behavior.
Compressed packet processing
compress.go, compress_negotiation_test.go, zstd/connection_test.go
compIO uses Zstandard for selected packets and zlib otherwise. Zstandard decoding respects the packet’s announced output length. Tests cover decode limits and connection behavior, including zlib fallback and optional live-server checks.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Client as MySQL client
  participant Driver as MySQL driver
  participant Server as MySQL server
  participant PacketIO as compIO
  participant Codec as Zstandard codec
  Client->>Driver: Enable compression and import mysql/zstd
  Driver->>Server: Advertise supported compression capability
  Server-->>Driver: Negotiate Zstandard or zlib
  Driver->>PacketIO: Initialize compression for negotiated capability
  PacketIO->>Codec: Encode or decode Zstandard packet payload
Loading

Suggested reviewers: rusher

Merge Risk: ⚪ Minimal · up to 24c3d

No actionable merge-blocking issue is established by the reviewed changes; normal project checks remain appropriate.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 10 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: adding optional Zstandard protocol compression.
Description check Passed The description directly explains the Zstandard compression behavior, fallback to zlib, dependency boundary, validation, and tests included in the changeset.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR

🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Oct 9, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 85.771% (+0.2%) from 85.562% — methane:zstd into go-sql-driver:master

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The negotiation, bounded decoding, fallback behavior, dependency isolation, documentation, and test coverage are consistent and complete.

0 open findings

What changed in this PR

Adds opt-in Zstandard protocol compression while preserving zlib fallback and dependency isolation.

Changes:

  • Adds optional pooled zstd codec and protocol negotiation.
  • Documents configuration and dependency behavior.
  • Adds codec, negotiation, integration, and CI coverage.
File Description
zstd/​zstd.go Implements optional zstd codec registration.
zstd/​zstd_test.go Tests codec safety and dependency isolation.
zstd/​connection_test.go Tests protocol and live-server behavior.
README.md Documents zstd activation and fallback.
packets.go Negotiates zstd and sends its level.
internal/​compression/​compression.go Defines the optional codec bridge.
go.sum Records dependency checksums.
go.mod Adds the zstd implementation dependency.
dsn.go Updates compression API documentation.
const.go Defines the zstd capability flag.
connector.go Activates negotiated zstd compression.
compress.go Adds zstd packet encoding and decoding.
compress_negotiation_test.go Tests capability negotiation and limits.
.github/​workflows/​test.yml Runs all packages and live zstd tests.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@shogo82148

Copy link
Copy Markdown
Contributor

Thanks for this! Overall the design and implementation look solid to me, and I don't see any blockers. go vet ./... and the related tests pass locally with -race (Go 1.27.1). I didn't run TestServerCompression against a real server.

What I like

  • Wiring the codec through internal/compression keeps klauspost/compress out of the build unless mysql/zstd is imported, and TestDriverDependencyBoundary guards that.
  • Negotiation matches MySQL's behavior: MySQL picks zlib when both flags are advertised, so advertising only zstd when it's available is correct. The level byte after the connection attributes matches the protocol spec.
  • Decoding is bounded properly. AvailableBuffer()[:0:uncompressedLength] together with WithDecodeAllCapLimit(true) refuses to decode past the announced size. I checked klauspost v1.20.1, and it rejects frames whose FrameContentSize exceeds cap-len.
  • The pooled encoders/decoders are never closed, but that's fine here: with concurrency=1 and only EncodeAll/DecodeAll, they don't start goroutines.

Design points worth discussing

  1. klauspost/compress in the root go.mod. Users who never import mysql/zstd still get klauspost/compress >= v1.20.1 in their module graph. MVS may bump the version for projects that already depend on an older klauspost, and it will show up in go mod graph and vulnerability scanners. A separate zstd/go.mod module is possible (the internal rule is path-based, so it could still import internal/compression), but then the Codec contract has to stay compatible across two module versions. I think the current approach is acceptable. It might be worth saying in the PR description that this trade-off was chosen on purpose.
  2. Process-wide effect. If any library transitively imports mysql/zstd, every compress=true connection in the process starts preferring zstd. That's reasonable for an opt-in, but a short note in the README might help.

Minor

  1. compress.go writePackets: if buf.AvailableBuffer() is too small, EncodeAll allocates a new slice and buf.Write copies it back. Calling buf.Grow(len(payload)) first would avoid this, since any output at least as large as the payload is thrown away anyway.
  2. compress.go readCompressedPacket: c.buff.Write(decoded) copies a slice onto itself (decoded points into c.buff's spare capacity). It's correct because tryGrowByReslice succeeds, but it's subtle. A short comment would help, e.g. "decoded points into buff's spare capacity; Write only commits the length".
  3. TestDriverDependencyBoundary runs whatever go is on PATH and needs the network or a module cache. That may fail in packaging or sandboxed environments. Consider skipping when exec.LookPath("go") fails, or under testing.Short().
  4. clientZstdCompression is defined outside the iota block. Continuing the block (e.g. adding clientOptionalResultsetMetadata for 1<<25), or adding a link to the MySQL docs, would keep the flag list easier to read.
  5. The new test files (compress_negotiation_test.go, zstd/*_test.go) are missing the license header that the existing test files have.
  6. The PR checklist is still unchecked.

CI

  • max_allowed_packet=48MB should be enough for the ~18.7MB roundtrip in TestServerCompression, including on MariaDB. Skipping the algorithm assertion on MariaDB is a nice touch.
  • With ./..., the zstd package tests now run with -race across all 21 matrix jobs. The extra time should be small, but it's worth a look.

@methane

methane commented Oct 10, 2026

Copy link
Copy Markdown
Member Author
  1. Users who never import mysql/zstd still get klauspost/compress >= v1.20.1 in their module graph. MVS may bump the version for projects that already depend on an older klauspost,

Since the changes in v1.20 do not provide much benefit for this library, I lowered the dependency version to v1.19.2.

In the README, I explained that the dependency on klauspost/compress only occurs when zstd is imported.

3. Calling buf.Grow(len(payload)) first would avoid this, since any output at least as large as the payload is thrown away anyway.

In typical cases where compression is effective, calling Grow provides little performance benefit and causes unnecessary allocations.

4. A short comment would help, e.g. "decoded points into buff's spare capacity; Write only commits the length".

added

6. clientZstdCompression is defined outside the iota block. Continuing the block (e.g. adding clientOptionalResultsetMetadata for 1<<25), or adding a link to the MySQL docs, would keep the flag list easier to read.

moved.

7. The new test files (compress_negotiation_test.go, zstd/*_test.go) are missing the license header that the existing test files have.

added

8. The PR checklist is still unchecked.

checked

@methane
methane merged commit a3303fe into go-sql-driver:master Oct 12, 2026
26 of 27 checks passed
@methane
methane deleted the zstd branch October 12, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants