perf(ingestion): optimize group batch store writes and reads - #69946
Merged
jose-sequeira merged 11 commits intoJul 14, 2026
Conversation
Object and array values in $group_set arrive as fresh JSON parses, so the previous loose reference comparison marked any group with a nested property as changed on every event. That caused constant no-op Postgres writes, version bumps, ClickHouse produces, and cross-pod optimistic-update conflicts for hot group keys.
The group store lagged the person store and showed up as a production flush bottleneck: per-key reads on the Postgres primary, per-row version-CAS writes with lockstep retry storms on hot group keys, inline creation transactions during event processing, and ClickHouse produces awaited inside flush. - add updateGroupsBatch: one UNNEST statement per flush with server-side jsonb merge, so no version assertion and no conflict retries; gated by GROUP_BATCH_WRITING_USE_BATCH_UPDATES (default off) - track properties_to_set deltas so batched writes only send changed keys - skip the conflict retry when the refetched row already contains the update, and sync the cached version after successful writes - create new groups with a single ON CONFLICT insert instead of a wrapping transaction - return ClickHouse messages from flush as side effects instead of awaiting delivery inline; the flush step emits group-specific size warnings - add prefetchGroups and prefetchGroupsStep: one batched read per chunk of $groupidentify events instead of a single-row fetch per group key; gated by GROUPS_PREFETCH_ENABLED (default off) - jitter optimistic-retry intervals so conflicting pods don't re-collide - extend fetchGroupsByKeys with created_at and version (Postgres and personhog) so prefetched entries are complete cache entries
Two pods flushing overlapping group sets in unsorted UNNEST order could deadlock. Sorting by row identity makes concurrent statements lock overlapping rows in the same order; the residual risk degrades to the existing individual-write fallback.
…ted_at and version
jose-sequeira
marked this pull request as ready for review
July 10, 2026 11:40
Contributor
|
Reviews (1): Last reviewed commit: "chore(ingestion): update fetchGroupsByKe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
Contributor
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
pl
reviewed
Jul 10, 2026
- key batch flush result routing by full row identity (team, type index, key) instead of the cache's team:key, and verify multi-group routing in the test - add GroupTypeManager.lookupGroupTypeIndex with own-property and type guards so attacker-supplied group types like __proto__ can't resolve to inherited values and poison SQL parameters; used by both the prefetch step and fetchGroupTypeIndex - remove the unreachable version-mismatch branch from the create path
![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
… effects Group creation awaited the ClickHouse Kafka produce inline per event. Delivery reports on the downstream producer take up to ~seconds under backpressure (p90 765ms observed on the historical lane), so a create-heavy workload on a single distinct_id serialized at ~200ms per event, exceeded the consumer liveness window, and livelocked the partition. Creates and direct-upsert fallbacks now queue their ClickHouse message in the store, and flush() drains them into its results, so they ride the same side-effect production as flushed updates (still awaited before offset commit; ReplacingMergeTree version ordering makes produce order irrelevant). This drops the inline MessageSizeTooLarge handling — oversized group messages surface through the flush step's group-specific warning instead — and the store no longer needs the ingestion outputs, so the constructor loses that parameter.
2 tasks
Three parallel arrays representing (team_id, group_type_index, group_key) tuples invited length mismatches at every call site. Take a single list of key objects and unnest inside the repository instead.
The cache keyed entries by (team_id, group_key) only, so two group types sharing a group key would read and overwrite each other's cached row, merging properties across types and syncing the wrong row after a flush.
pl
approved these changes
Jul 13, 2026
…-batch-store-optimizations # Conflicts: # nodejs/src/ingestion/pipelines/analytics/post-team-preprocessing-subpipeline.ts
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
3 participants
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.
Problem
The group batch store has been showing up as a production slowdown in ingestion flushes. It lagged the person store by a generation:
SELECTon the Postgres primary, one per group key per batch (no batched prefetch).UPDATE, so hot group keys (one big customer's organization receiving$groupidentifyon every pod) caused conflict-retry storms: each lost race cost a failed update, a refetch, and a retry at a fixed 50ms interval with no jitter.Changes
Active immediately:
ON CONFLICT DO NOTHINGinsert instead of a wrapping transaction.GroupFlushResulttype andBatchWritingStoreis now generic, keeping person and group types separate.Behind flags (default off):
GROUP_BATCH_WRITING_USE_BATCH_UPDATES: flush writes all dirty groups in oneUPDATE ... FROM UNNESTwith server-side jsonb merge of per-group deltas. No version assertion, no conflict retries, and concurrent pods cannot clobber each other's keys. Inputs are sorted by row identity for deterministic lock order; missing rows and statement failures fall back to the individual path.GROUPS_PREFETCH_ENABLED: aprefetchGroupsStepwarms the group cache with one batchedfetchGroupsByKeysquery per chunk of$groupidentifyevents, mirroring the persons prefetch including the released-batch guard.fetchGroupsByKeysnow returnscreated_atandversion(Postgres and personhog) so prefetched entries are complete cache entries.Note
Rollout: both flags ship off. The unflagged changes alone should remove most hot-key conflict storms. Suggested order: observe, then enable batch updates (watch
posthog_grouplock waits andbatch_store_flush_latency_seconds{store="group"}), then prefetch. Set the env vars on the ingestion API server deployment as well as the consumers.How did you test this code?
Automated only, no manual testing:
batch-writing-group-store.test.ts(27 passing): new cases cover the regressions each change guards - deep-equal no-op detection (a revert to reference comparison re-dirties groups on every event), the conflict short-circuit (a revert reintroduces retry storms), the batched flush path (delta contents, merged-row ClickHouse message, cache sync, both fallbacks), and prefetch serving upserts without single-row fetches. Existing tests updated for the new create/flush behavior; the suite also got ~100x faster because it no longer burns real retry backoffs.postgres-group-repository.integration.test.ts: newupdateGroupsBatchcase against real Postgres verifies the SQL merge semantics (jsonb||,LEASTcreated_at backdating, version bump, missing rows omitted) that a unit test cannot.flush-batch-stores-step.test.ts: group results are produced as side effects and an oversized group message emits a group-specific warning instead of failing the batch.prefetchGroupsStep.test.ts: only$groupidentifyevents with known group types are prefetched, grouped per batch store.Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code (Fable 5). Started as a review comparing the group and person batch stores to explain production flush slowdowns, then implemented the findings on José's direction. Skills invoked: /writing-tests.
Decisions along the way:
FlushResultperson-only per review feedback: instead of widening the persons type,BatchWritingStorebecame generic and groups got their ownGroupFlushResult.fetchGroupsByKeys(including the personhog gRPC mapping) rather than adding a Postgres-only prefetch method, after validating in the charts repo that personhog is not enabled on the analytics ingestion consumers today but is live on CDP and error-tracking apps sharing this repository code.SELECT ... FOR UPDATE ORDER BYCTE (deterministic but ~2x index probes per row) and chose input sorting instead: free, removes the practical deadlock risk at group-flush sizes, and the individual-write fallback covers the residue. Persons'updatePersonsBatchhas the same unsorted shape at higher volume; a follow-up there is worth a separate PR.