Repository navigation
Route partition DDL through pglogical on a provider database - #37
robertLichtnow wants to merge 4 commits into
Conversation
On a pglogical provider, a partition created the usual way is never added to a replication set, so its rows silently don't reach the subscribers. With --pglogical-default-group (and per-table --pglogical-group), add_partitions and maintain create each partition through pglogical.replicate_ddl_command on the table's group DDL set, with its primary key inline and its grants, and add it to the group's replication set in the same transaction. maintain fails a table with a partition in no replication set.
The group was configured per table, so a table moved to another group kept being routed to the old one until the flag was changed too. pgslice now reads the group from the replication set the table's partitions are in, with the parent locked, and refuses when that's ambiguous (partitions across sets, none in a set, or a configured group that disagrees). The flags only name the group of a table with no partitions yet.
In a cascade (current PG13 -> PG18 -> PG13 fallback) PG18 is a provider too. A partition added to its set only on the source was created on PG18 and the fallback but never joined PG18's set, so its rows silently never reached the fallback. The CREATE and replication_set_add_table now travel as one replicated command, so every node that applies it adds the partition to its own set.
[Medium impact] The new routing affects partition creation and row replication for opted-in pglogical workflows. Fix the built-in-set check before merging so correctly configured providers can create partitions. FindingsFix with agent prompt### Issue 1
src/pglogical.ts:98
`assertReady` removes `default` and `default_insert_only` before checking whether the chosen group's sets exist. If a table's partitions belong to `default` and `ddl_default` exists, the check still reports `default` missing and refuses to create partitions. Even another group fails because `ddl_default` is then treated as unpaired.
Keep the built-in sets in the existence check. Exclude them only from checks that require unrelated sets to have a pair.
### Issue 2
src/pglogical.test.ts:134-142
The new tests check only the provider's queued SQL and local membership. They never apply that SQL on a subscriber or check that rows reach the last node in a chain. A later change could break downstream delivery while CI still passes.
Add an automated three-node test that creates a partition, checks its membership on each node, and checks inserts, updates, and deletes on the last node. This is non-blocking, but would protect the main behavior this PR adds.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds optional pglogical routing for partition creation.
|
| const queued = await queuedDdl(transaction); | ||
| expect(queued.map((q) => q.sets)).toEqual([["ddl_saml"], ["ddl_saml"]]); | ||
| expect(queued[0].command).toBe( | ||
| `CREATE TABLE "public"."saml_sessions_202602" PARTITION OF "public"."saml_sessions" (PRIMARY KEY ("id")) FOR VALUES FROM ('2026-02-01') TO ('2026-03-01'); SELECT pglogical.replication_set_add_table('saml', '"public"."saml_sessions_202602"'::regclass, false)`, | ||
| ); | ||
| // The partition exists here too, with its key: replicate_ddl_command ran it. | ||
| expect(await primaryKeyOf(transaction, "saml_sessions_202603")).toBe( | ||
| "PRIMARY KEY (id)", | ||
| ); |
There was a problem hiding this comment.
Downstream behavior lacks automated tests
The new tests check only the provider's queued SQL and local membership. They never apply that SQL on a subscriber or check that rows reach the last node in a chain. A later change could break downstream delivery while CI still passes.
Add an automated three-node test that creates a partition, checks its membership on each node, and checks inserts, updates, and deletes on the last node. This is non-blocking, but would protect the main behavior this PR adds.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/pglogical.test.ts
Line: 135-143
Comment:
**Downstream behavior lacks automated tests**
The new tests check only the provider's queued SQL and local membership. They never apply that SQL on a subscriber or check that rows reach the last node in a chain. A later change could break downstream delivery while CI still passes.
Add an automated three-node test that creates a partition, checks its membership on each node, and checks inserts, updates, and deletes on the last node. This is non-blocking, but would protect the main behavior this PR adds.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…tings key --pglogical is now the only flag: an on/off switch, off by default, so merging changes nothing until a job passes it. A table's group still comes from the set its partitions are in; a table with no partitions yet takes it from a pglogical:<group> key in its settings comment (ignored once it has partitions, and by older pgslice versions). Routing also refuses on a node with a half-created group, a data set without its ddl_ set or the reverse.
| FROM pglogical.local_node l | ||
| JOIN pglogical.replication_set s ON s.set_nodeid = l.node_id | ||
| LEFT JOIN pglogical.replication_set_table r USING (set_id) | ||
| WHERE s.set_name NOT IN ('default', 'default_insert_only', 'ddl_sql') |
There was a problem hiding this comment.
Built-in sets block partition creation
assertReady removes default and default_insert_only before checking whether the chosen group's sets exist. If a table's partitions belong to default and ddl_default exists, the check still reports default missing and refuses to create partitions. Even another group fails because ddl_default is then treated as unpaired.
Keep the built-in sets in the existence check. Exclude them only from checks that require unrelated sets to have a pair.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/pglogical.ts
Line: 98
Comment:
**Built-in sets block partition creation**
`assertReady` removes `default` and `default_insert_only` before checking whether the chosen group's sets exist. If a table's partitions belong to `default` and `ddl_default` exists, the check still reports `default` missing and refuses to create partitions. Even another group fails because `ddl_default` is then treated as unpaired.
Keep the built-in sets in the existence check. Exclude them only from checks that require unrelated sets to have a pair.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
I actually think this is worth closing before we merge this in. I don't think it blocks us internally, but this tool is public and I think this is worth addressing.
There was a problem hiding this comment.
Agreed—this is worth fixing before merge. For a table whose partitions are in pglogical’s built-in default set, groupFor returns default, but assertReady filters that set out, so it rejects the table even when ddl_default exists. The fix is to include built-in sets when checking the selected group’s required sets, while excluding them only from the generic pairing checks for unrelated groups.
lxfontes
left a comment
There was a problem hiding this comment.
⚖️ greptile p1/p2 dont seem blocking
| FROM pglogical.local_node l | ||
| JOIN pglogical.replication_set s ON s.set_nodeid = l.node_id | ||
| LEFT JOIN pglogical.replication_set_table r USING (set_id) | ||
| WHERE s.set_name NOT IN ('default', 'default_insert_only', 'ddl_sql') |
There was a problem hiding this comment.
I actually think this is worth closing before we merge this in. I don't think it blocks us internally, but this tool is public and I think this is worth addressing.
Summary
We're upgrading core-db from PG13 to PG18 with pglogical replicating in a chain, current PG13 → PG18 clone → PG13 fallback (the fallback is never a source), and pgslice's nightly
maintaincreates core-db's partitions. On a pglogical provider that breaks silently: pglogical adds no table to a replication set by itself, so a new partition's rows never reach the subscribers, even when itsCREATEis replicated, while every subscription keeps reportingreplicating. This PR makes pgslice create partitions the way the chain needs.--pglogicalonadd_partitionsandmaintain, off by default: merging this changes nothing until a job passes it. The plan is to pass it on the staging core-db job only (workos/workos#78107), then production when its provider exists.CREATE TABLE … PARTITION OFandpglogical.replication_set_add_table('<group>', …)go as onepglogical.replicate_ddl_commandonddl_<group>. Every node that applies the command (PG18, then the fallback) adds the partition to its own set too. Adding it only on the source, as the first version of this PR did, left PG18's set without it, and the fallback got 0 of its rows. Inherited grants go through the same DDL set. In the classic model (no key on the parent, assaml_sessions) the primary key is declared inline, so no node ever holds a keyless partition whose replicated updates and deletes it couldn't apply.pglogical-move-table.md) is followed with no configuration change. A table with no partitions yet takes its group from apglogical:<group>key in its settings comment (column:created_at,period:month,cast:date,version:3,pglogical:rest), written by the migration that creates it. The key is ignored once the table has partitions, so it never needs removing, and older pgslice versions ignore it (the parser skips unknown keys).pglogical:key; the group's sets don't exist on this node; this node has a half-created group (a data set holding tables without itsddl_pair, or addl_set without its data set); or--intermediateis passed (the intermediate table only exists on the source).maintainalso exits 1 for any table with a partition in no set, including one created outside pgslice.ACCESS EXCLUSIVE, the lockCREATE … PARTITION OFtakes anyway, and only when the run has a partition to create, so a concurrent move of the table can't land between reading the group and committing the partition.PGSLICE_PGLOGICAL_TEST=1); the existing matrix skips those tests.What reviewers should know about the chain. Every node needs the same sets, including the fallback that nothing subscribes to: a node without them stops replication with
replication set … not foundinstead of silently dropping rows. A failing subscription can still reportreplicatingwhile it retries, so lag and the apply errors in the log are what to watch. PG18 runs an apply worker per group and serves the fallback, so it needsmax_worker_processesheadroom.Not in this PR. Retention: pgslice has none yet. For #33, pglogical refuses to drop a table that is still in a set; removing the partition from its set on the source and replicating the
DROPthrough the same group's DDL set works through the whole chain (each node updates its own membership; removing it inside the replicated command works too). #34 rewrites the same creation code; whichever lands second carries the routing over. Partitioning an existing table (prep/fill/swap) while replicating needs its own design.Design notes and every finding behind this, with how each was tested: workos/infra#2007,
docs/projects/pg18-upgrade/learnings.md(sections "New partitions", "Retiring partitions", "The daisy chain", "Moving a table between groups"). Linear: INFRA-6780 (4.27).Test plan
--pglogicalon the stagingmaintain-corejob (with the image built from this PR), the next run logsExtended table successfullyorTable already up to dateper table and exits 0, and on the staging writer every partition is in a set:SELECT c.oid::regclass FROM pg_class c WHERE c.relnamespace = 'public'::regnamespace AND c.relkind = 'r' AND c.relispartition AND NOT EXISTS (SELECT 1 FROM pglogical.replication_set_table t WHERE t.set_reloid = c.oid);returns no rows. A first run that exits 1 listing partitions created before the switch is expected; adding them to their set clears it.--pglogicalfrom the job; partitions are created directly again.Proof
All local runs on pglogical 2.4.8. Every node's version was printed in the runs below.
PGSLICE_PGLOGICAL_TEST=1against PostgreSQL 13.23 (the 404 existing-style tests plus 16 pglogical ones); without the variable the 16 skip and the rest pass. CI on this commit is green, including the new pglogical job on Postgres 13 and the 13/15/17/18 matrix.restandsamlon each hop), built CLI,maintain --pglogicalon the current primary withsaml_sessions(classic model, partitions insaml),posts(native model, inrest) and a brand-new emptynew_sessionswhose comment sayspglogical:saml. Run 1 created each table's next partitions,new_sessions' first two insaml; run 2 created nothing, exit 0. After inserts, updates and deletes, every new partition's row checksum identical on all three nodes; membership identical on all three; all four subscriptionsreplicating.postsmoved fromrestto a newhotgroup with the workos/infra runbook's SQL (sql/move-table/00–05), under writes. The nextmaintain --pglogicalputposts' new partitions inhot, queued onddl_hot; everypostspartition identical on PG18;sub_rest,sub_samlandsub_hotreplicating.replicating. With the membership inside the command and no sets on the fallback: the fallback stops withreplication set rest not found. With the sets everywhere: 1,000 rows and identical membership on all three.