postgres_cdc: Fix behaviour of narrowing of publication from ALL TABLES to named table and vice versa

[josephwoodward]

Currently there are some behavioural bugs in the PostgreSQL connector that we should address in isolation before merging #4720. These exist when a user modifies their configuration to go from a list of tables, to no tables in either direction.

All tables to named-tables

When creating a publication with any of the following tables configurations, the publication is created as a FOR ALL TABLES, which tracks all tables in the schema.

input:
  postgres_cdc:
    ...
    schema: public
    slot_name: bench_slot
    ...

input:
  postgres_cdc:
    ...
    schema: public
    tables: []
    slot_name: bench_slot
    ...

If a user then modifies their configuration to narrow one or more tables:

input:
  postgres_cdc:
    ...
    schema: public
    tables:
        - table_to_track
    slot_name: bench_slot
    ...

Instead of narrowing the publication to the table_to_track table, the publication still remains as tracking all tables (FOR ALL TABLES).

The consequence is there's no data loss, however the connector is still publishing change events for all tables in the schema (including new ones created later, and anything containing PII the user thought they'd excluded by narrowing)

How does this happen?

The GetPublicationTables attempts to find the tables being tracked before deciding whether to narrow it (if one or more tables are supplied) or expand it to FOR ALL TABLES (if no tables are supplied), however it's checking pg_publication_tables which is a view, not the authoritative membership catalog (it should use pg_publication.puballtables instead).

Named-tables to All Tables

Conversely, when modifying the configuration to go from named tables to FOR ALL TABLES, the table(s) are removed from the publication but the publication is left empty, meaning no tables would be tracked no change events streamed.

Breaking Behavioural Change

By fixing the incorrect aforementioned behaviour, the connector now attempts to modify the publication. However PostgreSQL does not altering a publication and instead requires it to be dropped and recreated. This impacts the user in the following ways:

	FOR ALL TABLES → named tables (narrowing)	named tables → FOR ALL TABLES (widening)
Mechanism	DROP PUBLICATION + CREATE PUBLICATION ... FOR TABLE ...	DROP PUBLICATION + CREATE PUBLICATION ... FOR ALL TABLES
To DROP the existing publication	Must own the publication (or be superuser)	Must own the publication (or be superuser)
To CREATE the new one	CREATE privilege on the database + ownership of the tables being published	Superuser — no privilege grant on the database or tables substitutes for this
Superuser required?	No	Yes
On failure	"the connected role must own %q and have CREATE privilege on the database; either grant those, or drop and recreate the publication manually"	"creating a FOR ALL TABLES publication requires superuser; either grant that, or drop and recreate the publication manually"

Proof of Work

Along with the automated tests in this change, the following screenshot demonstrates:

1. tables: [] creates a FOR ALL TABLES publication.
2. Switching to named tables (users) recreates the publication with named tables.
3. Increasing named tables from users to users, products and cart.
4. Reducing named tables from three (users, products and cart) to products and users.

Switching from initial creation of 3 named tables to FOR ALL TABLES:

[squiidz]

1. Widening loses data silently (logical_stream.go:195). Going from tables: [a] to empty (FOR ALL TABLES) drop/recreates the publication, but the stream resumes from the slot's old LSN and pgoutput filters retained WAL against the historic catalog — where the publication only contained table a. Changes to other tables written before the restart are silently dropped, and snapshot backfill doesn't kick in.

2. Narrowing leaks data (pglogrepl.go:332). The mirror image, and it directly undercuts the PR's motivation: narrowing to exclude PII tables still emits events for those tables for all WAL retained before the recreate, because retained WAL decodes against the old FOR ALL TABLES catalog.

3. Upgrade regression (pglogrepl.go:351). A non-superuser deployment with an untouched tables: [] config against a pre-existing named-table publication now hard-fails at startup with "requires superuser" where the old code started up. Needs at least a changelog/upgrade note — the docs frame this as a consequence of "changing this list", which isn't the only trigger.

4. Destructive op ordered before validation (logical_stream.go:195). The drop/recreate runs before the slot's output-plugin validation, so a startup that's doomed to fail anyway still destroys the operator's publication WITH options.

[josephwoodward]

Widening loses data silently (logical_stream.go:195). Going from tables: [a] to empty (FOR ALL TABLES) drop/recreates the publication, but the stream resumes from the slot's old LSN and pgoutput filters retained WAL against the historic catalog — where the publication only contained table a. Changes to other tables written before the restart are silently dropped, and snapshot backfill doesn't kick in.

This one isn't an issue as it'll only be for new tables, so it's expected behaviour to start streaming from the point of adding a new table.

Narrowing leaks data (pglogrepl.go:332). The mirror image, and it directly undercuts the PR's motivation: narrowing to exclude PII tables still emits events for those tables for all WAL retained before the recreate, because retained WAL decodes against the old FOR ALL TABLES catalog.

The only data the user will see in this case is the point up into the WAL drops and recreates the publication.

Upgrade regression (pglogrepl.go:351). A non-superuser deployment with an untouched tables: [] config against a pre-existing named-table publication now hard-fails at startup with "requires superuser" where the old code started up. Needs at least a changelog/upgrade note — the docs frame this as a consequence of "changing this list", which isn't the only trigger.

It started up because the behaviour was incorrect. I've updated the docs to highlight that this particular use case (going from named tables to all tables) requires superuser permission. See commit 0d0cf87.

Destructive op ordered before validation (logical_stream.go:195). The drop/recreate runs before the slot's output-plugin validation, so a startup that's doomed to fail anyway still destroys the operator's publication WITH options.

Good idea, output plugin validation moved above destructive behaviour. Addressed in commit a305a5d.

[claude]

This reordering — validating the slot's output plugin before CreatePublication can drop and recreate the publication — is a real behaviour change (it's the whole point of commit a305a5d), but it lands with no test. Nothing in the repo currently exercises the "slot exists with a different output plugin" branch either (it's only referenced at logical_stream.go:205-207), so the guarantee "we don't destroy the user's publication when the slot is unusable" is unverified and is easy to silently regress by moving these blocks again.

An integration test in pglogicalstream could create a slot with a non-pgoutput plugin (e.g. test_decoding, available in the stock postgres:16 image already used by createDockerInstance) plus a named-table publication, call NewPgStream with an empty tables list, and assert both that the error mentions the plugin mismatch and that puballtables is still false.

Per CONTRIBUTING.md §1.3.2/§1.3.3 — tests should cover end-to-end functionality and integration tests verify core workflows.

Ref: logical_stream.go#L201-L217

[Jeffail]

Thanks for this — the underlying fix looks right to me (checking pg_publication.puballtables rather than inferring from the view is clearly the correct source of truth, and moving the slot-plugin validation ahead of the destructive DDL is a nice touch). I ran a fairly deep review pass and a handful of things came out of it that seem worth a look before merging. Hedging appropriately — happy to be wrong on any of these:

1. Backlogged WAL decodes under the old publication membership. From what I can tell, pgoutput resolves membership against the historic catalog at each WAL record's time, so recreating the publication only affects records written after the recreate. Widening named→ALL with an existing slot means backlog changes to newly-included tables are silently skipped (and nothing snapshots them, since the slot exists); narrowing ALL→named still delivers the excluded tables' backlog up to the recreate point — which slightly softens the PII framing in the description. The old behaviour effectively hard-errored on this transition, so it could never happen silently. Might be worth at least a doc paragraph, and possibly a startup warning?

2. The recreate runs before we've proven we can acquire the slot (logical_stream.go — CreatePublication before startLr). In a rolling deploy, a second instance with a shape-changed config would drop/recreate the publication out from under the instance currently streaming, then fail anyway on the active slot. Relatedly, two racing startups both pass the pg_publication check and the loser's DROP fails with a non-42501 "does not exist", so it crash-loops until the race resolves. Concurrent instances on one slot were arguably never supported, so maybe this is accept-with-docs — but it seems worth being a deliberate call rather than an accident, since main had no DROP PUBLICATION path at all.

3. The new field docs claim in-place add/remove "needs no elevated privileges beyond table ownership" — my read of Postgres is that ALTER PUBLICATION ... ADD/DROP TABLE also requires owning the publication itself. That breaks exactly the pre-created-publication-under-an-admin-role pattern the same page recommends, and that ALTER-path 42501 doesn't get the friendly guidance wrapping either.

4. The 42501 guidance is a bit inconsistent between branches. The first-time CREATE PUBLICATION ... FOR ALL TABLES path returns the raw error with no guidance, even though the docs say the superuser requirement applies to first-time creation too. And in the recreate branch the message is chosen by the desired shape rather than which statement failed — e.g. a DROP failing with "must be owner of publication" gets labelled a superuser-for-FOR-ALL-TABLES problem, which could steer remediation the wrong way.

5. A few smaller ones, bundled: (a) the ALTER PUBLICATION ADD/DROP TABLE loops now run silently, whereas main logged the table set on every startup — feels inconsistent with the (good!) "warn loudly" rationale on the recreate path, so maybe an Infof per mutation or a summary line; (b) GetPublicationTables still reports a missing publication as "exists and is FOR ALL TABLES", and now queries puballtables a second time only for the caller to discard it — since this rewrite touched exactly that logic, it might be a good moment to return an honest not-found instead; (c) the DROP+CREATE atomicity rests on pgconn's implicit single-transaction wrapping of a multi-statement simple query — which is real and correct today, but it's guarded only by a comment plus a Docker-gated integration test, and it looks like the integration-test job skipped on this PR's CI run; an explicit BEGIN;...;COMMIT; would make it refactor-proof for free, and worth double-checking the new tests ran somewhere CI-visible.

None of this undermines the direction — the shape of the fix seems right to me, and 3–5 look like cheap wins. Happy to dig further into any of them in-thread if useful, and totally fine if some of these land as follow-ups.