Skip to content

fix(schema): store msgpack columns as binary in every dialect - #1411

Open
snowyukitty wants to merge 2 commits into
uptrace:masterfrom
snowyukitty:fix/msgpack-dialect-bytes
Open

fix(schema): store msgpack columns as binary in every dialect#1411
snowyukitty wants to merge 2 commits into
uptrace:masterfrom
snowyukitty:fix/msgpack-dialect-bytes

Conversation

@snowyukitty

Copy link
Copy Markdown

Fixes #1219.

The issue is filed against SQLite, but a round trip on every configured database
fails on master. A bun:",msgpack" column cannot be read back on any
dialect:

sql: Scan error on column index 1, name "encoded":
  msgpack: unexpected code=5c decoding map length   (0x5c = '\')
  msgpack: unexpected code=78 decoding map length   (0x78 = 'x')

Those two bytes are the whole story: the value was stored as the text of a
PostgreSQL byte literal, so decoding it later trips over the \ or the x.

There turned out to be two independent causes, and neither fix works without the
other.

1. The column was created as text

Table.newField discovers the SQL type from the Go field type, which for a
msgpack-tagged struct yields a textual column — varchar(255) on MySQL. But a
msgpack column stores the encoded bytes, not the Go type's own representation,
so it needs a binary type regardless of how the field is declared:

field.DiscoveredSQLType = DiscoverSQLType(field.IndirectType)
if field.Tag.HasOption("msgpack") {
	field.DiscoveredSQLType = sqltype.Blob
}

Without this, MySQL rejects a correct binary literal outright
(Error 1366: Incorrect string value), and 255 characters would truncate
anything larger anyway.

2. The value was written with PostgreSQL's literal syntax everywhere

appendMsgpack wrote through internal.HexEncoder, which hard-codes '\x..'
and never consults the dialect — the only value appender that doesn't. SQLite and
MySQL spell byte literals X'..'; MSSQL uses 0x...

The dialect needs the complete encoded value before it can wrap it, so the output
is now staged in a pooled writer and handed to gen.Dialect().AppendBytes.

The writer keeps one bit that HexEncoder also kept, and it earns its place:

encoder what msgpack does result
Marshaler.MarshalMsgpack() returning nil, nil still calls Write(nil) empty literal
CustomEncoder.EncodeMsgpack() returning without writing never calls the writer NULL

An empty buffer cannot tell those apart, so "was the writer used at all" is
tracked explicitly rather than inferred.

3. Which leaves internal/hex.go dead

HexEncoder was that file's entire contents and appendMsgpack was its only
caller, so the second commit removes it along with
github.com/tmthrgd/go-hex, of which it was the only importer.

To be precise about scope: this drops the dependency from the root module, while
36 submodule go.mod files still carry it as // indirect. I left those alone so
the diff stays readable — make go_mod_tidy sweeps them, and I'm glad to add
that as a third commit. So this narrows #1385 to a mechanical tidy rather than
closing it. Please drop the second commit if you'd rather take one of the
existing dependency PRs; after the first commit the file is dead either way.

Verification

internal/dbtest/msgpack_test.go adds a round trip driven by testEachDB, so it
runs against everything in docker-compose.yaml:

dialect master this branch
pg
pgx
mysql8
mysql5
mariadb
mssql2019
sqlite

I ran the entire internal/dbtest suite on both trees against the same
containers and diffed the failure lists: seven failures fixed, none introduced.

Unit coverage lives in schema/appendmsgpack_test.go — dialect syntax, appending
to an in-progress query, PostgreSQL output holding still, the unwritten-versus-
zero-length distinction, a failing encoder producing only an error marker,
zero-length outcomes routed through the dialect, and confirmation that the
bun:",msgpack" tag actually reaches this path via Field.AppendValue. Four of
those fail on master. TestMsgpackWriterReset exercises the staging writer
directly rather than through the pool, since sync.Pool may hand back a
different object and a test that assumes otherwise can pass for the wrong reason.

Also run: go vet ./... and go test -race (in a golang:1.25 container),
GOOS=linux GOARCH=386 go build ./..., and golangci-lint run with findings
identical to master.

Performance

Staging costs one buffer, taken from a pool that drops writers grown past 64 KiB
rather than keeping them alive. On a representative struct with a 512-byte
payload, 14 interleaved rounds of 30,000 fixed iterations:

ns/op B/op allocs/op
before 1762 4153 12
after 1760 4096 1

Paired median +10.5%, slower in 10 of 14 rounds; the medians coincide only
because my machine drifts, so the paired figure is the honest one. Twelve
allocations become one. I haven't isolated how much of the time is staging versus
the dialects using stdlib encoding/hex where go-hex had SIMD — an attempt to
separate them was confounded by escape analysis, so I'd rather not guess.

Things worth knowing before you merge

  • New tables get a binary column. Two things bound that: the change sets
    DiscoveredSQLType only, so an explicit bun:",msgpack,type:..." still wins
    and anyone who already worked around this is untouched; and since msgpack
    couldn't be read back anywhere before, no existing textual column holds msgpack
    that anything successfully reads today. Existing tables keep their old column
    until their owner alters them. Happy to document that, or to gate the type
    change, if you'd prefer.
  • The 64 KiB pool ceiling isn't tested. sync.Pool may return a different
    object or none, so any assertion about its contents can silently pass. I tested
    reset() directly instead and left the ceiling as three lines of visible code
    rather than write a test that could lie.
  • The benchmark is serial and single-sized; I make no claim about concurrency.

snowyukitty added 2 commits July 23, 2026 19:04
A `bun:",msgpack"` column could not be read back on any dialect. Two
independent causes had to be fixed together.

First, Table.newField discovers the SQL type from the Go field type, so a
msgpack column was created textual — varchar(255) on MySQL. The column
holds encoded bytes rather than the Go type's own representation, so it
needs a binary type whatever the field is declared as.

Second, appendMsgpack wrote through internal.HexEncoder, which hard-codes
PostgreSQL's '\x..' byte literal and never consults the dialect, while
every other value appender routes through Dialect.AppendBytes. SQLite and
MySQL spell byte literals X'..' and MSSQL uses 0x.., so the value was
stored as text and could not be decoded on the way back:

    msgpack: unexpected code=5c decoding map length   (0x5c = '\')
    msgpack: unexpected code=78 decoding map length   (0x78 = 'x')

The dialect needs the whole encoded value before it can wrap it, so the
output is staged in a pooled writer and handed to Dialect.AppendBytes.
The writer records whether the encoder wrote at all, because a
CustomEncoder that returns without writing is SQL NULL while a Marshaler
returning no bytes is an empty value, and buffer emptiness cannot tell
the two apart. HexEncoder tracked the same bit.

internal/dbtest/msgpack_test.go adds a round trip driven by testEachDB.
On master it fails for pg, pgx, mysql5, mysql8, mariadb, mssql2019 and
sqlite; with this change all seven pass, and the full dbtest suite gains
no new failures.

Fixes uptrace#1219
internal.HexEncoder was the only consumer of github.com/tmthrgd/go-hex
and appendMsgpack was its only caller, so the file is now dead.

The submodule go.mod files still carry the dependency as an indirect
requirement; `make go_mod_tidy` sweeps those separately.
@snowyukitty
snowyukitty force-pushed the fix/msgpack-dialect-bytes branch from b2f5288 to 081da3f Compare July 23, 2026 10:06
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.

Msgpack tag does not work for sqlite database

1 participant