Skip to content

postgres_cdc: add signalling support (without the need to restart)#4624

Closed
josephwoodward wants to merge 2 commits into
mainfrom
jw/postgres_cdc_no_reconnect_snapshot
Closed

postgres_cdc: add signalling support (without the need to restart)#4624
josephwoodward wants to merge 2 commits into
mainfrom
jw/postgres_cdc_no_reconnect_snapshot

Conversation

@josephwoodward

@josephwoodward josephwoodward commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Adds support for signalling a re-snapshot, but fundamentally changes the way the PostgreSQL connector works as the snapshot/streaming modes are quite tightly coupled.

Comment thread internal/impl/postgresql/input_pg_stream.go
Comment thread internal/impl/postgresql/pglogicalstream/logical_stream.go Outdated
@josephwoodward
josephwoodward force-pushed the jw/postgres_cdc_no_reconnect_snapshot branch 2 times, most recently from 2d12580 to 8b8e64e Compare July 23, 2026 17:35
Comment thread internal/impl/postgresql/input_pg_stream.go
postgres_cdc: update tests to verify singal event

postgres_cdc: wait for messages to be acked before restarting
@josephwoodward
josephwoodward force-pushed the jw/postgres_cdc_no_reconnect_snapshot branch from 8b8e64e to 7f6fb5c Compare July 24, 2026 13:26
}

if sig != nil {
batchMsg.MetaSet("operation", string(pglogicalstream.ControlSignalOpType))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Signal rows are emitted with operation=control_signal, contradicting the documented contract and this PR's own integration tests.

This line overwrites the message's operation metadata with control_signal, and the signal row is then flushed downstream via handleControlSignalflushBatch/msgChan. But the field documentation states signal rows are published as regular messages with operation=insert:

And every actionable-signal assertion in the new integration test expects operation: "insert" for the rpcn_signal_table row, e.g.

require.ElementsMatch(t, received, []any{
map[string]any{"operation": "insert", "table": "rpcn_signal_table", "lsn": "XXX/XXX"},
map[string]any{"operation": "read", "table": "events"},
map[string]any{"operation": "read", "table": "events"},
})
mu.Unlock()
db.MustExec(`INSERT INTO dbo.rpcn_signal_table (type, data) VALUES ('execute-snapshot', '{"data-collections": ["dbo.events"]}')`)
// Wait for the second re-snapshot: another signal row plus two snapshot reads,
// accumulated on top of the previous three.
assert.EventuallyWithT(t, func(c *assert.CollectT) {
mu.Lock()
defer mu.Unlock()
assert.Len(c, received, 6)
}, 25*time.Second, 100*time.Millisecond)
mu.Lock()
require.ElementsMatch(t, received, []any{
map[string]any{"operation": "insert", "table": "rpcn_signal_table", "lsn": "XXX/XXX"},
map[string]any{"operation": "read", "table": "events"},
map[string]any{"operation": "read", "table": "events"},
map[string]any{"operation": "insert", "table": "rpcn_signal_table", "lsn": "XXX/XXX"},
map[string]any{"operation": "read", "table": "events"},
. As written those rows arrive as control_signal, so the documented contract is broken and the enabled-signalling integration tests would not match their expected sets.

Since handleControlSignal is dispatched on sig != nil (not on this metadata), the control_signal operation serves no internal purpose and only leaks to the output. Consider dropping this override so signal rows forward as ordinary inserts, consistent with the docs and tests.

Comment on lines 24 to +32
output:
processors:
- benchmark:
interval: 1s
count_bytes: true
# file:
# path: "./benchmark_results.json"
# codec: lines
drop: {}
# processors:
# - benchmark:
# interval: 1s
# count_bytes: true
file:
path: "./benchmark_results.json"
codec: lines
# drop: {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unrelated benchmark output change looks like a committed local debugging tweak. Alongside the intended signal_table_name addition, this file also switches the benchmark output from drop: {} + the benchmark processor (which measures throughput) to writing rows into ./benchmark_results.json, and drops the count: 1000 batching bound (diff). These changes are unrelated to the signalling feature and disable the throughput measurement the bench harness exists for (§1.3.4). This looks like a local iteration left in by accident — worth confirming it's intended before merging.

@josephwoodward

Copy link
Copy Markdown
Contributor Author

This was an experimental branch to understand how a "no restart" version would look and it carries too much complexity versus restarting the connector on a signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant