perf: batch ingest paths through per-page write transactions - #138
Merged
Conversation
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: perf: batch ingest paths through per-page write transactions This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
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.
Every ingest path except the provider ran one
BEGIN IMMEDIATEtransaction per message (~10 statements each, serialized throughMaxOpenConns(1)). Benchmarking showed tx-per-page is ~3.4x faster. This converts the remaining paths to accumulate a page of messages intostore.WriteBatchand commit per page, following the provider idiom:summary.Messagesnow comes fromWriteBatchResult.MessagesWritten(identical semantics). Thread parents are batched separately from replies so the reply count stays exact.validateImportMessageKeysreplaces per-message point queries with a chunked bulkIN-list lookup (499/chunk, matching the store preflight precedent), preserving the exact error text.New additive store flag
MessageWrite.SkipWorkspaceCollisionturns cross-workspace message collisions into clean in-batch skips. This is transactionally safe: in the batch flow a collision only surfaces after a zero-row upsert, before any writes for that message.WriteBatchResult.CollisionsSkippedreports the skipped keys so the Slack API sync replays each throughskipMessageCollision, preserving the per-message warning logs #131 introduced — and a collided parent's thread is not queued for thread sync, matching #131's skip semantics. Desktop paths keep their historical silent skip. Covered by tests ininternal/store/batch_test.go(skip-and-commit and default-abort cases).Intentional semantic shift: a mid-page write failure now rolls back the whole page instead of leaving earlier messages committed — the intended atomicity of page-sized transactions.
Proof:
go build ./...,go vet ./...,go test ./...,go test -race ./...all green on top of10e287b.