Repository navigation
feat(clickhouse): add the ClickHouse destination package - #46
Open
gjtorikian wants to merge 7 commits into
Open
gjtorikian wants to merge 7 commits into
gjtorikian wants to merge 7 commits into
Conversation
Mirror streams are ReplacingMergeTree tables with tombstones and _live views; append streams are plain MergeTree. The lease, cursors and schema bookkeeping live in KeeperMap tables, the one compare-and-swap ClickHouse offers, so a Keeper and keeper_map_path_prefix are required. Projected columns are DEFAULT expressions over _raw, materialized when added to an existing table and recomputed on repair. Wires the package into CI (a ClickHouse container with an embedded Keeper), the Docker matrix, release-please and the READMEs.
 [High impact] The new destination can persist WorkOS identity and authentication history in ClickHouse. The PR still needs a write lock that remains effective while an insert is in flight. Findings
Fix with agent prompt### Issue 1
packages/sync-clickhouse/src/db/upsert.ts:420-424
Backfill and refresh run alongside the tail without taking its lease. If both writers read a live row at T1, refresh can prepare a live version at T2 while `deleteRows` prepares a tombstone still versioned at T1. `ReplacingMergeTree` keeps the live T2 row, even if the delete finishes last. The deleted object remains visible.
The same read-before-write gap can lose fields when an event merges against an older snapshot. Serialize writes for each key across processes, or use a write model that preserves deletes and merged fields under overlap.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+355
to
+359
| return { | ||
| key: row.key, | ||
| raw: merge(stored.raw, row.data), | ||
| updatedAt: stored.updatedAt, | ||
| deletedAt: later(stored.deletedAt, row.updatedAt), |
There was a problem hiding this comment.
Backfill and refresh run alongside the tail without taking its lease. If both writers read a live row at T1, refresh can prepare a live version at T2 while deleteRows prepares a tombstone still versioned at T1. ReplacingMergeTree keeps the live T2 row, even if the delete finishes last. The deleted object remains visible.
The same read-before-write gap can lose fields when an event merges against an older snapshot. Serialize writes for each key across processes, or use a write model that preserves deletes and merged fields under overlap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/sync-clickhouse/src/db/upsert.ts
Line: 355-359
Comment:
**Concurrent writes undo deletions**
Backfill and refresh run alongside the tail without taking its lease. If both writers read a live row at T1, refresh can prepare a live version at T2 while `deleteRows` prepares a tombstone still versioned at T1. `ReplacingMergeTree` keeps the live T2 row, even if the delete finishes last. The deleted object remains visible.
The same read-before-write gap can lose fields when an event merges against an older snapshot. Serialize writes for each key across processes, or use a write model that preserves deletes and merged fields under overlap.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.…ckpoint races Every mirror write is read-decide-write, and ClickHouse has no row lock to serialize it, so two writers on one key (the tail beside a backfill, or a poll outliving its lease) judged against the state before the other's insert landed: a merge could drop fields, a tombstone could lose a version tie, an append-only event could land twice. Each batch now holds a per-stream lock in a `_locks` KeeperMap table: strict-mode INSERT to take it, DELETE to release, conditional UPDATE to take over one a crashed process left behind once it expires. A lock wait that runs out is a transient error, so the engine retries rather than quarantines. `migrate` left a view alone whenever it had every column; a foreign definition with every column now goes through `applyLiveView` and is refused, or replaced under `--repair`. A checkpoint that Keeper refused over a version race (the holder's own renewal is enough) re-reads and retries instead of reporting a moved cursor and replaying the poll. Also: release-please calls the publish workflow for this package, and the test server config loses the `--` XML forbids inside a comment, which kept CI's ClickHouse from starting.
…ones at deletion time A batch that stalled past the lock's TTL could still insert rows judged against state another writer has since changed. It now checks it still holds the lock right before writing and fails with LOCK_LOST, a transient error the engine retries from a fresh read. An object delete's tombstone takes the deletion time as its version instead of the row's last update time, so it outranks any older snapshot on the version itself rather than on which insert landed last. The deletion came after anything a listing could still show of the row, so nothing legitimate is outranked.
# Conflicts: # .github/workflows/release-please.yml
… out-of-range offsets The same three fixes Postgres (#49) and MySQL (#50) got. npm installs the bin as a symlink named after the command, which the old argv check did not recognise, so `npx workos-sync-clickhouse` imported the CLI and did nothing. Duplicate keys in a batch are now judged by the instant each timestamp names rather than by its text, so `01:00:00+01:00` no longer outranks `00:30:00Z`. And the timestamp projection range-checks the clock and the offset ahead of the best-effort parser, which read `24:00` as the next midnight and `+00:99` as a zone where the other destinations project NULL.
…ate append rows `init --repair` redefines a column, and its `_live` view keeps the type it declared when it was created: a query through the view then returns the old type or fails to convert. The views of the streams a repair touched are now re-created after it, and the check that leaves an up-to-date view alone compares column types as well as names. Append-only tables are a ReplacingMergeTree keyed by the event id, so a second copy of an event collapses at the next merge, and under FINAL before then. The write path's existence check under the stream lock still keeps copies out in the first place; the table is the backstop for a poll that outlived its lease.
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.
Summary
@workos/sync-clickhouse, a ClickHouse destination with the same CLI, container image andDestinationsemantics as the Postgres and MySQL packages, aimed at what ClickHouse is for: keeping theauthentication.*sign-in log and the event archive past the 90-day retention, with the identity objects mirrored beside them for the joins.ReplacingMergeTree(_updated_at)tables with_deleted_attombstones and generated_liveviews (FINAL, tombstones hidden); append streams are plainMergeTree. Projected columns areDEFAULTexpressions over_raw(JSONExtract(... 'Nullable(T)'),parseDateTime64BestEffortOrNull), with data-skipping indexes where the catalog asks for one.ALTER UPDATEas CAS, verified on a local 26.10 and on ClickHouse Cloud 26.6. That is the source of the package's one extra requirement, a Keeper pluskeeper_map_path_prefix, which the README states at the technology level alongside the single-node config CI uses.ReplacingMergeTreecleared with lightweightDELETE;redactuses lightweightDELETEacross the mirror, the sign-in log and the archive.MATERIALIZEd so old parts stop re-evaluating the expression on every read, andinit --repairfollowsMODIFY COLUMNwith anUPDATE ... WHERE 1, sinceMODIFY COLUMNalone does not recompute stored values.--exclude-streamsand the oversized-stream reporting from feat: Abandon oversized reconcile streams, add excludeStreams #45 into this CLI.Testing
164 tests run against a
clickhouse/clickhouse-server:26.10container started with the config intest/clickhouse/(embedded Keeper,keeper_map_path_prefix); the pure tests run without one and the rest skip. CI gains a ClickHouse job, the Docker workflow gains theclickhousematrix entry, and release-please gains the package. The image builds andstreamsruns from it.