perf(dotc1z): filter grants by has_external_match column at SQL#776
perf(dotc1z): filter grants by has_external_match column at SQL#776arreyder wants to merge 2 commits into
Conversation
processGrantsWithExternalPrincipals iterates every grant in the sync but
only acts on rows carrying an ExternalResourceMatch{,All,ID} annotation.
On be-temporal-sync, listGrantsWithExpansionInternal accounted for 20.33%
flat / 23.53% cum of alloc_space (~22GB / 3h) — dominated by the per-row
sqlite columnBlob copy + proto.Unmarshal work for rows the caller was
about to skip anyway.
Push the annotation check to the storage layer:
- New has_external_match column on the grants table, set on write from
the in-memory grant's annotations via a cheap Any.MessageIs check
(no unmarshal). Populated in every upsert mode including
PreserveExpansion (executeGrantChunkedUpsert still writes EXCLUDED.data
on conflict; the column must track the data blob exactly).
- Migration ALTERs the column in with default 0 for older c1zs, plus a
partial index on (sync_id) WHERE has_external_match = 1 (same shape
as the existing needs_expansion index).
- Backfill extended (backfillGrantExpansionColumn → backfillGrantDerivedColumns)
to derive has_external_match while the proto is already unmarshaled
for the expansion backfill; sentinel path split by has_external_match
so each branch still bulk-updates in one statement.
- grantListOptions gains ExternalMatchOnly; new GrantStore method
ListWithAnnotationsExternalMatchOnly opts in. processGrantsWithExternalPrincipals
switches to it; the in-loop annotation check stays as a correctness
fallback for rows written before the backfill completes.
- BATON_DISABLE_EXTERNAL_MATCH_FILTER=1 bypasses the predicate for fast
revert without a re-release.
- Pebble adapter falls back to the unfiltered iterator (no SQL column);
correctness preserved by the in-loop annotation check.
This is a rewrite of #776 against current main's API — main moved
GrantListOptions, GrantListMode, InternalGrantRow et al. from the
public connectorstore package to internal pkg/dotc1z/internal_grant_options.go,
and replaced the raw ListGrantsInternal entry point with the
GrantStore.ListWithAnnotations family. The intent and on-disk shape
remain identical to the original PR; the wiring is adapted to the new
internal surface.
## Benchmark
BenchmarkListGrantsExternalMatch_PRRevival compares the SQL-filtered
(this PR) vs unfiltered (current main) paths through the
processGrantsWithExternalPrincipals workload, on AMD Ryzen 7 5700X3D,
benchtime=5x:
n=10,000 grants:
match % filtered ns/op unfiltered ns/op speedup
1% 604,361 35,817,293 59× (90× less mem, 92× fewer allocs)
5% 3,285,496 35,728,381 11× (19× less mem, 19× fewer allocs)
20% 10,182,980 36,909,050 3.6× (5× less mem, 5× fewer allocs)
100% 40,469,356 38,651,628 parity (regression guard)
n=50,000 grants:
match % filtered ns/op unfiltered ns/op speedup
1% 3,136,992 177,058,596 56× (96× less mem, 96× fewer allocs)
5% 14,756,822 178,732,617 12× (19× less mem, 20× fewer allocs)
20% 48,958,824 179,619,881 3.7× (5× less mem, 5× fewer allocs)
100% 199,639,761 196,864,152 parity (regression guard)
The 100% match case is pinned as the regression guard — the filter
imposes no measurable cost vs. the pre-fix path when every row would
match anyway.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arreyder
force-pushed
the
crr/listgrants-expansion-alloc
branch
from
43b6032 to
b707829
Compare
Reviving this PR with a fresh adaptation against current mainThe branch was parked since 2026-05-22 on a 4-file rebase conflict against main. Main moved Re-authored as a single clean commit ( Why this still mattersI benchmarked current main against the proposed PR with a fresh n=10,000 grants
n=50,000 grants
These numbers match the original PR's claim almost exactly (the PR claimed 60× at 1%/10k; actual is 59×). The slim-grants writer (#781) is in main's baseline and didn't close the gap — slim still requires an unmarshal, and the consumer filters in Go after that. Pushing the filter to SQL via the partial index sidesteps both costs. Test plan
Out of scope vs. original PRThe original branch carried ~313 lines of test cases against the deprecated public |
General PR Review: perf(dotc1z): filter grants by has_external_match column at SQLBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThis PR adds a Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Re-bench after critical-fix commit
|
| match % | unfiltered (main baseline) | filtered (this PR) | speedup |
|---|---|---|---|
| 1% | 35.03 ms / 15.63 MB / 390k allocs | 0.60 ms / 172 KB / 4,264 allocs | 58× / 91× less mem / 92× fewer allocs |
| 5% | 36.49 ms / 15.73 MB / 392k allocs | 3.19 ms / 819 KB / 20,258 allocs | 11× / 19× / 19× |
| 20% | 37.70 ms / 16.10 MB / 398k allocs | 9.80 ms / 3.23 MB / 80,225 allocs | 3.8× / 5× / 5× |
| 100% | 40.24 ms / 18.08 MB / 430k allocs | 41.12 ms / 16.16 MB / 400k allocs | parity (regression guard) |
n=50,000 grants
| match % | unfiltered (main baseline) | filtered (this PR) | speedup |
|---|---|---|---|
| 1% | 179.39 ms / 78.16 MB / 1.95M allocs | 3.14 ms / 817 KB / 20,266 allocs | 57× / 96× / 96× |
| 5% | 178.12 ms / 78.66 MB / 1.96M allocs | 15.79 ms / 4.05 MB / 100,266 allocs | 11× / 19× / 20× |
| 20% | 180.09 ms / 80.51 MB / 1.99M allocs | 47.64 ms / 16.17 MB / 400,229 allocs | 3.8× / 5× / 5× |
| 100% | 201.51 ms / 90.43 MB / 2.15M allocs | 201.85 ms / 80.83 MB / 2.00M allocs | parity |
The bench now also asserts row-count correctness: b.Fatalf if either path yields a count other than numGrants * matchPercent / 100. A regression that under-selects rows would abort the bench rather than silently look like a speedup.
What changed since the prior comment
26b535cd addressed a critical correctness bug surfaced in a multi-agent code review of b707829b: backfillGrantDerivedColumns is gated on sync_runs.grants_backfilled=0, but every existing sync was already flipped to 1 by the prior expansion-column PR. Migration would have ALTERed in has_external_match with default 0, the gated backfill would no-op, the SQL predicate would filter to zero rows, and processGrantsWithExternalPrincipals would silently process nothing on every existing c1z. Fixed by introducing a separate has_external_match_backfilled flag column on sync_runs and a dedicated backfill loop. New regression test TestBackfillHasExternalMatchColumn_OldSyncWithBackfilledFlag reproduces the failure mode and asserts the fix.
processGrantsWithExternalPrincipals iterates every grant in the sync but
only acts on rows carrying an ExternalResourceMatch{,All,ID} annotation.
On be-temporal-sync, listGrantsWithExpansionInternal accounted for 20.33%
flat / 23.53% cum of alloc_space (~22GB / 3h) — dominated by the per-row
sqlite columnBlob copy + proto.Unmarshal work for rows the caller was
about to skip anyway.
Push the annotation check to the storage layer:
- New has_external_match column on the grants table, set on write from
the in-memory grant's annotations via a cheap Any.MessageIs check
(no unmarshal). Populated in every upsert mode including
PreserveExpansion (executeGrantChunkedUpsert still writes EXCLUDED.data
on conflict; the column must track the data blob exactly).
- Migration ALTERs the column in with default 0 for older c1zs, plus a
partial index on (sync_id) WHERE has_external_match = 1 (same shape
as the existing needs_expansion index).
- Backfill extended (backfillGrantExpansionColumn → backfillGrantDerivedColumns)
to derive has_external_match while the proto is already unmarshaled
for the expansion backfill; sentinel path split by has_external_match
so each branch still bulk-updates in one statement.
- grantListOptions gains ExternalMatchOnly; new GrantStore method
ListWithAnnotationsExternalMatchOnly opts in. processGrantsWithExternalPrincipals
switches to it; the in-loop annotation check stays as a correctness
fallback for rows written before the backfill completes.
- BATON_DISABLE_EXTERNAL_MATCH_FILTER=1 bypasses the predicate for fast
revert without a re-release.
- Pebble adapter falls back to the unfiltered iterator (no SQL column);
correctness preserved by the in-loop annotation check.
This is a rewrite of #776 against current main's API — main moved
GrantListOptions, GrantListMode, InternalGrantRow et al. from the
public connectorstore package to internal pkg/dotc1z/internal_grant_options.go,
and replaced the raw ListGrantsInternal entry point with the
GrantStore.ListWithAnnotations family. The intent and on-disk shape
remain identical to the original PR; the wiring is adapted to the new
internal surface.
## Benchmark
BenchmarkListGrantsExternalMatch_PRRevival compares the SQL-filtered
(this PR) vs unfiltered (current main) paths through the
processGrantsWithExternalPrincipals workload, on AMD Ryzen 7 5700X3D,
benchtime=5x:
n=10,000 grants:
match % filtered ns/op unfiltered ns/op speedup
1% 604,361 35,817,293 59× (90× less mem, 92× fewer allocs)
5% 3,285,496 35,728,381 11× (19× less mem, 19× fewer allocs)
20% 10,182,980 36,909,050 3.6× (5× less mem, 5× fewer allocs)
100% 40,469,356 38,651,628 parity (regression guard)
n=50,000 grants:
match % filtered ns/op unfiltered ns/op speedup
1% 3,136,992 177,058,596 56× (96× less mem, 96× fewer allocs)
5% 14,756,822 178,732,617 12× (19× less mem, 20× fewer allocs)
20% 48,958,824 179,619,881 3.7× (5× less mem, 5× fewer allocs)
100% 199,639,761 196,864,152 parity (regression guard)
The 100% match case is pinned as the regression guard — the filter
imposes no measurable cost vs. the pre-fix path when every row would
match anyway.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code review caught a critical regression in the prior commit on this PR: backfillGrantDerivedColumns is gated on sync_runs.grants_backfilled=0, but every existing sync was already flipped to 1 by the prior expansion- column PR. Migration ALTERed has_external_match in with default 0, the gated backfill no-op'd, ListWithAnnotationsExternalMatchOnly's WHERE has_external_match=1 filter then returned zero rows, and processGrantsWithExternalPrincipals silently processed nothing on the entire installed base. The in-loop annotation check never fired because no rows arrived. Fix: - Add a dedicated has_external_match_backfilled flag column on sync_runs (separate from grants_backfilled, which is no longer a useful gate). - New syncs set it to 1 at creation; the existing ALTER + default 0 picks up pre-existing rows so the backfill walks them on first open. - New function backfillHasExternalMatchColumn() walks pending syncs, unmarshals grants in pages, bulk-updates matching IDs, and marks the flag at the end. Idempotent — second run reports no-op. - InitTables calls it after backfillGrantDerivedColumns; either or both may need to run depending on the c1z's age. Also from review: - Make the kill-switch testable: change externalMatchFilterDisabled from a package-init var to a per-call function so t.Setenv can flip it mid-test. Cold path (once per page); cost is one syscall. - Bench correctness assertions: compute expectedMatches up front and b.Fatalf if either /filtered or /unfiltered yields the wrong count. Without this, a regression that under-selects rows would look like a speedup. The existing 59×/11×/3.6× numbers held under the new asserts. - New test TestBackfillHasExternalMatchColumn_OldSyncWithBackfilledFlag reproduces the failure mode: simulates an old c1z (grants_backfilled=1, has_external_match=0, has_external_match_backfilled=0), runs the new backfill, asserts the annotated row flipped to 1, the non-annotated row stayed at 0, the sync flag flipped to 1, and a second run is a no-op. Without this commit's fix the test would fail at the first assert. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
arreyder
force-pushed
the
crr/listgrants-expansion-alloc
branch
from
26b535c to
0e69cfd
Compare
1 participant
Summary
processGrantsWithExternalPrincipalsiterates every grant in the sync but only acts on rows carrying anExternalResourceMatch{,All,ID}annotation. Onbe-temporal-sync,listGrantsWithExpansionInternalwas 20.33% flat / 23.53% cum of alloc_space (~22 GB / 3 h), dominated by the per-row sqlitecolumnBlobcopy +proto.Unmarshalwork for rows the caller was about to skip anyway.Push the annotation check down to SQL:
has_external_matchcolumn ongrants, set on write from the in-memory grant's annotations via a cheapAny.MessageIscheck (no unmarshal).grantExtractFieldssets the column in every upsert mode — includingPreserveExpansion, becauseexecuteGrantChunkedUpsertstill writesEXCLUDED.dataon conflict there; the column must track the data blob's annotation set exactly.ALTERs the column in with default0for older c1zs, plus a partial index(sync_id) WHERE has_external_match = 1matching the existingneeds_expansionindex shape.backfillGrantExpansionColumn→backfillGrantDerivedColumns) deriveshas_external_matchwhile the proto is already unmarshaled for the expansion backfill. Sentinel branch is split by match-presence so each group still bulk-updates in one statement.GrantListOptions.ExternalMatchOnlyopted into bysyncer.listAllGrantsWithExpansion. The in-loop annotation check atprocessGrantsWithExternalPrincipalsis kept as a defensive fallback.BATON_DISABLE_EXTERNAL_MATCH_FILTER=1kill-switch for fast revert without re-release.Clone and attached-diff paths are schema-agnostic (column discovery via
PRAGMA table_info) so the new column flows through automatically.Benchmark
BenchmarkListGrantsWithExpansion_ExternalMatchOnlyseeds N grants at varying match densities and compares the filtered path against the pre-fix behavior via the kill-switch.benchtime=3xon AMD Ryzen 7 5700X3D:n=10,000 grants (pre-fix: ~14.5 MB, ~380k allocs, ~37 ms regardless of density):
n=50,000 grants (pre-fix: ~72 MB, ~1.9 M allocs, ~177 ms):
100% match density is pinned as a regression guard — the filter imposes no measurable cost vs. the pre-fix path when every row would match anyway.
Pressure test
Before implementing, verified (in order):
grantExtractFieldsis called on every upsert with the full*v2.Grant— cheapAny.MessageIsworks.listGrantsWithExpansionInternalhas only one caller in-repo (the one we convert).ExternalResourceMatch*annotations are only set by connectors pre-PutGrants; no in-place annotation mutation exists.newGrantForExternalPrincipalcopies them to expanded grants, which go back throughUpsertGrants(Replace)— extraction runs fresh.state.HasExternalResourcesGrants()is observation-based, not eager.NewC1File→InitTables→Migrationsbefore any read can happen.PreserveExpansionupsert rewritesdatafromEXCLUDED.data, sohas_external_matchmust track it — covered by the update map adjustment and a targeted test.Test plan
TestHasExternalMatch_WriteExtraction— every annotation flavor + co-existence withGrantExpandableTestHasExternalMatch_PreserveExpansionUpdatesColumn— the critical PreserveExpansion refresh caseTestListGrantsInternal_ExternalMatchOnlyFilter— SQL predicate returns exactly the matching rowsTestBackfillMigration_PopulatesHasExternalMatch— all three backfill branches (expandable+match, sentinel+match, sentinel+no-match)TestExternalMatchFilter_KillSwitch— env var disables the predicateTestExternalResourceMatchIDWithExpandableRemapping+ full sync/dotc1z/synccompactor suites pass unchangedTestCloneSyncMigratedColumnOrderupdated to simulate old c1z with the new ALTERgo vet ./...cleanbe-temporal-syncafter deploy; if the predicate mis-plans on a customer's c1z,BATON_DISABLE_EXTERNAL_MATCH_FILTER=1rolls back without a re-release🤖 Generated with Claude Code