Use sequence-backed IDs on CockroachDB so the web UI can address them - #1017
Open
cycsmail wants to merge 2 commits into
Open
Use sequence-backed IDs on CockroachDB so the web UI can address them#1017cycsmail wants to merge 2 commits into
cycsmail wants to merge 2 commits into
Conversation
Owner
|
can't a migration fix existing tables? |
Author
|
Yep, fair point. Added two migrations (split DML/DDL like 304a32906234 does for cockroach): the first renumbers any existing ids above 2^53 down into the safe range and repoints the FKs, the second swaps the id defaults from unique_rowid() to sequences. Also found why the cockroach CI job went red: sql_sequence_cached gives each connection its own block of ids so they stop following insertion order, which broke the vendor sort test. Switched to plain sql_sequence and verified the whole thing against a real cockroach v23.1.2, old huge-id rows and all. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #797.
CockroachDB expands SERIAL primary keys to
DEFAULT unique_rowid(), which produces 64-bit IDs. JavaScript numbers are only exact up to 2^53, so the web UI rounds them: the database hands out vendor ID 1134663890672549889, the UI sends back 1134663890672549900, and every follow-up request 404s.This asks CockroachDB for sequence-backed SERIAL instead (
serial_normalization=sql_sequence_cached), so tables are created with ordinary small IDs. It goes in as a per-connection server setting rather than a SET statement because it has to be in force when the CREATE TABLE DDL runs, and the Alembic migrations build their connection through the sameDatabase.connect().One catch worth knowing about:
serial_normalizationonly applies at table creation time, so this fixes fresh databases. An existing CockroachDB install already hasunique_rowid()baked into its column defaults and rows with 64-bit IDs that can't be shrunk after the fact, so recreating the database is the way out there.Added two unit tests that check the setting is passed for cockroachdb and not for the other backends; they and the existing suite pass. I don't have a live CockroachDB here, so the integration side is on the CI cockroachdb job.