Skip to content

Reduce memory on insert operations: Remove one of the copies of the batch that we keep in memory#109

Merged
BentsiLeviav merged 6 commits into
mainfrom
clickhouse/reduce-memory-used-in-insert-operations
Jul 19, 2026
Merged

Reduce memory on insert operations: Remove one of the copies of the batch that we keep in memory#109
BentsiLeviav merged 6 commits into
mainfrom
clickhouse/reduce-memory-used-in-insert-operations

Conversation

@koletzilla

Copy link
Copy Markdown
Contributor

Summary

Related to #107

Problem: Syncs on wide tables were hitting the container memory limit (OOM) during WriteBatch. For each insert batch (up to 100k rows), the connector held three full copies in memory at once:

  • The raw CSV rows ([][]string)
  • A converted copy with every cell boxed as interface{} ([][]any, typically the largest of the three),
  • And the clickhouse-go column block built by batch.Append.

Solution: Eliminate the boxed intermediate copy. InsertBatch now takes a lazy row sequence (iter.Seq2[[]any, error]) instead of a materialized slice, and ReplaceBatch converts each CSV row via ToInsertRow on the fly as it's appended to the driver batch — so each converted row is garbage-collectable immediately after Append. Peak memory per batch drops from raw + boxed + driver block to raw + driver block.

The raw CSV batch is intentionally kept: inserts are retried on network errors, so the sequence must be re-iterable (documented on InsertBatch). Conversion errors abort the insert and are not retried, as before.

As a side simplification, MergeUpdatedRows now returns a compacted slice instead of a slice with holes plus a skipIdx map.

No user-facing changes.

Follow-ups planned:

  • Set size batches by bytes apart from row count so it's also limited by it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Reduces peak memory usage during WriteBatch insert operations by removing the materialized “boxed” intermediate [][]any/[][]interface{} copy and instead converting CSV rows to driver rows lazily while appending to the ClickHouse batch.

Changes:

  • Refactors InsertBatch to accept a re-iterable lazy row sequence (iter.Seq2[[]any, error]) and converts rows on-the-fly during batch.Append.
  • Simplifies MergeUpdatedRows to return a compacted [][]any (no “holes” + skipIdx map), skipping and warning on missing PK matches.
  • Updates ReplaceBatch/UpdateBatch to use MapErr(slices.Values(...), ...) for lazy row production.

Correctness & Data Integrity

  • Retry semantics are preserved (the sequence must be re-iterable per attempt; doc comment correctly calls this out).
  • Update path behavior remains equivalent: rows with missing table matches are still skipped; now the “no rows to insert” case is handled explicitly in UpdateBatch.

ClickHouse-specific concerns

  • No change to the underlying PrepareBatch/Append/Send buffering behavior; this is correctly scoped to removing one in-process copy.

Error handling & observability

  • Errors remain wrapped; conversion errors now abort the batch to return the connection to the pool.

Tests

  • Repository includes e2e coverage exercising insert/update flows via the SDK tester (destination/main_e2e_test.go). (Not executed as part of this review.)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
destination/db/rows.go Simplifies updated-row merge to return a compacted slice and skip missing matches.
destination/db/clickhouse.go Implements lazy row conversion and updates insert/update batch code paths to avoid materializing boxed rows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread destination/db/clickhouse.go Outdated
Comment thread destination/db/clickhouse.go Outdated
Comment thread destination/db/clickhouse.go Outdated
Comment thread destination/db/clickhouse.go
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
destination/db/rows.go 71.42% 2 Missing ⚠️
destination/db/clickhouse.go 94.44% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

koletzilla and others added 5 commits July 7, 2026 18:30
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@koletzilla
koletzilla requested a review from BentsiLeviav July 8, 2026 13:27
@BentsiLeviav
BentsiLeviav merged commit 0fc452e into main Jul 19, 2026
5 checks passed
@BentsiLeviav
BentsiLeviav deleted the clickhouse/reduce-memory-used-in-insert-operations branch July 19, 2026 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants