fix(warehouse-sources): remove the old key when a cdc update changes it by danielcarletti · Pull Request #108577 · PostHog/posthog

danielcarletti · 2026-09-29T17:46:23Z

Problem

A Postgres CDC table keeps a stale row after a source UPDATE changes that row's primary key.
The old key stays live in the consolidated table and stays open in the _cdc history table.
Tables with REPLICA IDENTITY FULL also log a false cdc_pk_columns_diverged warning on every capture run.
Both bugs are on master. The devbox validation of feat(warehouse-sources): remove the legacy cdc lane #105912 found them; this PR is stacked on feat(warehouse-sources): remove the legacy cdc lane #105912.

Changes

Capture turns an update that changes the merge key into a delete of the old key and an insert of the new key, at the same position.
The delete carries only the old key, like a delete under the default replica identity. The existing delete enrichment fills the rest of the row.
The decoder reports the old values of the columns an update changed (ChangeEvent.previous_values). They come from the old key tuple (K) or the full old row (O).
The old key tuple sends non-key columns as NULL. Those NULLs no longer overwrite an unchanged TOAST column, which now stays marked as omitted.
The decoder compared the replica identity with 2, but pgoutput sends a character ('f' for FULL). The fix silences the false warning.
Capture splits only when the merge key covers a unique index that Postgres checks on every row: not deferrable, not partial, no expressions. It reads these from pg_index on each run.
Any other merge key can be held by two rows at once: a deferrable constraint allows it mid-transaction, and a user-chosen key may not be unique. There the delete would remove a live row, so those tables keep upserting on the new key, as on master.
The decoder keeps old values only for those split keys. Under REPLICA IDENTITY FULL the old row carries every column, and keeping them all would grow every spilled transaction.
A key change now stages two rows (the delete and the insert), so it counts as two synced rows.
Mechanical: previous_values rides through the transaction spill file, and capture drops it before the batcher holds the event. Relation becomes a frozen dataclass.

Known limits:

A table whose merge key no enforced unique index covers still keeps a stale old key after a key change, as on master.
Under the default replica identity, a key change that leaves a TOASTed column unchanged cannot carry that column's value. Postgres sends neither the old nor the new value. The column loads as NULL, or as the value of an earlier row that held the new key, as it did on master.
A replica identity index that excludes the merge key sends no old key for that column, so the update stays a plain upsert, as before.

How did you test this code?

Decoder tests: old values per marker (K with the key changed or unchanged, O, none). They fail on the base branch.
Decoder tests: an old key tuple never fills an unchanged TOAST column; a spilled transaction keeps previous_values; a FULL relation decoded from the wire yields no key.
Capture test: a key change buffers D(old) then I(new) at one position, also for one column of a composite key. A non-key change, or a key change on a deferrable-key table, stays one U. It fails on the base branch.
TestGetEnforcedUniqueKeys runs the pg_index query on the test database. It keeps a plain primary key and the key columns of a covering index, and skips a deferrable primary key, a deferrable unique constraint, a partial index and an expression index.
The capture test also checks that a merge key with no enforced unique index, or one narrower than the index, stays an upsert, and that capture hands the split keys to the decoder.
Not run: this branch on the devbox, and the loader applying the split pair to Delta. The pair takes the same path as a real delete and insert.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Release status

No feature flag controls this change
This change is behind a feature flag and is not available to users
This change makes a previously flagged feature available to everyone

Automatic notifications

Publish to changelog?

Docs update

None. No doc describes key changes or the replica identity check.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

One of three PRs that fix master bugs found while validating feat(warehouse-sources): remove the legacy cdc lane #105912 on a devbox. Each is based on feat(warehouse-sources): remove the legacy cdc lane #105912's branch. The devbox ran against disposable test tables; nothing from it is in this PR.
Skills: /writing-tests, /writing-code-comments, /reviewing-with-coderabbit, /writing-pr-descriptions.
Duplicate search (cdc primary key, replica identity): no open PR fixes this.
CodeRabbit CLI, 5 findings:
- Fixed: annotate the new test signatures (two findings).
- Rejected locally, then fixed after the same finding came back in review: deferred key swaps. Resolving swaps per transaction needs unbounded key tracking for spilled transactions, so only keys an enforced unique index covers are split.
A fresh-eyes review replaced the first deferrable-constraint check with the enforced-unique-index check, and found the FULL-identity spill growth. Both are in the last commit.
- Rejected locally, then fixed in review: previous_values is now Mapping[str, object].
- Rejected: require a replica identity that covers the merge key. That mismatch already existed, and this PR leaves its behavior unchanged.

🤖 Generated with Claude Code

An UPDATE that changes a primary key merged only on the new key, so the old key's row stayed live in the consolidated table and open in the history table. The decoder now reports the old values from the old key tuple (or the old row under REPLICA IDENTITY FULL), and capture turns a key-changing update into a delete of the old key and an insert of the new one. The old key tuple sends non-key columns as NULL; those NULLs no longer fill unchanged TOAST columns. The decoder compared the relation's replica identity with 2, but pgoutput sends it as a character ('f' for FULL), so FULL tables reported every column as key and logged cdc_pk_columns_diverged on every run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…egacy-lane' into claude/cdc-pk-update-deletes-old-key

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-09-29T17:46:35Z

🤖 CI report

⚠️

Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️

Comment density — 5% of added code lines are comments (14 of 295)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File	Comment lines	Added lines
`products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/decoder.py`	7	27
`products/warehouse_sources/backend/temporal/data_imports/cdc/types.py`	3	7
`products/warehouse_sources/backend/temporal/data_imports/cdc/activities.py`	2	61
`products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/tests/test_decoder.py`	2	66

This check does not block merging. It updates on every push and clears when the share drops.

⚠️

Backend coverage — 98.0% of changed backend lines covered — 12 uncovered

🧪 Backend test coverage

Patch coverage — changed backend lines (products + core): ████████████████████ 98.0% (703 / 715)

File	Patch	Uncovered changed lines
`products/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.py`	16.7%	1028–1031, 1044
`products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/stream_reader.py`	25.0%	348–350
`products/warehouse_sources/backend/management/commands/repair_stalled_schema_schedules.py`	50.0%	160
`products/warehouse_sources/backend/temporal/data_imports/cdc/legacy_conversion.py`	97.6%	71, 73
`products/warehouse_sources/backend/temporal/data_imports/cdc/activities.py`	98.8%	1481

🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 404535705342689 -n patch-coverage), or the coverage-data block at the end of this comment.

Per-product line coverage (touched products)

Product	Coverage	Lines
`demo`	`███████████░░░░░░░░░` 52.8%	1,411 / 2,673
`batch_exports`	`████████████████░░░░` 81.2%	21,527 / 26,502
`cdp`	`██████████████████░░` 88.2%	4,545 / 5,155
`mcp_analytics`	`██████████████████░░` 89.2%	5,038 / 5,651
`product_tours`	`██████████████████░░` 89.3%	1,331 / 1,491
`dashboards`	`██████████████████░░` 89.5%	6,904 / 7,714
`data_warehouse`	`██████████████████░░` 90.0%	14,009 / 15,570
`notebooks`	`██████████████████░░` 90.2%	15,297 / 16,964
`signals`	`██████████████████░░` 90.2%	57,838 / 64,097
`cohorts`	`██████████████████░░` 90.4%	8,420 / 9,316
`streamlit_apps`	`██████████████████░░` 90.8%	2,684 / 2,956
`managed_warehouse`	`██████████████████░░` 91.0%	10,252 / 11,263
`tasks`	`██████████████████░░` 91.1%	75,525 / 82,874
`data_modeling`	`██████████████████░░` 91.4%	10,554 / 11,543
`exports`	`██████████████████░░` 91.6%	9,680 / 10,562
`engineering_analytics`	`██████████████████░░` 91.7%	11,032 / 12,030
`business_knowledge`	`██████████████████░░` 92.2%	7,684 / 8,330
`conversations`	`███████████████████░` 92.5%	28,734 / 31,062
`early_access_features`	`███████████████████░` 92.6%	1,332 / 1,439
`stamphog`	`███████████████████░` 92.8%	8,109 / 8,742
`canvas`	`███████████████████░` 92.8%	6,873 / 7,405
`approvals`	`███████████████████░` 93.0%	3,974 / 4,271
`mcp_registry`	`███████████████████░` 93.1%	1,670 / 1,794
`notifications`	`███████████████████░` 93.2%	1,145 / 1,229
`error_tracking`	`███████████████████░` 93.2%	16,359 / 17,547
`surveys`	`███████████████████░` 93.3%	6,571 / 7,040
`slack_app`	`███████████████████░` 93.4%	13,995 / 14,989
`autoresearch`	`███████████████████░` 93.6%	8,481 / 9,061
`context_layer`	`███████████████████░` 93.9%	3,415 / 3,638
`web_analytics`	`███████████████████░` 94.0%	21,815 / 23,218
`alerts`	`███████████████████░` 94.0%	8,569 / 9,114
`billing_alerts`	`███████████████████░` 94.1%	2,094 / 2,226
`mcp_store`	`███████████████████░` 94.4%	8,940 / 9,472
`ai_observability`	`███████████████████░` 94.5%	24,473 / 25,896
`workflows`	`███████████████████░` 94.6%	15,222 / 16,093
`wizard`	`███████████████████░` 94.7%	6,150 / 6,496
`reminders`	`███████████████████░` 94.8%	760 / 802
`review_hog`	`███████████████████░` 95.0%	11,507 / 12,119
`endpoints`	`███████████████████░` 95.1%	9,206 / 9,681
`annotations`	`███████████████████░` 95.1%	817 / 859
`customer_analytics`	`███████████████████░` 95.2%	25,636 / 26,938
`marketing_analytics`	`███████████████████░` 95.3%	19,566 / 20,528
`posthog_ai`	`███████████████████░` 95.3%	2,488 / 2,610
`experiments`	`███████████████████░` 95.4%	32,457 / 34,020
`actions`	`███████████████████░` 95.5%	756 / 792
`logs`	`███████████████████░` 95.5%	15,399 / 16,130
`data_catalog`	`███████████████████░` 95.5%	4,401 / 4,606
`tracing`	`███████████████████░` 95.6%	3,536 / 3,699
`replay_vision`	`███████████████████░` 95.6%	27,675 / 28,939
`growth`	`███████████████████░` 95.7%	11,228 / 11,734
`messaging`	`███████████████████░` 95.8%	3,798 / 3,963
`skills`	`███████████████████░` 95.8%	6,972 / 7,274
`product_analytics`	`███████████████████░` 96.0%	28,470 / 29,647
`access_control`	`███████████████████░` 96.3%	7,113 / 7,386
`revenue_analytics`	`███████████████████░` 96.4%	1,876 / 1,946
`user_interviews`	`███████████████████░` 96.5%	2,867 / 2,971
`feature_flags`	`███████████████████░` 96.5%	25,588 / 26,509
`warehouse_sources`	`███████████████████░` 97.3%	456,596 / 469,402
`data_quality`	`████████████████████` 97.6%	7,587 / 7,774
`metrics`	`████████████████████` 98.0%	4,084 / 4,166
`analytics_platform`	`████████████████████` 98.3%	2,784 / 2,833
`pulse`	`████████████████████` 98.5%	2,046 / 2,078
`live_debugger`	`████████████████████` 99.2%	626 / 631

Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.

coderabbitai · 2026-09-29T17:52:17Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

PostgreSQL CDC update decoding now records differing old values and preserves them through transaction spill and replay. CDC extraction uses those values to emit a delete for the old key followed by an insert when a primary-key value changes. Deferrable-key updates remain single events. Updates without a changed primary-key value also remain single events.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to d17c1

For most tables, primary-key changes now correctly remove the old row. A table that combines a regular primary key with an unrelated deferrable unique constraint still leaves the old row in place, as it did before this change. The PR can merge once the owner is aware of this gap, but tightening the constraint check would complete the fix.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to d17c1

A broad exception for tables with deferrable constraints can leave an old row visible after an ordinary key change. The effect is limited to affected replicated tables, but it undermines the cleanup this change is intended to provide.

Retained concerns

Medium · reliability · observed: Classifying deferrability by table suppresses old-key cleanup even for a non-swap key change. An unrelated deferrable unique constraint also qualifies the table. The old consolidated row can remain live and its history version open after the new key is upserted.

Security review details

Security Blast Radius

inferred — The demonstrated stale-row outcome is scoped to replicated tables for which the source reports a deferrable constraint; it can affect their consolidated and history representations. The inspected path does not establish a new cross-tenant sink or privilege.

Security Findings and Attack Paths

inferred — A party able to change a replicated source row's merge key can trigger the stale-row outcome on an exempt table. This is a data-retention and correctness path, not evidence of unauthorized source access or a cross-tenant attack.

Trust Boundaries and Controls

observed — The activity loads the source by source ID and uses its adapter and configured schemas; the inspected setup does not itself verify that the input team ID owns that source. Caller authorization was not established, but no change to that lookup or new caller was identified here.

Resilience and Maintainability Implications

observed — Extraction buffers both generated events before considering a flush and confirms only a fully yielded transaction during a micro-flush. The inspected downstream replay handles equal-position rows, while atomic visibility across the consolidated and history output lanes remains unproven.

Hardening Proposals

proposed — Constrain swap handling to the relevant merge-key transition, rather than exempting every update on a table with any deferrable constraint; account for already-buffered upserts if the exemption has been active.

🚥 Pre-merge checks | ✅ 1

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The pull request description is complete and follows the repository template. It explains the problem, user-visible changes, known limits, automated tests, release status, documentation impact, and ag…

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 54ba9e4a-d49b-434f-97d8-3b27771bdb9e

📥 Commits

Reviewing files that changed from the base of the PR and between 80afa8b and 277fead.

📒 Files selected for processing (5)

products/warehouse_sources/backend/temporal/data_imports/cdc/activities.py
products/warehouse_sources/backend/temporal/data_imports/cdc/tests/test_extract_activity.py
products/warehouse_sources/backend/temporal/data_imports/cdc/types.py
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/decoder.py
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/tests/test_decoder.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

greptile-apps · 2026-09-29T17:52:35Z

[Medium risk] Changes how CDC updates with key changes are handled.

The PR is not safe to merge until it preserves both rows during a same-transaction key swap and satisfies the repository requirements.

_{Reviews (1) · Last reviewed commit: "chore(warehouse-sources): annotate the k..."}

Two rows can swap keys in one transaction only when the key constraint is DEFERRABLE. Splitting each update into a delete of the old key and an insert of the new one would then drop a row: the second row's delete removes the key the first row's insert took. Capture now reads which tables have a deferrable primary key or unique constraint and leaves their key updates as upserts on the new key, as before. Also: `Relation` becomes a frozen dataclass, `previous_values` is typed without `Any`, and a test helper docstring line is dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

products/warehouse_sources/backend/temporal/data_imports/cdc/tests/test_extract_activity.py (1)

1509-1510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Annotate the new reader-stub signature.

Add types for schema, tables, and the return value. Match the CDCStreamReader.get_deferrable_key_tables contract. As per coding guidelines, “Write as if mypy --strict were on” and “Annotate every signature.”

Source: Coding guidelines

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: f3989f5b-44c2-441e-bbcb-7f62cb508485

📥 Commits

Reviewing files that changed from the base of the PR and between 277fead and d17c1ce.

📒 Files selected for processing (7)

products/warehouse_sources/backend/temporal/data_imports/cdc/activities.py
products/warehouse_sources/backend/temporal/data_imports/cdc/tests/test_extract_activity.py
products/warehouse_sources/backend/temporal/data_imports/cdc/types.py
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/decoder.py
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/stream_reader.py
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/tests/test_decoder.py
products/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.py

💤 Files with no reviewable changes (1)

products/warehouse_sources/backend/temporal/data_imports/sources/postgres/cdc/tests/test_decoder.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.

trunk-io · 2026-09-29T18:36:25Z

_{View Full Report ↗︎ ⋅ Docs}

The deferrable-constraint check asked the wrong question. It kept every table with any deferrable constraint as upserts, even when the merge key itself was a plain primary key. It also split tables whose merge key nothing enforces, such as a key the user chose, where two rows can hold one key and the delete would remove a row that still exists. Capture now reads each table's unique indexes that Postgres checks on every row (not deferrable, not partial, no expressions) and splits only when one of them is covered by the merge key. It hands those keys to the decoder, which keeps old values only for them. Under REPLICA IDENTITY FULL the old row carries every column, and keeping the rest grew every spilled transaction for values nothing read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

danielcarletti and others added 3 commits September 29, 2026 14:36

Merge remote-tracking branch 'refs/remotes/origin/claude/cdc-remove-l…

4004316

…egacy-lane' into claude/cdc-pk-update-deletes-old-key

chore(warehouse-sources): annotate the key-change test signatures

277fead

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

danielcarletti self-assigned this Sep 29, 2026

coderabbitai Bot reviewed Sep 29, 2026

View reviewed changes

Comment thread products/warehouse_sources/backend/temporal/data_imports/cdc/activities.py

Comment thread products/warehouse_sources/backend/temporal/data_imports/cdc/types.py Outdated

greptile-apps Bot reviewed Sep 29, 2026

View reviewed changes

danielcarletti marked this pull request as ready for review September 29, 2026 18:07

pr-assigner-resolver-posthog Bot requested a review from a team September 29, 2026 18:08

coderabbitai Bot reviewed Sep 29, 2026

View reviewed changes

Comment thread products/warehouse_sources/backend/temporal/data_imports/sources/postgres/postgres.py Outdated

danielcarletti and others added 2 commits September 29, 2026 15:43

chore(warehouse-sources): annotate the capture reader stub

65c00bf

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Navigation Menu

fix(warehouse-sources): remove the old key when a cdc update changes it - #108577

fix(warehouse-sources): remove the old key when a cdc update changes it#108577
danielcarletti wants to merge 6 commits into
claude/cdc-remove-legacy-lanefrom
claude/cdc-pk-update-deletes-old-key

danielcarletti commented Sep 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 29, 2026 •

edited

Loading

🧪 Backend test coverage

Uh oh!

coderabbitai Bot commented Sep 29, 2026 •

edited

Loading

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot commented Sep 29, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

trunk-io Bot commented Sep 29, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

danielcarletti commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

How did you test this code?

Release status

Automatic notifications

Docs update

🤖 Agent context

Uh oh!

github-actions Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 CI report

🧪 Backend test coverage

Uh oh!

coderabbitai Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Security Architecture Review

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

trunk-io Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

danielcarletti commented Sep 29, 2026 •

edited

Loading

github-actions Bot commented Sep 29, 2026 •

edited

Loading

coderabbitai Bot commented Sep 29, 2026 •

edited

Loading

greptile-apps Bot commented Sep 29, 2026 •

edited

Loading

trunk-io Bot commented Sep 29, 2026 •

edited

Loading