feat: unblock multi-state loading (address uniqueness keyed on region) - #7
Merged
Conversation
…load
ohio_addresses.hash is HouseNumber|Street|Unit|City|Postcode and the column was
UNIQUE. No state anywhere in it.
With a postcode present that mostly did not matter -- US ZIPs are
state-specific, so they disentangle the key -- but ingestion requires only a
house number and a street, and county GeoJSON extracts frequently ship without
a ZIP column. For those rows the key collapses to house number, street, unit
and city, and city names collide across states constantly: Springfield,
Columbus, Franklin.
The failure was silent and looked like success. A colliding row hit the unique
constraint, the importer counted it as a duplicate, and a load that quietly
discarded half a state reported "N records imported, M duplicates skipped" and
completed green. A test now pins that old behaviour so the regression stays on
the record.
Migration 23 changes the uniqueness key rather than the hash contents.
Recomputing every hash would rewrite all ~6M rows; keying on (hash, region) is
one index build and means the same thing. It also:
- backfills region to 'OH' where NULL or empty, since NULLs are distinct in a
unique index and those rows could otherwise duplicate without limit
- upper-cases existing values, because 'oh' and 'OH' are different keys and a
lowercase upload would reintroduce the collision
- adds CHECK (region <> ''), since NOT NULL does not stop an empty string and
'' would put every stateless row in one shared bucket
- indexes region, without which filtering by state is unusable
No DEFAULT on region, deliberately: a default would silently label a future
non-Ohio import as Ohio, which is the same quiet corruption being fixed.
The other half is a state filter. Filtering by county alone is ambiguous once
two states exist, so ?state=OH now narrows it, case-insensitively.
The down migration can legitimately fail once a second state is loaded -- no
single-column unique constraint can hold rows that share a hash across regions.
That is the rollback refusing to destroy data.
COST: the unique index build takes a SHARE lock, so reads continue and writes
wait; on ~6M rows expect tens of seconds. Writes here are imports, which are
deliberate admin actions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3
**The migration blocked reads, not just writes.** ALTER TABLE ... SET NOT NULL takes ACCESS EXCLUSIVE -- verified, not assumed -- and a lock once taken is held until commit. Taking it before the index build meant every /search and /geocode query blocked for the whole build, while migrations run asynchronously against a server already serving. The two index builds (SHARE) now come first and everything needing ACCESS EXCLUSIVE is last, holding it only for its own validation scan. The cost note said "reads continue"; it was wrong. **The down migration could leave the table with no uniqueness at all.** Its own comment says the final ADD CONSTRAINT is expected to fail once two states are loaded -- but the composite key and the CHECK were already dropped by then, so every ON CONFLICT (hash, region) would fail with 42P10 and deduplication would be off entirely. Now one transaction, so the documented "correct outcome" is a clean no-op rather than a half-dropped schema. **ON CONFLICT (hash, region) shipped ahead of the index it needs.** Migrations run asynchronously while upload endpoints are live, so a deploy-window import failed every batch and marked the dataset failed -- indistinguishable from a corrupt file. This is the third time this branch family has hit that window, so it is now a named guard: database.RequireSchemaVersion, called at the top of the import path. Read paths can degrade; a write path that needs a constraint cannot. **The legacy loader was updated for the new conflict target but not the new invariants.** It truncated "Ohio" to "Oh" -- a different uniqueness bucket from "OH" and invisible to a region filter -- and produced "" when an extract had no state, which the region CHECK rejects on every row. Since that error text is not "duplicate key", the loop logged per row and returned 0 inserted with a nil error: a total failure reported as success. It now normalises through a real state-code check. **Geocoding ignored the state in the query.** The parser extracts one and searchByComponents discarded it, so "100 Main St, Springfield, IL" matched the Ohio row on street and city and returned it at full confidence -- the exact collision this branch exists to prevent, on the endpoint that matters most. State is now an anchor on every tier: what relaxes as tiers widen is street-level detail, never the state. **An uploaded state code was upper-cased but never validated.** "HO" for "OH" now creates a separate dedup bucket, so the import succeeds green and a re-upload under the correct code inserts a second full copy. Validated at all three upload entry points. **DROP CONSTRAINT IF EXISTS failed open.** If the production constraint carries another name it is a silent no-op, leaving a live global unique hash beside the new key while the migration logs success -- the same silent loss being fixed. It now asserts against pg_constraint and errors. **"Every row is Ohio" was an assumption.** Now self-verifying: the migration refuses to stamp OH onto blank regions if any other state is present. Verified by seeding an IL row and watching it refuse. The two full-table UPDATEs are also one pass now rather than two rewrites of ~6M rows. **?state=%20 and ?state=Ohio returned an empty page with no explanation.** Trimmed and validated at the edge, 400 with an example. Test fixtures carry the region CHECK and schema_migrations, so the cases those guard fail in the suite rather than only in production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3
# Conflicts: # services/address_service.go
They arrived on separate branches and first met here. Both extend the same hand-numbered placeholder sequence -- the bbox consumes four positions -- and a mistake there binds the wrong value to the wrong column, which returns plausible rows from the wrong place rather than an error. Seeds the same coordinates in two states so only the state filter can separate them, then checks bbox alone, bbox plus state, and bbox plus state plus a text query, each adding placeholders of its own. The fixture also gains the generated fts column and its indexes: without them a text query combined with a territory filter could not run at all, which is why this combination had never been exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
ohio_addresses.hashisHouseNumber|Street|Unit|City|Postcodeand the columnwas
UNIQUE. There is no state anywhere in it.With a postcode present that mostly does not matter — US ZIPs are
state-specific, so they disentangle the key. But ingestion requires only a house
number and a street, and county GeoJSON extracts frequently ship without a ZIP
column. For those rows the key collapses to house number, street, unit and
city, and city names collide across states constantly.
The failure is silent and looks like success. A colliding row hits the
unique constraint, the importer counts it as a duplicate, and a load that
quietly discarded half a state reports
N records imported, M duplicates skippedand completes green.That is a test, pinning the old behaviour so the regression stays on the
record.
The key, not the hash
Recomputing every hash would rewrite all ~6M rows. Keying on
(hash, region)is one index build and means exactly the same thing.
Migration 23 also handles what that key needs to actually hold:
regionto'OH'where NULL or empty — NULLs are distinctin a unique index, so those rows could otherwise duplicate without limit.
'oh'and'OH'are different keys, so alowercase upload would reintroduce the collision. Ingest normalises too.
CHECK (region <> '')—NOT NULLdoes not stop an empty string, and''would put every stateless row into one shared bucket.region— without it, filtering by state is unusable.No
DEFAULTon region, deliberately: a default would silently label a futurenon-Ohio import as Ohio, which is the same quiet corruption being fixed.
The other half: a state filter
?state=OH, case-insensitive. Filtering by county alone is ambiguous once twostates exist — roughly two dozen states have a Franklin County. A test seeds
Franklin County in both OH and IN and asserts the unfiltered count is 3 and the
Ohio-filtered count is 2.
Rollback
The down migration can legitimately fail once a second state is loaded: no
single-column unique constraint can hold rows sharing a hash across regions.
That is the rollback refusing to destroy data, and the fix is to roll forward.
Cost and verification
The unique index build takes a SHARE lock — reads continue, writes wait; on ~6M
rows expect tens of seconds. Writes here are imports, which are deliberate admin
actions.
Applied twice: onto a database carrying the real chain (with NULL and lowercase
regions seeded, both normalised correctly) and 1→23 from scratch. Verified after
migration that the old constraint is gone, the new key is present, region is
NOT NULL, and the same address in two states keeps both rows. Full suite green
against four probe databases.
🤖 Generated with Claude Code
https://claude.ai/code/session_01S2nhUJ4Zdsxf7cP9DapsY3