Experimental: changelog table for crdb - #3211
Conversation
appendRelationshipChangelogBatch built one multi-row INSERT with 14 columns per relationship. Postgres/pgx cap bound parameters at 65535, so a BulkLoad of more than ~4600 relationships would overflow the limit and fail the whole transaction, defeating the point of the feature. Split the insert into chunks of changelogInsertChunkSize (default 4000) executed within the same transaction, keeping the per-transaction ordinal counter continuous across chunks so (change_ts, ordinal) uniqueness is preserved. Adds a regression test that lowers the chunk size and bulk-loads across multiple chunks, asserting every row is observed over the changelog watch.
AddChangedDefinition's *core.CaveatDefinition branch looked up a prior entry under nsPrefix+name to decrement its byte size, but caveats are stored under caveatPrefix+name. The mismatch meant re-changing the same caveat within one buffering window never found the prior entry, over-counting currentByteSize on every repeat change. Shared by all datastores' watch path (Postgres, MySQL, and the CRDB changelog path); not data loss, but a real accounting bug. Adds a regression test mirroring the existing namespace-replacement test, which fails against the prior code.
…watch watchViaChangelog always buffers and dedups changes per poll interval via common.NewChanges and does not honor opts.EmissionStrategy, even though the CRDB datastore advertises WatchEmitsImmediately support. A caller requesting EmitImmediatelyStrategy silently gets buffered, checkpoint-grouped delivery instead, and Creates are reported as Touch. Document this as an accepted limitation of the experimental path rather than re-architecting it; immediate emission is moot given the follower-read latency floor anyway.
…ion_metadata table
| delete(record.caveatsDeleted, t.Name) | ||
|
|
||
| if existing, ok := record.definitionsChanged[nsPrefix+t.Name]; ok { | ||
| if existing, ok := record.definitionsChanged[caveatPrefix+t.Name]; ok { |
There was a problem hiding this comment.
this looks like a latent bug to me
There was a problem hiding this comment.
😱 yes, please send a different PR
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| --datastore-disable-watch-support disable watch support (only enable if you absolutely do not need watch) | ||
| --datastore-engine string type of datastore to initialize ("cockroachdb", "mysql", "postgres", "spanner") (default "memory") | ||
| --datastore-experimental-column-optimization enable experimental column optimization (default true) | ||
| --datastore-experimental-crdb-changelog-watch EXPERIMENTAL: use a dual-written changelog table polled at a closed timestamp for Watch instead of a changefeed (CockroachDB driver only). Keeps Watch advancing during bulk loads at the cost of write latency. |
There was a problem hiding this comment.
at the cost of write latency.
we should verify this before merge
| --datastore-experimental-crdb-changelog-watch EXPERIMENTAL: use a dual-written changelog table polled at a closed timestamp for Watch instead of a changefeed (CockroachDB driver only). Keeps Watch advancing during bulk loads at the cost of write latency. | ||
| --datastore-experimental-crdb-changelog-watch-max-offset duration EXPERIMENTAL: safety margin subtracted from the cluster clock to derive the changelog Watch completeness cursor/checkpoint; MUST be >= the cluster's --max-offset (CockroachDB default 500ms) or Watch can miss commits (CockroachDB driver only) (default 250ms) |
There was a problem hiding this comment.
nit: these are the only flags that we added (to serve); why do they also appear under gc and repair?
| delete(record.caveatsDeleted, t.Name) | ||
|
|
||
| if existing, ok := record.definitionsChanged[nsPrefix+t.Name]; ok { | ||
| if existing, ok := record.definitionsChanged[caveatPrefix+t.Name]; ok { |
There was a problem hiding this comment.
😱 yes, please send a different PR
| case tuple.UpdateOperationCreate: | ||
| return "create", nil | ||
| case tuple.UpdateOperationTouch: | ||
| return "touch", nil |
There was a problem hiding this comment.
if i send a touch but the relationship already existed and nothing got written, will this writes an event to the changelog? if the answer is yes, this seems useless 🤔 what consumer would care about this?
| TableCaveat = "caveat" | ||
| TableRelationshipCounter = "relationship_counter" | ||
| TableTransactionMetadata = "transaction_metadata" | ||
| TableRelationshipChangelog = "relationship_changelog" |
There was a problem hiding this comment.
| TableRelationshipChangelog = "relationship_changelog" | |
| TableChangelog = "changelog" |
| // changelogRowFields holds the reconstructed non-key columns of a changelog | ||
| // row, shared by the buffered and immediate scan paths. | ||
| type changelogRowFields struct { | ||
| kind string |
| // distinguishes Touch vs Delete — at the row level a physical insert is | ||
| // indistinguishable from a touch of a previously nonexistent row — so those | ||
| // backends report inserts as Touch. The immediate path emits through | ||
| // streamingChangeProvider, which does represent Create distinctly, so it passes |
There was a problem hiding this comment.
The immediate path emits through streamingChangeProvider, which does represent Create distinctly
what's the reason for this and do we want to preserve it? seems weird that each "mode" returns different things
Note: for reference, hasn't been validated
Description
CockroachDB changefeeds stop emitting
resolvedtimestamps during bulk loads, which stalls SpiceDB's CRDBWatch(it relies on changefeedresolvedcheckpoints to know a revision is complete). This adds an experimental, opt-in, CRDB-only Watch path that doesn't depend on the changefeed for correctness, so Watch keeps advancing under bulk load.Behind
--datastore-experimental-crdb-changelog-watch(default off — when off, behavior is unchanged):relationship_changelogtable (hash-sharded PK on the HLC commit timestamp, row-level TTL) at init.Watchpolls the table at present time instead of consuming a changefeed. The table is append-only, so a present read sees every committed row; completeness comes from the cluster's max clock offset (checkpoint/cursor advances only toclusterNow − maxOffset), not changefeedresolved, so no write is skipped. A lightweight changefeed on the table is used only as a low-latency nudge, with the poll interval as the correctness backstop.Notes:
maxOffset(--datastore-experimental-crdb-changelog-watch-max-offset, default 250ms). Must be ≥ the cluster's--max-offset(CRDB default 500ms) or Watch can miss commits.Testing
Extensive unit and integration tests, but the impact to write throughput has not been measured, that could make this a dealbreaker.
TODO: run with
--datastore-engine cockroachdb --datastore-experimental-crdb-changelog-watchand exercise Watch during a bulk load.