Skip to content

jw/pg multi schema - #4692

Draft
josephwoodward wants to merge 53 commits into
mainfrom
jw/pg_multi_schema
Draft

jw/pg multi schema#4692
josephwoodward wants to merge 53 commits into
mainfrom
jw/pg_multi_schema

Conversation

@josephwoodward

Copy link
Copy Markdown
Contributor
  • postgres_cdc: add multi-schema support
  • postgres_cdc: reject empty quoted schema identifier and fix misleading godoc
  • postgres_cdc: add tests/current/ Docker Compose + Taskfile manual test harness
  • ** postgres_cdc: add commit_ts_ms and before metadata fields**
  • postgres_cdc: fix lint and docs
  • postgres_cdc: review fixes and test coverage
  • postgres_cdc: fix tests
  • test(cdctest): waive tigerbeetle_cdc conformance fields
  • postgres_cdc: fix lint
  • postgres_cdc: skip missing tables per-schema instead of failing whole publication
  • postgres_cdc: fix lint
  • Update internal/impl/postgresql/pglogicalstream/schema_resolver.go
  • postgres_cdc: warn when schema pattern matches privilege-hidden schemas
  • postgres_cdc: Address minor issues
  • postgres_cdc: fix broken test
  • postgres_cdc: replace pg_schema with database_schema
  • postgres_cdc: move schema validation to unit test closer to use
  • postgres_cdc: clean up redundant comment
  • postgres_cdc: normalie test structure
  • postgres_cdc: t.Context()
  • postgres_cdc: revert to existing behaviour
  • postgres_cdc: improve on missing table coverage
  • update docs
  • postgres_cdc: add schema_pattern field for multi-schema replication
  • postgres_cdc: fix unicode schema_pattern rejection, sync changelog
  • postgres_cdc: fix stale schema_pattern refs in manual test harness
  • postgres_cdc: add excluded_schema
  • postgres_cdc: fix failing signalling tests
  • postgres_cdc: add further test coverage for uuid schemas

ness-david-dedu and others added 30 commits July 6, 2026 22:00
…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.
Co-authored-by: Joseph Woodward <joseph.woodward@xeuse.com>
  information_schema.schemata only lists schemas the connecting role can
  see, so a schema hidden by missing USAGE was silently dropped from a
  matched pattern with no signal to the user. resolveSchemas now cross-checks
  pg_catalog.pg_namespace, which isn't privilege-filtered, and reports those
  as inaccessible so logical_stream.go can warn instead of skipping silently.
If no schema is found and tables is empty then we want to ensure we
create publication for all tables.
Introduces schema_pattern as a new optional field for glob-based
multi-schema CDC, instead of overloading schema with pattern semantics.
This keeps schema backwards compatible: it reverts to exact-name-only
behavior (now defaulting to "public"), while schema_pattern opts into
the glob resolution path. Setting both is a config error.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
validateSchemaPattern only accepted [a-zA-Z0-9_*], stricter than
sanitize.NormalizePostgresIdentifier (unicode.IsLetter/IsDigit) which
governs the exact-name schema path. Since schema_pattern is now the
sole caller of validateSchemaPattern, an unquoted pattern with
non-ASCII letters (e.g. münchen) was rejected at startup even though
the equivalent schema value has always been accepted. Widened the
character classes to match, using utf8.DecodeRuneInString for the
first-character check instead of a raw byte cast.

Also corrected the Unreleased changelog entry, which still described
the old design where schema itself accepted glob patterns.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
internal/impl/postgresql/tests/current/ is a docker-compose-backed
Taskfile harness for manually exercising multi-schema CDC against a
real Postgres (tenant_a/tenant_b). It predates the schema/schema_pattern
split and still used schema: tenant_* directly, which the split turns
into a literal (and invalid) exact schema name instead of a glob.

- test_config.yaml: schema -> schema_pattern for the tenant_* glob.
- Taskfile.yaml test:invalid-schema: schema="" is now a no-op (unset
  schema_pattern falls back to schema's "public" default), so it no
  longer exercises a failure path. Repointed at schema_pattern=1abc,
  which still fails fast in newPgStreamInput before any DB connection,
  matching the task's original intent.

Verified by bringing the compose stack up and running the smoke test;
config loads and reaches runtime init (blocked only by the license
check in this sandboxed environment, which is unrelated).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Comment on lines +41 to +42
fieldSchemaInclude = "schema_include"
fieldSchemaExclude = "schema_exclude"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CDC config naming: schema_include/schema_exclude are bespoke names where the fleet already has canonical include/exclude

CONTRIBUTING §5.3.1 lists the canonical CDC filtering fields as "tables (or include/exclude)", and §5.3.3 says "Do not invent bespoke names where a canonical one already exists… New connectors must use the canonical names from the start." §5.5.7 names oracledb_cdc as the reference for include/exclude matching.

The reference implementation uses plain include/exclude holding regexes over schema-qualified names, which already covers schema selection:

).
Field(service.NewStringField(ociFieldCheckpointCache).
Description("A https://www.docs.redpanda.com/redpanda-connect/components/caches/about[cache resource^] to use for storing the current System Change Number (SCN) that has been successfully delivered, this allows Redpanda Connect to continue from that System Change Number (SCN) upon restart, rather than consume the entire state of OracleDB's redo logs. If not set the default Oracle based cache will be used, see `" + ociFieldCheckpointCacheTableName + "` for more information.").
Optional(),
).
Field(service.NewStringField(ociFieldCheckpointCacheTableName).
Description("The identifier for the checkpoint cache table name. If no `" + ociFieldCheckpointCache + "` field is specified, this input will automatically create a table and stored procedure under the `rpcn` schema to act as a checkpoint cache. This table stores the latest processed System Change Number (SCN) that has been successfully delivered, allowing Redpanda Connect to resume from that point upon restart rather than reconsume the entire redo log. When `" + ociFieldPDBName + "` is set and this field is left at its default value, the table name is automatically derived per PDB (e.g. `RPCN.CDC_CHECKPOINT_MYPDB`) to avoid SCN collisions between pipelines monitoring different PDBs. Set this field explicitly to opt out of that auto-derivation.").
Default(defaultCheckpointCache).

ociFieldTablesExclude = "exclude"
ociFieldTablesInclude = "include"
// Description("Regular expressions for tables to include.").Example("SCHEMA.PRODUCTS")

This PR instead introduces schema_include (a single string) and schema_exclude (a list) with glob semantics rather than regex, so postgres_cdc ends up with a third filtering vocabulary alongside tables and oracle's include/exclude. That is exactly the divergence §5.3.3 and §3.1.5 ("codebase feels consistent with other Redpanda Connect connectors") are meant to prevent, and it will be a breaking rename later when postgres_cdc converges.

Suggested fix: express schema filtering through the canonical include/exclude regex fields matched against schema.table, as oracledb_cdc does. If glob-over-schemas is deliberately preferred here, please get the deviation agreed and record it in the conformance gate at internal/plugins/cdctest so the fleet decision is explicit rather than implicit.

Comment thread internal/impl/postgresql/pglogicalstream/logical_stream.go Outdated
Comment on lines +41 to +42
fieldSchemaInclude = "schema_include"
fieldSchemaExclude = "schema_exclude"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bespoke include/exclude naming and matching syntax diverges from the CDC fleet (CONTRIBUTING §5.3.3, §5.5.7, §3.1.5)

CONTRIBUTING.md §5.3.1 names include/exclude as the canonical selection fields, and §5.3.3 says "Do not invent bespoke names where a canonical one already exists". §5.5.7 specifies "Include/exclude regex table matching. Reference: oracledb_cdc."

The fleet already implements this axis as top-level include/exclude string lists of regular expressions matched against SCHEMA.TABLE:

  • ociFieldSnapshotMaxBatchSize = "snapshot_max_batch_size"
    ociFieldTablesExclude = "exclude"
    ociFieldTablesInclude = "include"
    ociFieldCheckpointLimit = "checkpoint_limit"
    ociFieldCheckpointCache = "checkpoint_cache"
    ociFieldCheckpointCacheKey = "checkpoint_cache_key"
    ociFieldCheckpointCacheTableName = "checkpoint_cache_table_name"
    ociFieldBatching = "batching"
    ociFieldPDBName = "pdb_name"
    (exclude / include)
  • Optional(),
    ).
    Field(service.NewStringField(ociFieldCheckpointCache).
    Description("A https://www.docs.redpanda.com/redpanda-connect/components/caches/about[cache resource^] to use for storing the current System Change Number (SCN) that has been successfully delivered, this allows Redpanda Connect to continue from that System Change Number (SCN) upon restart, rather than consume the entire state of OracleDB's redo logs. If not set the default Oracle based cache will be used, see `" + ociFieldCheckpointCacheTableName + "` for more information.").
    Optional(),
    ).
    Field(service.NewStringField(ociFieldCheckpointCacheTableName).
    Description("The identifier for the checkpoint cache table name. If no `" + ociFieldCheckpointCache + "` field is specified, this input will automatically create a table and stored procedure under the `rpcn` schema to act as a checkpoint cache. This table stores the latest processed System Change Number (SCN) that has been successfully delivered, allowing Redpanda Connect to resume from that point upon restart rather than reconsume the entire redo log. When `" + ociFieldPDBName + "` is set and this field is left at its default value, the table name is automatically derived per PDB (e.g. `RPCN.CDC_CHECKPOINT_MYPDB`) to avoid SCN collisions between pipelines monitoring different PDBs. Set this field explicitly to opt out of that auto-derivation.").
    Default(defaultCheckpointCache).
    Example("RPCN.CHECKPOINT_CACHE").
    (Regular expressions for tables to include. / ... exclude., e.g. SCHEMA.PRODUCTS)
  • internal/impl/mssqlserver/input_mssqlserver_cdc.go uses the same include/exclude names.

This PR instead adds schema_include (single string) and schema_exclude (string list) with a glob (*) syntax that is neither the canonical name nor the fleet's regex semantics, so a user configuring multi-schema capture writes something different in postgres_cdc than in oracledb_cdc/microsoft_sql_server_cdc. §3.1.5 asks that "the codebase feels consistent with other Redpanda Connect connectors, avoiding bespoke or idiosyncratic implementations."

Suggested fix: express schema selection through the fleet's include/exclude regex fields matched against schema.table (which subsumes both the schema glob and the per-schema tables resolution added here), or — if a schema-scoped field is genuinely required — align at least the matching syntax with the fleet's regexes and get the new names recorded in the §5.3 canonical list / internal/plugins/cdctest before landing.

}
return nil, fmt.Errorf("no schemas found matching schema_include pattern %q", config.DBSchemaInclude)
}
config.Logger.Infof("schema_include pattern %q resolved to %d schema(s): %v", config.DBSchemaInclude, len(schemas), schemas)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Schema-set drift across reconnects is silent (CONTRIBUTING §1.2.2, §3.1.4)

NewPgStream re-resolves schema_include on every connect and reconnect, but nothing compares the newly resolved set against the previous one, so two materially different outcomes are logged only as the ordinary Infof on this line. The field description added in this PR spells both out:

Field(service.NewDurationField(fieldHeartbeatInterval).
Description("The interval at which to write heartbeat messages. Heartbeat messages are needed in scenarios when the subscribed tables are low frequency, but there are other high frequency tables writing. Due to the checkpointing mechanism for replication slots, not having new messages to acknowledge will prevent postgres from reclaiming the write ahead log, which can exhaust the local disk. Having heartbeats allows Redpanda Connect to safely acknowledge data periodically and move forward the committed point in the log so it can be reclaimed. Setting the duration to 0s will disable heartbeats entirely. Heartbeats are created by periodically writing logical messages to the write ahead log using `pg_logical_emit_message`.").
ShortDescription("Interval at which to write heartbeat messages, keeping the replication slot current on low-traffic tables.").
Default("1h").

  1. A schema that starts matching after the slot exists is added to the publication, but its pre-existing rows are never snapshotted — the docs say they "are silently missing from the output".
  2. A schema that is dropped/renamed has its tables removed from the publication by CreatePublication a few lines below (
    // Batch DROP: single ALTER statement for all removed tables.
    if len(tablesToRemove) > 0 {
    var sb strings.Builder
    fmt.Fprintf(&sb, "ALTER PUBLICATION %s DROP TABLE ", publicationName)
    for i, t := range tablesToRemove {
    if i > 0 {
    sb.WriteString(", ")
    }
    sb.WriteString(t.String())
    }
    sb.WriteByte(';')
    sq, err := sanitize.SQLQuery(sb.String())
    if err != nil {
    return fmt.Errorf("sanitizing drop tables query: %w", err)
    }
    if _, err := conn.Exec(ctx, sq).ReadAll(); err != nil {
    return fmt.Errorf("removing tables from publication: %w", err)
    }
    }
    ) — the docs say "nothing is ever logged about the resulting publication drop itself".

CONTRIBUTING §1.2.2 requires that "Unexpected behavior should emit warning or error logs", and §3.1.4 asks for an implementation with "no known bugs or missing core functionality". Silently ceasing to replicate a tenant, or silently emitting an un-snapshotted tenant under stream_snapshot: true, are exactly the cases an operator needs a signal for — documenting them under §1.2.3 doesn't remove the §1.2.2 obligation.

Suggested fix: persist the resolved schema set on the Stream/config and, on each subsequent resolution, Warnf the added schemas (noting their snapshot is skipped) and the removed schemas (noting their tables are being dropped from the publication). The same applies to the inaccessibleSchemas warning above, which currently fires identically on every reconnect regardless of whether the set changed.

// the License. You may obtain a copy of the License at
//
// https://github.com/redpanda-data/connect/blob/main/licenses/rcl.md
// https://github.com/redpanda-data/connect/v4/blob/main/licenses/rcl.md

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

License header URL was changed to a broken link. This line previously read https://github.com/redpanda-data/connect/blob/main/licenses/rcl.md and this PR rewrote it to insert /v4/, which is not a real GitHub path (there is no redpanda-data/connect/v4 repository) — it 404s.

The canonical RCL header template is https://github.com/redpanda-data/connect/blob/bc81de9be3fb2c566b8998176479671610c240ea/licenses/rcl_header.go.txt, and the godev reference in CLAUDE.md documents the same non-/v4/ URL; 300+ files in the repo use it.

Please revert this line to the template URL, and use the template URL in the three new files added by this PR (pglogicalstream/schema_resolver.go, schema_resolver_test.go, schema_resolver_integration_test.go) rather than propagating the /v4/ variant. Note the new files are also inconsistent with each other on the copyright year (2026 vs 2024).

schemas = remaining
}

if len(inaccessibleSchemas) > 0 && !slices.Equal(inaccessibleSchemas, cfg.previouslyInaccessibleSchemas) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Order-sensitive comparison of an unordered query result → repeated warnings on every reconnect.

resolveSchemas builds inaccessibleSchemas from SELECT nspname FROM pg_catalog.pg_namespace WHERE ... with no ORDER BY (schema_resolver.go#L107-L131), so row order is not guaranteed to be stable between connects. slices.Equal here is therefore an order-sensitive comparison of what is semantically a set: if PostgreSQL returns the same two hidden schemas in a different order after a reconnect, the "missing USAGE privilege" warning re-fires even though nothing changed.

This is inconsistent with the adjacent drift detection a few lines below, which correctly uses set semantics via diffSchemaSets (schema_resolver.go#L186-L194).

Suggested fix: compare as a set (e.g. reuse diffSchemaSets and warn only when added/removed is non-empty), or add a deterministic ORDER BY to both queries in resolveSchemas so the slice comparison is meaningful. Repeated spurious warnings on a healthy pipeline also cut against CONTRIBUTING.md §1.2.2 ("Normal operation should emit no logs").

Comment thread internal/impl/postgresql/pglogicalstream/logical_stream.go Outdated
// conditionalConnectors lists CDC inputs that are only registered under specific
// build tags (e.g. cgo). They are exempt from the stale-entry guard when not
// registered, but are still subject to conformance checks when they are.
var conditionalConnectors = map[string]bool{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Out of scope for this PR.

This PR is scoped to postgres_cdc multi-schema replication (schema_include/schema_exclude), but this hunk adds a conditionalConnectors build-tag exemption and rewrites all five tigerbeetle_cdc waiver reasons (cdc_conformance_test.go#L102-L108) — none of which relate to postgres_cdc. The new postgres_cdc fields aren't part of canonicalFields, so this test needs no change for this feature.

CONTRIBUTING.md §3.1.1: "the PR stays within that agreed scope; additional components or capabilities beyond it are proposed and reviewed separately."

Suggested fix: split the cgo-conditional exemption and the tigerbeetle waiver re-classification into their own PR, so the CDC conformance-gate change gets reviewed on its own merits rather than as a drive-by in a Postgres feature.

@@ -0,0 +1,120 @@
version: "3"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bespoke manual harness committed at a non-descriptive path, with no README, duplicating coverage this PR already adds in CI.

internal/impl/postgresql/tests/current/ is a new top-level convention for this repo. The established equivalent is bench/, which this connector already has and which ships a README explaining how to run it (bench/README.md, bench/Taskfile.yaml). "current" doesn't describe what the directory holds, and nothing here tells a future reader when to reach for it.

Everything it exercises — the tenant_* glob, schema_exclude: [tenant_c] carve-out, database_schema metadata, and the two negative-validation cases — is already covered by testcontainers-backed tests added in this same PR (TestIntegrationMultiSchemaExcludeCarvesOutTenant, TestIntegrationMultiSchemaIncludeExcludeConfigValidation) plus the unit test TestSchemaExcludeRequiresSchemaInclude. Those run in CI; this harness can only silently rot.

CONTRIBUTING.md §3.1.5 ("codebase feels consistent with other Redpanda Connect connectors, avoiding bespoke or idiosyncratic implementations") and §6 ("No binaries, build artifacts, or local tooling committed").

Suggested fix: drop the directory, or — if the manual smoke path is genuinely wanted — move it under the existing bench/-style layout with a README stating its purpose and its relationship to the CI integration tests.

Comment thread internal/impl/postgresql/input_pg_stream.go
}
return nil, fmt.Errorf("no schemas found matching schema_include pattern %q", cfg.DBSchemaInclude)
}
cfg.Logger.Infof("schema_include pattern %q resolved to %d schema(s): %v", cfg.DBSchemaInclude, len(schemas), schemas)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Info logs fire on every connect and reconnect (CONTRIBUTING §1.2.2: "Provides relevant logging to support troubleshooting. Unexpected behavior should emit warning or error logs. Normal operation should emit no logs.")

Three new Infof calls run on the success path of every schema resolution, and — as the schema_include field docs added in this PR state — resolution is re-run on every connect and reconnect, including the automatic reconnects after a transient replication failure:

  • schema_resolver.go:146schema_exclude %v excluded %d schema(s) ...
  • schema_resolver.go:162schema_include pattern %q resolved to %d schema(s): %v (this line)
  • logical_stream.go:156%q has no tables list configured: auto-discovered ...

None of these describe unexpected behaviour, so on a flapping connection they become steady-state noise. This is also inconsistent with the adjacent inaccessible-schema warning at schema_resolver.go:151-154, which was deliberately de-duplicated across reconnects via previouslyInaccessibleSchemas.

Suggested fix: drop these to Debugf, or gate them on the resolved set actually having changed since the previous connect (the previouslyResolvedSchemas bookkeeping right below already computes exactly that).

// diffSchemaSets reports schemas present in current but not previous
// (added) and vice versa (removed). Callers should ignore the result when
// previous is nil - that's the first resolution, not real drift.
func diffSchemaSets(previous, current []string) (added, removed []string) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

New reconnect-drift logic has no test coverage.

Every other new helper in this file got a unit test in schema_resolver_test.go (globToLike, escapeLike, schemaPatternToLike, schemaMatchesExcludePattern), but diffSchemaSets and the previouslyResolvedSchemas / previouslyInaccessibleSchemas bookkeeping it feeds (schema_resolver.go:151-173) are untested — no unit test and no integration test exercises a second Connect() against a changed schema set.

This is the behaviour the schema_include docs call out as "easy to miss" (a schema appearing between reconnects is never snapshotted; a schema disappearing is silently dropped from the publication), so it's the part most worth pinning down. diffSchemaSets is a pure function over two slices and is trivially table-testable; the added/removed warning paths can be driven by resolving twice against a DB where a schema is created/dropped in between.

Per the project test patterns, changed code should carry tests.

Comment thread internal/impl/postgresql/tests/current/Taskfile.yaml
if cfg.previouslyResolvedSchemas != nil {
added, removed := diffSchemaSets(cfg.previouslyResolvedSchemas, schemas)
if len(added) > 0 {
cfg.Logger.Warnf("schema_include pattern %q now also matches schema(s) %v that did not match on the previous connect; their tables are being added to the publication, but any rows already in them will NOT be snapshotted even if stream_snapshot is enabled - only changes made from now on will be captured", cfg.DBSchemaInclude, added)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Silent data loss for schemas matched after the slot exists.

This warning states the defect in its own text: a schema that starts matching schema_include after the replication slot has been created gets its tables added to the publication, but "any rows already in them will NOT be snapshotted even if stream_snapshot is enabled". Because postgres_cdc uses the replication slot itself as its checkpoint, this also applies across a full process restart, not just a reconnect — once the slot exists, NewPgStream never re-runs the snapshot for the newly matched schema.

This is the primary use case for the feature: in a multi-tenant, schema-per-tenant database, provisioning a new tenant schema is the normal event, and every row written to that schema before the next reconnect is dropped from the output with only a WARN log.

This conflicts with:

  • CONTRIBUTING.md §5.5.2"Snapshot + streaming with a gap-free handoff".
  • CONTRIBUTING.md §3.1.4"The implementation is complete and correct, with no known bugs or missing core functionality." Documenting a known silent-loss path does not satisfy this.

Suggested direction: when diffSchemaSets reports added schemas and stream_snapshot is enabled, snapshot the newly discovered schemas before resuming streaming (capturing their stream position before the snapshot read, per §5.5.2), rather than adding them to the publication caught-up. If that is out of scope for this PR, the safer default would be to fail the reconnect instead of silently proceeding, with an explicit opt-in field for the current lossy behaviour.

See schema_resolver.go#L164-L172.

Comment on lines +41 to +42
fieldSchemaInclude = "schema_include"
fieldSchemaExclude = "schema_exclude"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bespoke include/exclude naming and pattern syntax vs. the CDC fleet.

The fleet already has a canonical include/exclude filter for CDC inputs. CONTRIBUTING.md §5.3.1 lists the canonical names as tables` (or `include`/`exclude`), §5.3.3 says "Do not invent bespoke names where a canonical one already exists", and §5.5.7 names the reference implementation: "Include/exclude regex table matching. Reference: oracledb_cdc."

oracledb_cdc implements that as top-level include/exclude string lists of regular expressions matched against SCHEMA.TABLE:

).
Field(service.NewStringField(ociFieldCheckpointCache).
Description("A https://www.docs.redpanda.com/redpanda-connect/components/caches/about[cache resource^] to use for storing the current System Change Number (SCN) that has been successfully delivered, this allows Redpanda Connect to continue from that System Change Number (SCN) upon restart, rather than consume the entire state of OracleDB's redo logs. If not set the default Oracle based cache will be used, see `" + ociFieldCheckpointCacheTableName + "` for more information.").
Optional(),
).
Field(service.NewStringField(ociFieldCheckpointCacheTableName).
Description("The identifier for the checkpoint cache table name. If no `" + ociFieldCheckpointCache + "` field is specified, this input will automatically create a table and stored procedure under the `rpcn` schema to act as a checkpoint cache. This table stores the latest processed System Change Number (SCN) that has been successfully delivered, allowing Redpanda Connect to resume from that point upon restart rather than reconsume the entire redo log. When `" + ociFieldPDBName + "` is set and this field is left at its default value, the table name is automatically derived per PDB (e.g. `RPCN.CDC_CHECKPOINT_MYPDB`) to avoid SCN collisions between pipelines monitoring different PDBs. Set this field explicitly to opt out of that auto-derivation.").
Default(defaultCheckpointCache).
Example("RPCN.CHECKPOINT_CACHE").

This PR instead adds schema_include (a single string) and schema_exclude (a list), both using a *-glob syntax rather than regex, for a filtering job the canonical include/exclude regex already covers (tenant_.*\..*). A user moving between oracledb_cdc and postgres_cdc now meets two different field names and two different pattern languages for the same concept, which is what §5.3.3 and §3.1.5 ("feels consistent with other Redpanda Connect connectors, avoiding bespoke or idiosyncratic implementations") are meant to prevent.

Worth reconciling with the CDC standard before this lands — either adopt include/exclude with regex semantics, or record an explicit, reasoned divergence.

// diffSchemaSets reports schemas present in current but not previous
// (added) and vice versa (removed). Callers should ignore the result when
// previous is nil - that's the first resolution, not real drift.
func diffSchemaSets(previous, current []string) (added, removed []string) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

diffSchemaSets has no unit test.

Every other pure helper added in this file — globToLike, escapeLike, schemaPatternToLike, schemaMatchesExcludePattern — got a table-driven unit test in the new schema_resolver_test.go, but diffSchemaSets did not. It is the function that decides whether the reconnect drift warnings fire, including the silent-data-loss warning at schema_resolver.go#L167, and none of the new integration tests exercise a reconnect with a changed schema set — so this logic is currently untested end to end as well.

It is a pure function over two slices; a table test covering added-only, removed-only, both, no change, and a nil previous costs very little and matches the pattern of its siblings in the same file. Worth adding per CONTRIBUTING.md §1.3.2.

@@ -0,0 +1,120 @@
version: "3"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tests/current/ duplicates the automated integration coverage and doesn't match any existing directory convention.

This adds a manually-driven docker-compose + Taskfile harness (Taskfile.yaml, docker-compose.yaml, setup.sql, test_config.yaml, test_config_schema_exclude_no_include.yaml) that exercises exactly what the new testcontainers-backed tests in this same PR already cover automatically — tenant_a/tenant_b/tenant_c with schema_include: tenant_* and schema_exclude: tenant_c is TestIntegrationMultiSchemaExcludeCarvesOutTenant, and test:schema-exclude-requires-include is TestSchemaExcludeRequiresSchemaInclude.

Harnesses of this shape do exist in the repo, but always under a named purpose directory — internal/impl/postgresql/bench/, internal/impl/iceberg/demo/, internal/impl/iceberg/e2e/, internal/impl/aws/resources/. There is no tests/ directory under internal/impl/ anywhere else, and current/ doesn't convey what it holds or when it stops being "current". postgresql/ already has a bench/ tree that this could fold into if a real-endpoint harness is the intent (which would also serve §1.3.4).

Per §3.1.5 ("avoiding bespoke or idiosyncratic implementations") and the §6 checklist ("No binaries, build artifacts, or local tooling committed"): either drop this in favour of the integration tests, or move it under a conventional directory name.

- |
set +e
go run ../../../../../cmd/redpanda-connect/main.go run \
--set 'input.postgres_cdc.schema_include=1abc' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

test:invalid-schema no longer exercises the failure path it documents.

The task feeds schema_include=1abc and the trailing comment asserts an invalid schema_include error with a non-zero exit — but validateSchemaPattern deliberately accepts a leading digit. The unit test in this same PR pins that behaviour explicitly:

{"1abc", ""},"a leading digit is not an identifier-syntax violation here … so 1abc must be as valid as any other unquoted pattern"
https://github.com/redpanda-data/connect/blob/bd0346b610cbc5cce5e0318be6b669b5af03d501/internal/impl/postgresql/input_pg_stream_test.go#L451-L458

So this task now starts a real pipeline run against PG_DSN instead of failing fast. After f39ff34 allowed leading digits, the only unquoted pattern validateSchemaPattern still rejects is one containing a " (e.g. a"b) — point the task at that, or drop it, since TestSchemaIncludeValidation already covers this deterministically without Docker.

Secondly, the exit-code check is inert in both smoke tasks: go run … 2>&1 | head -5 followed by echo "exit $?" reports head's status, not the binary's, so it prints exit 0 regardless of whether the connector failed. Capture the status with PIPESTATUS[0] or drop the head pipe.

Finally, on placement: internal/impl/postgresql/tests/ is the only tests/ directory under internal/impl/. Compose-backed harnesses in this repo live under bench/, demo/, or resources/ — and this connector already has internal/impl/postgresql/bench/. CONTRIBUTING §3.1.5 asks that connectors feel consistent with the rest of the codebase rather than introducing bespoke layouts; current/ in particular doesn't convey what the harness is for.

Optional().
Default("public"),
).
Field(service.NewStringField(fieldSchemaInclude).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

CDC standard conformance: schema_include / schema_exclude are bespoke names with bespoke matching semantics for a selection axis the fleet already has canonical fields for.

CONTRIBUTING §5.3.1 lists the canonical config names as ``tables(orinclude`/`exclude`)`, §5.3.3 says "Do not invent bespoke names where a canonical one already exists", and §5.5.7 specifies "Include/exclude regex table matching. Reference: `oracledb_cdc`". The reference implementation exposes those as top-level regex lists over fully-qualified `SCHEMA.TABLE` names:

Optional(),
).
Field(service.NewStringField(ociFieldCheckpointCache).
Description("A https://www.docs.redpanda.com/redpanda-connect/components/caches/about[cache resource^] to use for storing the current System Change Number (SCN) that has been successfully delivered, this allows Redpanda Connect to continue from that System Change Number (SCN) upon restart, rather than consume the entire state of OracleDB's redo logs. If not set the default Oracle based cache will be used, see `" + ociFieldCheckpointCacheTableName + "` for more information.").
Optional(),
).
Field(service.NewStringField(ociFieldCheckpointCacheTableName).
Description("The identifier for the checkpoint cache table name. If no `" + ociFieldCheckpointCache + "` field is specified, this input will automatically create a table and stored procedure under the `rpcn` schema to act as a checkpoint cache. This table stores the latest processed System Change Number (SCN) that has been successfully delivered, allowing Redpanda Connect to resume from that point upon restart rather than reconsume the entire redo log. When `" + ociFieldPDBName + "` is set and this field is left at its default value, the table name is automatically derived per PDB (e.g. `RPCN.CDC_CHECKPOINT_MYPDB`) to avoid SCN collisions between pipelines monitoring different PDBs. Set this field explicitly to opt out of that auto-derivation.").
Default(defaultCheckpointCache).
Example("RPCN.CHECKPOINT_CACHE").

microsoft_sql_server_cdc uses the same include/exclude pair. That shape already covers the multi-tenant case this PR targets (^tenant_.*\..*$ selects every table in every tenant_* schema, and exclusions are the same list with the opposite sense), so a user moving between postgres_cdc and oracledb_cdc now meets two different names and two different pattern languages — glob * here vs. regex there, which §5.5.7 names explicitly.

Worth confirming this divergence was agreed under §3.1.1 before it lands, since renaming later is a breaking change (§5.3.2 only makes it non-breaking if the canonical field is the one added). If glob-over-schemas really is the right UX for Postgres, please say why in the PR description and add a note to internal/plugins/cdctest so the deviation is recorded rather than discovered later.

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.

2 participants