Skip to content

mssqlserver: store checkpoint LSN as varbinary#4394

Draft
josephwoodward wants to merge 1 commit into
mainfrom
mmt/mssqlserver-lsn-varbinary
Draft

mssqlserver: store checkpoint LSN as varbinary#4394
josephwoodward wants to merge 1 commit into
mainfrom
mmt/mssqlserver-lsn-varbinary

Conversation

@josephwoodward
Copy link
Copy Markdown
Contributor

@josephwoodward josephwoodward commented May 5, 2026

The CDC checkpoint cache stored LSNs in a varchar(100) column. When
writing a []byte to a varchar, go-mssqldb performs an implicit
binary-to-hex-string conversion (e.g. "0x0000002d00000bc3b80003"), so
reads returned the ASCII representation of the LSN rather than the
original binary value. On resume, those ASCII bytes were passed as a
varbinary parameter to the change-table query and, starting with 0x30,
sorted above any real LSN (which starts with 0x00), leaving the stream
spinning with no rows matched.

Change the cache column and stored procedure parameter to varbinary(10)
so the driver transfers the LSN as-is.

Fixes CON-382

Cherry picked from #4304 so I can verify backwards compatibility.

The CDC checkpoint cache stored LSNs in a varchar(100) column. When
writing a []byte to a varchar, go-mssqldb performs an implicit
binary-to-hex-string conversion (e.g. "0x0000002d00000bc3b80003"), so
reads returned the ASCII representation of the LSN rather than the
original binary value. On resume, those ASCII bytes were passed as a
varbinary parameter to the change-table query and, starting with 0x30,
sorted above any real LSN (which starts with 0x00), leaving the stream
spinning with no rows matched.

Change the cache column and stored procedure parameter to varbinary(10)
so the driver transfers the LSN as-is.

Fixes CON-382
@josephwoodward josephwoodward self-assigned this May 5, 2026
CREATE TABLE %s (
cache_key varchar(7) NOT NULL PRIMARY KEY,
cache_val varchar(100)
cache_val varbinary(10)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Schema migration concern: createCacheTable uses IF NOT EXISTS so existing deployments that already have the table with cache_val varchar(100) will keep the old column type after this fix. Meanwhile createUpsertStoredProc uses CREATE OR ALTER, so the stored procedure parameter is updated to @Value varbinary(10). On an upgrade, the proc will then INSERT/UPDATE a varbinary(10) value into a varchar(100) column — MSSQL will perform the same implicit binary-to-hex-string conversion that this PR is trying to eliminate, and the bug reproduces.

Either:

  • migrate the existing column (e.g. ALTER TABLE ... ALTER COLUMN cache_val varbinary(10) after detecting the old type), or
  • document that operators must drop the existing checkpoint cache table on upgrade.

If a manual drop is the intended remediation for CON-382, please add a note in the PR description / release notes since the existing cached LSN is already corrupted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See comment in PR description 👍

@claude
Copy link
Copy Markdown

claude Bot commented May 5, 2026

Commits
LGTM

Review
Targeted, well-scoped fix — switches the CDC checkpoint cache column and stored-proc parameter from varchar(100) to varbinary(10) so the go-mssqldb driver no longer applies an implicit binary-to-hex-string conversion. Test now uses a real 10-byte LSN binary value. License header, error handling, and code style all consistent with project patterns.

One concern:

  1. Schema migration on upgrade: createCacheTable uses IF NOT EXISTS, so existing deployments keep cache_val varchar(100), while createUpsertStoredProc uses CREATE OR ALTER and updates @Value to varbinary(10). The new proc will then write a varbinary(10) value into a varchar(100) column, triggering the same implicit conversion the PR is fixing. Inline comment left for discussion — either migrate the column or call out a manual drop step in the release notes.

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.

2 participants