Grant Hash Digest Index + leaf only tree#977
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // IfNewer is by definition a post-seal mutation path, so the | ||
| // grant digests (cloned along with the sync's keyspace) are | ||
| // kept in step incrementally. | ||
| mm := newGrantDigestMutator(e) |
There was a problem hiding this comment.
🟡 Suggestion (medium-low confidence): Unlike PutGrantRecords, this path has no dedup pre-pass. If the same external_id appears twice in one call and both pass the freshness check, e.db.Get(key) returns the same stored value both times (it doesn't see in-batch writes), so mm.removeGrant(old) runs twice while addGrant runs for each duplicate. The repeated remove folds the old content hash into the root XOR an even number of times (cancelling it) while the count delta stays consistent — so applyPartition's negative-count guard won't trip and the corrupted digest persists silently. The digest does stay consistent with the equally-orphaned hash index, so end-to-end diff impact is bounded, but consider adding the same last-occurrence dedup pass PutGrantRecords uses (or documenting that callers must pre-dedup).
| return &Options{ | ||
| durability: DurabilitySync, | ||
| slowQueryThreshold: 5 * time.Second, | ||
| grantDigestIndex: true, |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): The grant digest index defaults to on, so every connector that upgrades the SDK starts writing an extra per-grant index row (+8-byte value) and building per-entitlement digests at seal — a default-behavior change that grows every .c1z and adds seal-time work, even for files that are never diffed. The repo's compatibility criteria prefer new behavior to default to the old behavior and be opt-in where possible. This appears deliberate (the diff feature needs the index present by default), so non-blocking — but pkg/sdk/version.go is unchanged (v0.15.5); consider a minor bump / migration note so downstreams notice the new default write/size cost.
| return nil, err | ||
| } | ||
|
|
||
| if !okA || !okB { |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): When a digest root is absent on either side this falls back to computeBucketDigest, which folds the by_entitlement_principal_hash index. For a file with no hash index — a read-only open of a pre-migration file (applyIndexMigrations is skipped when readOnly), or one written with WithGrantDigestIndex(false) — that fold yields count 0, so a future diff consumer comparing against a populated file would silently read it as "entitlement has zero grants." options.go already documents that the diff driver must detect index absence (via the manifest) and rebuild-or-error; that guard isn't in this PR. Worth ensuring the consumer enforces it before this path ships, so an absent index can't be mistaken for an empty entitlement.
|
@- |
e196316 to
2c8a4ff
Compare
General PR Review: Grant Hash Digest Index + leaf only treeBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryFull-diff security and correctness pass over the whole PR (Pebble engine grant hash index + XOR set digests, plus the connectorstore reader capability and compactor/cleanup wiring). This is an additive, backward-compatible change: the new Note (by design, not a finding): sqlite-to-pebble conversions ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
| // Set false on files that will never be grant-diffed (e.g. local CLI | ||
| // syncs, connector development) to skip the per-grant index write and | ||
| // the seal-time digest fold. NOTE: a file written with this off has no | ||
| // hash index, so a future cross-file diff MUST detect its absence |
There was a problem hiding this comment.
grantDigestIndex is in memory only, so how does a "future cross-file diff" detect its absence? What is written to the disk that can be detected?
| // index on an existing file requires a fresh sync or explicit rebuild, | ||
| // not an Open-time backfill. | ||
| func (e *Engine) backfillGrantHashIndexAndDigests(ctx context.Context) error { | ||
| if !e.opts.grantDigestIndex { |
There was a problem hiding this comment.
when indexMigrations are applied and they return nil we still record the index version with writeAppliedIndexVersion in applyIndexMigrations, so in the case here where we return nil when grantDigestInex is nil, I'm not sure if it is possible to back fill later if grantDigestIndex is enabled
There was a problem hiding this comment.
Before merge:
- We should profile this. There is possibly a way to do the work in a single pass or way more efficiently.
- We need to run the benchmarks for expansion and compaction to see what it does to perf.
- We need to run the benchmark for the whale C1z to see what happens to perf, there.
Then, we can decide if this tradeoff is a net savings.
| Name: "grant_by_entitlement_principal_hash", | ||
| Version: 2, | ||
| Apply: func(ctx context.Context, e *Engine) error { | ||
| return e.backfillGrantHashIndexAndDigests(ctx) |
There was a problem hiding this comment.
Historically, grant migrations cause incidents. For pebble, do we record if we have run a migration? If not, we probably should. That info could potentally go in the c1z header, too. A differetn strategy would be to not run the migration at all, and opt into the new behavior on the c1 side if the c1z supports it.
| // back to an on-demand index fold, and the on-Open migration | ||
| // backfills the stored digests when the file is next opened | ||
| // writable. Skipped entirely when the digest index is disabled. | ||
| if s.b.e.opts.grantDigestIndex { |
There was a problem hiding this comment.
Stepping through 4 objects violates Code Complete.
| // consumer onto the on-demand index fold, and the on-Open migration | ||
| // backfills it next time the file opens writable. Skipped when the | ||
| // digest index is disabled — there is no hash index to fold over. | ||
| if a.engine.GrantDigestIndexEnabled() { |
There was a problem hiding this comment.
This will load every single grant after we sync, but we already walked through all of them in the process of syncing. Can we do this work incrementally?
| // not extract (sources) — unmarshal only on this | ||
| // non-fresh path. | ||
| old := &v3.GrantRecord{} | ||
| if err := unmarshalRecord(oldVal, old); err != nil { |
There was a problem hiding this comment.
This should be a shallow unmarshal for the fields we need (probably just the digest), so that we don't allocate a ton of pointers on the heap on the hot path. Can we get the other fields out of new record or whatever index is handy?
| closer.Close() | ||
| return fmt.Errorf("PutGrantRecords: unmarshal old %q: %w", r.GetExternalId(), err) | ||
| } | ||
| if err := mm.removeGrant(old); err != nil { |
There was a problem hiding this comment.
mm or removeGrant should have a more descriptive name.
| return nil | ||
| } | ||
|
|
||
| func (m *digestMutator) applyPartition(batch *pebble.Batch, mp *mutatorPartition) error { |
There was a problem hiding this comment.
This function looks like it is doing too much work.
| curCount int64 | ||
| curDigest [hashLen]byte | ||
| ) | ||
| val, closer, err := m.e.db.Get(n.key) |
There was a problem hiding this comment.
This is doing a random get in a loop per grant. This is probably bad.
| // which folds fields the raw index scan does not extract | ||
| // (sources). | ||
| old := &v3.GrantRecord{} | ||
| if err := unmarshalRecord(oldVal, old); err != nil { |
There was a problem hiding this comment.
Same story here, we try very hard in this function to unmarshal any protos.
2c8a4ff to
f710243
Compare
| // IfNewer is by definition a post-seal mutation path, so the | ||
| // grant digests (cloned along with the sync's keyspace) are | ||
| // kept in step incrementally. | ||
| mm := newGrantDigestMutator(e) |
There was a problem hiding this comment.
🟡 Suggestion: The digest mutator is created here unconditionally, unlike PutGrantRecords/PutExpandedGrantRecords, which gate it on e.opts.grantDigestIndex. With the index disabled, writeGrantIndexes (line 86) skips writing the hash-index entry, but mm still runs. For a file synced with the index on and later reopened with WithGrantDigestIndex(false), the stored roots are present, so mm updates the digests while the hash index goes unwritten — leaving the digest diverged from its index until the next rebuild. Consider gating mm (and the removeGrantRaw/addGrant/apply calls) on e.opts.grantDigestIndex to match the other write paths. Narrow edge case (documented as requiring a fresh sync), hence non-blocking.
| // buildPartitionDigest can read the count from the stored root and | ||
| // skip its countKeysInRange pass. Non-fatal for the same reason as | ||
| // the digest build itself. | ||
| if err := a.engine.FlushFreshRoots(); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): FlushFreshRoots is documented "Must be called within writeMu", but EndSync calls it holding only a.mu, not the engine's writeMu. Unlike PersistFreshRoots (which wraps persistFreshRoots in withWrite) and unlike Close (which calls writeWG.Wait() first), this path mutates the freshRoots map with no serialization against in-flight PutGrantRecords/PutExpandedGrantRecords writers (which mutate the same map under writeMu). If any grant write is concurrent with EndSync, this races the map. Consider having FlushFreshRoots wrap the flush in withWrite like PersistFreshRoots, or document/enforce the "no writes in flight at EndSync" sequencing.
3878c04 to
402d85e
Compare
| if entID != "" && principalRT != "" && principalID != "" { | ||
| bh := principalBucketHash(principalRT, principalID) | ||
| if err := batch.Delete(encodeGrantByEntPrincHashIndexKey(entID, bh, principalRT, principalID, externalID), nil); err != nil { | ||
| return scratch, err | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Every other delete in this ...Scratch variant reuses the scratch buffer (append(scratch[:0], ...)), but this hash-index delete allocates a fresh key via encodeGrantByEntPrincHashIndexKey. Since appendGrantByEntPrincHashIndexKeyBytes already exists as the byte-view/append form, you could keep the buffer-reuse contract here to avoid a per-mutation allocation. Minor perf nit, not correctness. (confidence: high this diverges from the pattern; low that it matters in practice)
|
Benchmarks run with and without the Feature, adds ~1 second at 10M grants (from 7s to 8s) |
|
I just merged my other branch. Apologies for the conflicts. Fable and I talked about this branch a bit, and this is what it suggests another agent do: Task: Port the grant-digest system from
|
|
@mj-palanker Can we close this PR? |
3 participants
Introduces grant content hashing.
Adds a new grant index where the key sorts grants into buckets based on the hash of the principal ID such that each entitlement's grants are now sorted into roughly equal sized buckets. The value is the hash of the content of the grant, such that it genuinely should describe it's state.
each "bucket" is actually a set number of bits into the 2 byte hash s.t. we can dynamically "bucket" at higher / lower resolution without re-indexing.
any two c1zs could compare any given bucket for grant equality by just scanning this index.
We also add new objects which stores an XOR rollup of the hashes on an entitlement.
for a given bit count k, we store 2^k leaf nodes where the value is the XOR of all hashes in their bucket.
we also store a root note which is the XOR of ALL grants on the entitlement (equivalent to the rollup of the leaf nodes)