Feature/postgres cdc multi schema#4589
Conversation
…t harness Two-schema (tenant_a, tenant_b) Postgres 16 setup that exercises the multi-schema CDC pipeline end-to-end. Also adds schema_validation unit tests that verify invalid patterns are rejected at startup without a DB.
squiidz
left a comment
There was a problem hiding this comment.
Re-reviewed after the rebase — the critical issue from the earlier pass is resolved: stream_message.go and replication_message_decoders.go are no longer touched, so the duplicate CommitTs/Before/tuple-decoding reimplementation (which had drifted from what main already shipped in #4554/#4555) is gone. The diff is now correctly scoped to the new schema-glob + pg_schema + publication-batching work layered on top of main's existing commit_ts_ms/before support. 👍
A few remaining items from the original pass, still present:
-
gofmt: the struct literal in
input_pg_stream.goisn't gofmt-clean —DBSchemaPattern: schema, DBTables: tables,
gofmt -wwantsDBSchemaPattern: schema,/DBTables: tables,to stay column-aligned withRefreshAuthToken:in the same literal. This will fail agofmt -l/CI format check as-is. -
validateSchemaPattern's quoted-identifier check (input_pg_stream.go) reimplements a shallow prefix/suffix/length check instead of delegating tosanitize.UnquotePostgresIdentifier, which this PR already uses inschema_resolver.go. The existing helper correctly handles escaped double-quotes ("foo""bar"); the new check doesn't. -
No
CHANGELOG.mdentry — recentpostgres_cdcPRs (#4554, #4555, ...) all added one under "Unreleased". -
tests/current/(Taskfile/docker-compose/setup.sql/test_config.yaml) reads like a personal manual-test harness — worth confirming this is meant to be committed long-term vs. a scratch dir that should be dropped or renamed to something more descriptive. -
Worth a doc callout: schema-pattern resolution happens once at stream construction (
resolveSchemasruns at startup only). For the stated multi-tenant use case, a newtenant_cschema created after the pipeline starts won't be picked up until restart. Not a bug, but non-obvious given the motivating use case. -
Test coverage gap: no test for the "zero schemas matched" startup-error path (
no schemas found matching pattern %q).
None of these block correctness — (1) is the only one that will mechanically fail CI as-is.
removing the need for one slot per tenant.
state leaking into subsequent transactions.