Skip to content

fix(wal): line-protocol WAL entries carry their database envelope (#889) - #1055

Merged
xe-nvdk merged 2 commits into
Basekick-Labs:mainfrom
lecodev-26:fix/replication-wal-source-integrity
Oct 5, 2026
Merged

xe-nvdk merged 2 commits into
Basekick-Labs:mainfrom
lecodev-26:fix/replication-wal-source-integrity

Conversation

@lecodev-26

@lecodev-26 lecodev-26 commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #889.

Line-protocol writes reach the buffer as columns with no raw client bytes, so their WAL entry took the row-format fallback and went out bare: the replication stream carried no database, and a receiver parsing it with ParseEnvelope(payload, "default") filed every row under default. The row-format path now writes the same [0x01][dbLen][db][msgpack] envelope the msgpack path has always used.

Replay-safe (the reader parses the envelope and tries the row array first; recovery routes by the _database stamped on each record), receiver-safe, checkpoint-safe (same tracked identities), oversize-safe (arrays split by element), readable by every binary from 26.05.1.

Scope note: the original branch also started a replication receiver on readers/compactors without a local WAL (#886) and gated the file registrar by role (#888). Both were dropped in review; see the review comment. #886 and #888 stay open for the local-storage replication design pass.

Test plan

  • TestLineProtocolWALEntriesCarryTheirDatabase fails on main, passes here (20× alone under -race)
  • go test -tags=duckdb_arrow -race ./internal/ingest/ ./internal/wal/ ./internal/cluster/
  • gofmt -l, go vet -tags=duckdb_arrow, go build -tags=duckdb_arrow ./cmd/... ./internal/...
  • Release note added to RELEASE_NOTES_2026.09.3.md

@lecodev-26
lecodev-26 force-pushed the fix/replication-wal-source-integrity branch 2 times, most recently from d2dc072 to 76d8d90 Compare October 4, 2026 17:46
lecodev-26 and others added 2 commits October 4, 2026 13:13
… and registrar gates; add the regression test and note

Review fold-in on top of the contributor's change (Basekick-Labs#1055):

- Drop the cmd/arc/main.go hunk (Basekick-Labs#886). Starting a receiver on readers and
  compactors is the switch that turns Basekick-Labs#888 live: a replica applies replicated
  rows to its own ArrowBuffer, flushes them to its own storage under a unique
  name, and its queries read that file alongside the copy pulled from the
  primary (read_parquet globs storage; the query handler never reads the
  buffer). On a compactor the unannounced copy is merged into compacted output
  that is announced through the manifest bridge. Basekick-Labs#886 stays closed until the
  Basekick-Labs#888 hand-off lands.
- Drop the internal/cluster/file_registrar.go role gate. It removes the
  announcement, not the file, so it does not address Basekick-Labs#888, and it would
  silently stop announcing files the /api/v1/import/* endpoints write on a
  reader or compactor (those endpoints have no write forwarding and no role
  check, so today their flushes are announced and pulled). That seam needs
  the design pass with Basekick-Labs#886/Basekick-Labs#888.
- Keep the row-format WAL envelope (Basekick-Labs#889). Replay-safe (the reader parses the
  envelope and tries the row array first; recovery routes by the _database
  stamped on each record), receiver-safe, checkpoint-safe (same tracked
  tokens), oversize-safe (array split), and readable by every binary from
  26.05.1.
- Test: TestLineProtocolWALEntriesCarryTheirDatabase captures the
  replication-hook payload for a WriteColumnarDirect write with no raw bytes,
  checks that ParseEnvelope(payload, "default") resolves to the real database
  and the inner payload is a row array, and that the on-disk entry still
  replays as a tracked row entry. Fails on main.
- Release note in RELEASE_NOTES_2026.09.3.md under Bug fixes, saying why
  Basekick-Labs#886 and Basekick-Labs#888 are not in this release and what a reader that already
  receives will observe.
@xe-nvdk
xe-nvdk force-pushed the fix/replication-wal-source-integrity branch from 76d8d90 to 123a6d2 Compare October 5, 2026 03:08
@xe-nvdk xe-nvdk changed the title fix(cluster): harden WAL replication source handling fix(wal): line-protocol WAL entries carry their database envelope (#889) Oct 5, 2026
@xe-nvdk

xe-nvdk commented Oct 5, 2026

Copy link
Copy Markdown
Member

Thanks for taking these on, @lecodev-26. I reviewed the three changes against the configurations they reach and landed the one that is sound on its own. Here is what changed and why.

Kept: the #889 envelope on the row-format WAL path. Correct and exactly the right mechanism. Replay-safe (reader.go parses the envelope and then tries the row array first; recovery routes by the _database stamped on each record), receiver-safe (the map decode fails and the row fallback runs), checkpoint-safe (same tracked tokens), oversize-safe (arrays split by element), and readable by every binary from 26.05.1. I added TestLineProtocolWALEntriesCarryTheirDatabase, which captures the replication-hook payload the sender would stream and checks that ParseEnvelope(payload, "default") resolves to the real database. It fails on main.

Dropped: the cmd/arc/main.go gate split (#886). This is the reason the PR cannot say "Fixes #888":

  • A replica's query freshness is literally its own flushed Parquet. NewQueryHandler takes DuckDB plus storage and never reads the in-memory buffer; FROM db.m becomes read_parquet('<base>/db/m/**/*.parquet') (query.go:3137); the puller writes the primary's copy into the same tree (puller.go:1658). A reader that starts receiving ends up with both files in its own storage and reads both, permanently (readers never compact, and a file that was never in the manifest is never deleted by a manifest delete). That is the 3-vs-6 measured on the rig in cluster: replicas register their replicated rows in the manifest — duplicate rows on query #888.
  • On a compactor it spreads. Compaction lists its inputs from storage, not from the manifest (compaction/manager.go:631, tier.go:322), so the compactor's unannounced duplicate is merged into compacted output, which is announced through the manifest bridge and pulled by every node. Dedup only collapses it when the batch carries arc:tags.

Dropped: the registrar role gate. It removes the announcement, not the file, so it does not change what the replica reads. And it has a victim: the /api/v1/import/* endpoints have no write forwarding and no role check (import.go), so an import run against a reader writes into that reader's buffer, and today its flush is announced and pulled by the rest of the cluster. With the gate, that file would stay local with no counter and no log. The right seam is to mark replication-applied batches so their flush skips registration, which belongs with the #886/#888 design pass.

So #886 and #888 stay open and #889 closes. One consequence to be upfront about: on a cluster whose readers already receive (a local WAL enabled on the reader), line-protocol rows now land in the right database, and the reader still flushes and announces its own copy, so until #888 lands they count twice on every node, as native msgpack rows already do. The 26.09.3 release note says so. I retitled the PR to match what ships. Appreciate the clean diff; the envelope fix was exactly right. @drakeo338 proposed this same source-side envelope in #889 on 2026-09-23 and asked how to sequence it; the note credits both of you, and sorry for the silence on that question.

@xe-nvdk xe-nvdk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed against the configuration matrix (standalone, Pattern 1 writer/reader/compactor, Pattern 2); the #889 envelope is replay-, receiver-, checkpoint- and oversize-safe, with a regression test that fails on main. The #886 and #888 hunks were dropped in review (see the comment above). CI green on 123a6d2.

@xe-nvdk
xe-nvdk merged commit 52d2a8c into Basekick-Labs:main Oct 5, 2026
3 checks passed
xe-nvdk added a commit that referenced this pull request Oct 5, 2026
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.

cluster: line-protocol writes replicate into the default database, not the one they were written to

2 participants