fix(connectors): fix postgres source CDC producing no messages#3640
fix(connectors): fix postgres source CDC producing no messages#3640mattp5657 wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3640 +/- ##
=============================================
- Coverage 73.53% 42.58% -30.95%
Complexity 937 937
=============================================
Files 1283 1281 -2
Lines 138809 121901 -16908
Branches 114699 97791 -16908
=============================================
- Hits 102077 51917 -50160
- Misses 33563 67257 +33694
+ Partials 3169 2727 -442
🚀 New features to boost your workflow:
|
|
i think this is somewhat related to #3313 |
|
@mmodzelewski could you please check this one? |
mmodzelewski
left a comment
There was a problem hiding this comment.
Thanks, @mattp5657. The change looks solid, I've posted a few comments where the code could be improved.
| break; | ||
| }; | ||
| let name_end = pos + bracket_offset; | ||
| let column_name = &data[pos..name_end]; |
There was a problem hiding this comment.
Quoted column names keep their literal quotes as JSON keys. test_decoding applies quote_identifier to column names, so a mixed-case or keyword column (createdAt, user) arrives as "createdAt"[timestamp with time zone]:'...' and this parser emits the JSON key \"createdAt\" with the double-quote characters included. Table names already go through unquote_pg_identifier; column names should get the same treatment, otherwise downstream consumers looking up the plain column name miss. Worth a unit test with a quoted column name fixture.
| result | ||
| } | ||
|
|
||
| fn parse_column_value(data: &str, start: usize) -> (serde_json::Value, usize) { |
There was a problem hiding this comment.
The unchanged-toast-datum sentinel leaks into records as a real value. When an UPDATE does not touch a TOASTed column (large text/jsonb), test_decoding emits the bare token unchanged-toast-datum, and the parser stores data["col"] = "unchanged-toast-datum", which is indistinguishable from a legitimate string. Consumers will silently ingest the sentinel as the column value. Suggest handling it explicitly in parse_bare_scalar (map to null, or omit the key) and adding a fixture for it.
| let http = Client::new(); | ||
| wait_for_source_status(&http, &api_url, ConnectorStatus::Running).await; | ||
|
|
||
| sqlx::query("SELECT pg_drop_replication_slot($1)") |
There was a problem hiding this comment.
Flake risk: this first pg_drop_replication_slot races the still-running connector, which calls pg_logical_slot_get_changes every 50ms and briefly holds the slot on each call. If the drop lands inside that window, Postgres raises ERROR 55006: replication slot "iggy_slot" is active for PID ... and the expect panics. Rough odds are a few percent per run, which is enough to flake CI. Wrapping the drop in a short retry loop fixes it. The second drop below is fine since the failed restart leaves no poller running.
| enable_wal_cdc = false | ||
| publication_name = "iggy_publication" | ||
| # CDC options (only used when mode = "cdc") | ||
| replication_slot = "iggy_slot" |
There was a problem hiding this comment.
Two operational hazards of the new slot handling deserve README coverage:
- Slot sharing:
setup_cdcaccepts any pre-existingtest_decodingslot, so two CDC connectors against the same database with the defaultreplication_slot = "iggy_slot"silently share one slot.pg_logical_slot_get_changesconsumes on read, so each connector steals a subset of the other's changes. The README should state that every CDC connector needs a unique slot name. - WAL retention: a slot orphaned after a connector is decommissioned retains WAL indefinitely (default
max_slot_wal_keep_size = -1) until the disk fills. The README should document dropping the slot (SELECT pg_drop_replication_slot('iggy_slot')) as part of decommissioning.
| @@ -303,10 +301,6 @@ impl PostgresSource { | |||
| } | |||
|
|
|||
| async fn setup_cdc(&self) -> Result<(), Error> { | |||
There was a problem hiding this comment.
Not on this line, but related to CDC startup: open() validates mode but not cdc_backend. A typo like cdc_backend = "built-in" passes startup, then every poll_cdc call returns an error that the SDK loop logs and swallows, leaving a Running connector that produces nothing. That is the same silent-death failure mode this PR fixes for the slot mismatch. Validating the backend in the "cdc" arm of open(), where the slot mismatch already fails loudly, would close it.
| }); | ||
| } | ||
| None | ||
| let rest = rest |
There was a problem hiding this comment.
The old-key columns are parsed past and discarded here, so DatabaseRecord.old_data is always None even though the struct has the field and the data is sitting in the row. For a PK-changing UPDATE (and for DELETE under REPLICA IDENTITY FULL) consumers only see the new tuple and cannot learn which row it replaced, which breaks e.g. keyed upsert/delete downstream. Issue #3582 lists unpopulated old_data as well. Suggest running parse_record_columns over the old-key section and populating old_data instead of dropping it.
Which issue does this PR address?
Relates to #3582
Rationale
Builtin CDC mode currently produces zero messages against a real
database - wrong plugin/read-path pairing, and a parser matching a
format no real Postgres output plugin emits. This PR is scoped to
getting CDC actually working and provably correct, nothing more.
I have reduce the scope of this specific PR for reviewability, in the future
would look to create issues and work on these issues:
pgoutputas the long-term plugin - real binary decoder + streamingconnection is a much bigger lift;
test_decodingunblocks correctnesstoday. Target once
cdc_backend = "pg_replicate"gets picked up.no LSN tracked) - needs its own review since it touches the
state-persistence contract. Follow-up: #TODO.
commit time,
old_dataunpopulated) - mechanical, separate PR.What changed?
Before: wrong slot plugin, a parser matching a format no real Postgres
output emits, and invalid setup SQL - CDC produced zero messages, and
several related bugs failed silently too. After: parser rewritten
against real captured output, setup/poll SQL fixed and bounded, and
those failure modes now work correctly or fail loudly. Covered by
corpus-driven unit tests plus integration tests against a live
wal_level=logicalcontainer.test_decodingoutputLocal Execution
AI Usage
Claude was used to help generate and review this PR.