Skip to content

Route partition DDL through pglogical on a provider database - #37

Closed
robertLichtnow wants to merge 4 commits into
masterfrom
pglogical-partition-ddl
Closed

robertLichtnow wants to merge 4 commits into
masterfrom
pglogical-partition-ddl

Conversation

@robertLichtnow

@robertLichtnow robertLichtnow commented Oct 29, 2026 •

Copy link
Copy Markdown

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 maintain creates 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 its CREATE is replicated, while every subscription keeps reporting replicating. This PR makes pgslice create partitions the way the chain needs.

  • --pglogical on add_partitions and maintain, 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.
  • With it on, each new partition's CREATE TABLE … PARTITION OF and pglogical.replication_set_add_table('<group>', …) go as one pglogical.replicate_ddl_command on ddl_<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, as saml_sessions) the primary key is declared inline, so no node ever holds a keyless partition whose replicated updates and deletes it couldn't apply.
  • A table's group comes from the database, the set its partitions are already in, so a table moved between groups (the tested procedure in workos/infra#2007, pglogical-move-table.md) is followed with no configuration change. A table with no partitions yet takes its group from a pglogical:<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).
  • Fails closed, creating nothing for the table, when: its partitions are spread across sets; it has partitions but none in a set; it has no partitions and no 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 its ddl_ pair, or a ddl_ set without its data set); or --intermediate is passed (the intermediate table only exists on the source). maintain also exits 1 for any table with a partition in no set, including one created outside pgslice.
  • The group is read with the parent locked in ACCESS EXCLUSIVE, the lock CREATE … PARTITION OF takes 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.
  • A new CI job runs the suite on Postgres 13 with pglogical (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 found instead of silently dropping rows. A failing subscription can still report replicating while 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 needs max_worker_processes headroom.

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 DROP through 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

  • After workos/workos#78107 deploys --pglogical on the staging maintain-core job (with the image built from this PR), the next run logs Extended table successfully or Table already up to date per 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.
  • Rollback: drop --pglogical from 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.

  • Suite: 420 passed with PGSLICE_PGLOGICAL_TEST=1 against 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.
  • The chain, real order (current 13.23 → PG18 18.6 → fallback 13.23, rest and saml on each hop), built CLI, maintain --pglogical on the current primary with saml_sessions (classic model, partitions in saml), posts (native model, in rest) and a brand-new empty new_sessions whose comment says pglogical:saml. Run 1 created each table's next partitions, new_sessions' first two in saml; 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 subscriptions replicating.
  • Following a move (current 13.23 → PG18 18.6). posts moved from rest to a new hot group with the workos/infra runbook's SQL (sql/move-table/00–05), under writes. The next maintain --pglogical put posts' new partitions in hot, queued on ddl_hot; every posts partition identical on PG18; sub_rest, sub_saml and sub_hot replicating.
  • Why membership has to travel in the command (same chain, by hand): a partition created through replicated DDL with membership added on the source only: PG18 1,000 of 1,000 rows, fallback 0, PG18's set without it, every subscription replicating. With the membership inside the command and no sets on the fallback: the fallback stops with replication set rest not found. With the sets everywhere: 1,000 rows and identical membership on all three.
  • The same chain results were first produced with the first two nodes' versions swapped (18.6 → 13.23 → 13.23) and are unchanged in the real order.

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.
@robertLichtnow
robertLichtnow marked this pull request as ready for review October 29, 2026 19:19
@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[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.

Findings

  1. P1 Built-in sets block partition creation ▶
  2. P2 Downstream behavior lacks automated tests ▶
Fix 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.

Summary

Adds optional pglogical routing for partition creation.

  • New partitions and their replication-set membership travel together through pglogical.
  • maintain flags partitions that are missing replication-set membership.

Diagram

sequenceDiagram
  participant CLI
  participant Provider
  participant Subscriber
  participant Fallback
  CLI->>Provider: maintain --pglogical
  Provider->>Provider: Lock parent and find group
  Provider->>Provider: Check sets
  Provider->>Provider: Create partition and add membership
  Provider-->>Subscriber: Replicated CREATE and membership
  Subscriber-->>Fallback: Replicated CREATE and membership
Loading

Reviews (2) · Last reviewed commit: "feature: Replace the pglogical group fla..." · Reviewed by Greptile

Comment thread src/pglogical.test.ts
Comment on lines +135 to +143
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)",
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 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.
Comment thread src/pglogical.ts
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')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 lxfontes left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚖️ greptile p1/p2 dont seem blocking

Comment thread src/pglogical.ts
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')

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants