From 8bd1a4914898c503ef3bbafd9f6d8f546c0b0d3b Mon Sep 17 00:00:00 2001 From: Stacy Curry Date: Mon, 12 Oct 2026 09:53:25 -0500 Subject: [PATCH 1/6] fix: Disable mirroring triggers instead of dropping them; lock the parent first on drop DROP TRIGGER on a partitioned parent deletes the cloned child triggers before the parent trigger, so it locks the partitions before the parent and deadlocks against statements entering through the parent. On oauth_sessions in production that killed application statements for 53 seconds before the drop itself was chosen as the victim. disable_mirroring and disable_retired_mirroring now turn the trigger off with ALTER TABLE ... DISABLE TRIGGER, which locks the parent first and never blocks reads, and leave the trigger and function in place. --drop removes them under an explicit ACCESS EXCLUSIVE lock on the table, which swap and unswap now use as well. The mirroring commands take --lock-timeout (default 5s), and status reports whether each trigger is enabled. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 3 + README.md | 4 +- src/commands/disable-mirroring.test.ts | 50 +++ src/commands/disable-mirroring.ts | 35 +- .../disable-retired-mirroring.test.ts | 56 +++ src/commands/disable-retired-mirroring.ts | 32 +- src/commands/enable-mirroring.ts | 8 +- src/commands/enable-retired-mirroring.ts | 9 +- src/commands/status.test.ts | 2 + src/mirroring.test.ts | 321 +++++++++++++++++- src/mirroring.ts | 79 ++++- src/pgslice.test.ts | 4 + src/pgslice.ts | 45 ++- src/swapper.ts | 2 +- src/table.ts | 23 ++ src/types.ts | 14 + 16 files changed, 657 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 609f487..920152e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## Unreleased +- Changed `disable_mirroring` and `disable_retired_mirroring` to turn the trigger off with `ALTER TABLE ... DISABLE TRIGGER` instead of dropping it. On a partitioned table the drop locked every partition before the parent and deadlocked against live traffic; disabling locks the parent first, never blocks reads, and leaves the trigger and function in place so `enable_mirroring` can turn them back on. Pass `--drop` to remove the trigger and function, which now takes an explicit ACCESS EXCLUSIVE lock on the table first. +- Added `--lock-timeout` (default `5s`) to the enable and disable mirroring commands, matching `swap`. +- Added `mirrorTriggerEnabled` and `retiredMirrorTriggerEnabled` to `status`, so a disabled trigger is distinguishable from an active one. - Added support for managing existing (already-partitioned) tables: bounds-anchored extension that recognizes legacy-named partitions and extends contiguously without renaming, gaps, or overlaps - Added a weekly (ISO-week, Monday-aligned) period - Added composite / parent-owned primary key support to `add_partitions` (skips the redundant per-partition key when the parent owns one) diff --git a/README.md b/README.md index 4640897..a6628a8 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,8 @@ pgslice disable_mirroring After the swap, the original mirroring triggers are no longer needed since the tables have been swapped. +This turns the trigger off rather than dropping it, which never blocks reads and cannot deadlock against live traffic on a partitioned table. Pass `--drop` to remove the trigger and its function; that takes an ACCESS EXCLUSIVE lock on the table, acquired before anything else so it is safe on a partitioned table too. Both forms honour `--lock-timeout` (default: `5s`). + 11. Enable Reverse Mirroring (now-partitioned table to retired table) This will make unswapping later less problematic as the two tables are kept in sync. Note that @@ -168,7 +170,7 @@ pgslice fill
--swapped pgslice disable_retired_mirroring
``` -Once you're confident the retired table is no longer needed and you're ready to drop it, disable the retired mirroring triggers. +Once you're confident the retired table is no longer needed, disable the retired mirroring triggers. This turns the trigger off under a light lock, parent first, so it is safe to run against a busy partitioned table; `status` then reports the trigger as present but disabled. When you drop the retired table, run it again with `--drop` to remove the trigger and its function. Both forms honour `--lock-timeout` (default: `5s`). 14. Back up the retired table with a tool like [pg_dump](https://www.postgresql.org/docs/current/static/app-pgdump.html) and drop it diff --git a/src/commands/disable-mirroring.test.ts b/src/commands/disable-mirroring.test.ts index df07948..ca658e1 100644 --- a/src/commands/disable-mirroring.test.ts +++ b/src/commands/disable-mirroring.test.ts @@ -1,5 +1,6 @@ import { describe, expect } from "vitest"; import { sql } from "slonik"; +import { z } from "zod"; import { commandTest as test } from "../testing/index.js"; import { DisableMirroringCommand } from "./disable-mirroring.js"; @@ -76,6 +77,55 @@ describe("DisableMirroringCommand", () => { const output = commandContext.stdout.read()?.toString(); expect(output).toContain("Mirroring triggers disabled for posts"); }); + test("keeps the trigger in place but disabled by default", async ({ + cli, + commandContext, + pgslice, + transaction, + }) => { + await pgslice.enableMirroring(transaction, { table: "posts" }); + + const exitCode = await cli.run( + ["mirroring", "disable", "posts"], + commandContext, + ); + + expect(exitCode).toBe(0); + const trigger = await transaction.one( + sql.type(z.object({ tgenabled: z.string() }))` + SELECT tgenabled FROM pg_trigger WHERE tgname = 'posts_mirror_trigger' + `, + ); + expect(trigger.tgenabled).toBe("D"); + }); + + test("drops the trigger and function with --drop", async ({ + cli, + commandContext, + pgslice, + transaction, + }) => { + await pgslice.enableMirroring(transaction, { table: "posts" }); + + const exitCode = await cli.run( + ["mirroring", "disable", "posts", "--drop"], + commandContext, + ); + + expect(exitCode).toBe(0); + const output = commandContext.stdout.read()?.toString(); + expect(output).toContain("Mirroring triggers dropped for posts"); + + const remaining = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT ( + (SELECT COUNT(*) FROM pg_trigger WHERE tgname = 'posts_mirror_trigger') + + (SELECT COUNT(*) FROM pg_proc WHERE proname = 'posts_mirror_to_intermediate') + )::int AS count + `, + ); + expect(remaining).toBe(0); + }); }); describe("schema-qualified tables", () => { diff --git a/src/commands/disable-mirroring.ts b/src/commands/disable-mirroring.ts index 9c8917f..ba1766b 100644 --- a/src/commands/disable-mirroring.ts +++ b/src/commands/disable-mirroring.ts @@ -9,8 +9,15 @@ export class DisableMirroringCommand extends BaseCommand { static override usage = Command.Usage({ description: "Disable mirroring triggers after partitioning is complete", details: ` - This command removes the triggers on the source table that mirror operations - to the intermediate table. Use this after partitioning is complete. + This command turns off the trigger on the source table that mirrors + operations to the intermediate table. Use this after partitioning is + complete. + + The trigger is disabled, not dropped: on a partitioned table that takes a + light lock on the parent first and never blocks reads. Pass --drop to + remove the trigger and its function instead, which takes an ACCESS + EXCLUSIVE lock on the table for the duration of the drop. Either way the + lock wait is bounded by --lock-timeout. `, examples: [ ["Disable mirroring for a table", "$0 mirroring disable posts"], @@ -18,16 +25,36 @@ export class DisableMirroringCommand extends BaseCommand { "Disable mirroring with explicit schema", "$0 mirroring disable myschema.posts", ], + [ + "Remove the trigger and function entirely", + "$0 mirroring disable posts --drop", + ], + [ + "With custom lock timeout", + "$0 mirroring disable posts --lock-timeout 10s", + ], ], }); table = Option.String({ required: true, name: "table" }); + lockTimeout = Option.String("--lock-timeout", "5s", { + description: "Bound on how long to wait for the table lock", + }); + drop = Option.Boolean("--drop", false, { + description: "Drop the trigger and its function instead of disabling", + }); override async perform(pgslice: Pgslice): Promise { await pgslice.start(async (tx) => { - await pgslice.disableMirroring(tx, { table: this.table }); + await pgslice.disableMirroring(tx, { + table: this.table, + lockTimeout: this.lockTimeout, + drop: this.drop, + }); this.context.stdout.write( - `Mirroring triggers disabled for ${this.table}\n`, + this.drop + ? `Mirroring triggers dropped for ${this.table}\n` + : `Mirroring triggers disabled for ${this.table}\n`, ); }); } diff --git a/src/commands/disable-retired-mirroring.test.ts b/src/commands/disable-retired-mirroring.test.ts index 527f7fd..28b5d3c 100644 --- a/src/commands/disable-retired-mirroring.test.ts +++ b/src/commands/disable-retired-mirroring.test.ts @@ -1,5 +1,6 @@ import { describe, expect } from "vitest"; import { sql } from "slonik"; +import { z } from "zod"; import { commandTest as test } from "../testing/index.js"; import { DisableRetiredMirroringCommand } from "./disable-retired-mirroring.js"; @@ -84,6 +85,61 @@ describe("DisableRetiredMirroringCommand", () => { const output = commandContext.stdout.read()?.toString(); expect(output).toContain("Retired mirroring triggers disabled for posts"); }); + test("keeps the trigger in place but disabled by default", async ({ + cli, + commandContext, + pgslice, + transaction, + }) => { + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + const exitCode = await cli.run( + ["mirroring", "disable-retired", "posts"], + commandContext, + ); + + expect(exitCode).toBe(0); + const trigger = await transaction.one( + sql.type(z.object({ tgenabled: z.string() }))` + SELECT tgenabled FROM pg_trigger WHERE tgname = 'posts_retired_mirror_trigger' + `, + ); + expect(trigger.tgenabled).toBe("D"); + }); + + test("drops the trigger and function with --drop", async ({ + cli, + commandContext, + pgslice, + transaction, + }) => { + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + const exitCode = await cli.run( + ["mirroring", "disable-retired", "posts", "--drop"], + commandContext, + ); + + expect(exitCode).toBe(0); + const output = commandContext.stdout.read()?.toString(); + expect(output).toContain("Retired mirroring triggers dropped for posts"); + + const remaining = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT ( + (SELECT COUNT(*) FROM pg_trigger WHERE tgname = 'posts_retired_mirror_trigger') + + (SELECT COUNT(*) FROM pg_proc WHERE proname = 'posts_mirror_to_retired') + )::int AS count + `, + ); + expect(remaining).toBe(0); + }); }); describe("schema-qualified tables", () => { diff --git a/src/commands/disable-retired-mirroring.ts b/src/commands/disable-retired-mirroring.ts index dd72ebc..b251c56 100644 --- a/src/commands/disable-retired-mirroring.ts +++ b/src/commands/disable-retired-mirroring.ts @@ -12,9 +12,15 @@ export class DisableRetiredMirroringCommand extends BaseCommand { static override usage = Command.Usage({ description: "Disable retired mirroring triggers", details: ` - This command removes the triggers on the source table that mirror operations - to the retired table. Use this when you no longer need to keep the retired - table in sync with the main table. + This command turns off the trigger on the source table that mirrors + operations to the retired table. Use this when you no longer need to keep + the retired table in sync with the main table. + + The trigger is disabled, not dropped: on a partitioned table that takes a + light lock on the parent first and never blocks reads. Pass --drop to + remove the trigger and its function instead, which takes an ACCESS + EXCLUSIVE lock on the table for the duration of the drop. Either way the + lock wait is bounded by --lock-timeout. `, examples: [ [ @@ -25,19 +31,37 @@ export class DisableRetiredMirroringCommand extends BaseCommand { "Disable retired mirroring with explicit schema", "$0 mirroring disable-retired myschema.posts", ], + [ + "Remove the trigger and function entirely", + "$0 mirroring disable-retired posts --drop", + ], + [ + "With custom lock timeout", + "$0 mirroring disable-retired posts --lock-timeout 10s", + ], ], }); table = Option.String({ required: true, name: "table" }); + lockTimeout = Option.String("--lock-timeout", "5s", { + description: "Bound on how long to wait for the table lock", + }); + drop = Option.Boolean("--drop", false, { + description: "Drop the trigger and its function instead of disabling", + }); override async perform(pgslice: Pgslice): Promise { await pgslice.start(async (tx) => { await pgslice.disableMirroring(tx, { table: this.table, targetType: "retired", + lockTimeout: this.lockTimeout, + drop: this.drop, }); this.context.stdout.write( - `Retired mirroring triggers disabled for ${this.table}\n`, + this.drop + ? `Retired mirroring triggers dropped for ${this.table}\n` + : `Retired mirroring triggers disabled for ${this.table}\n`, ); }); } diff --git a/src/commands/enable-mirroring.ts b/src/commands/enable-mirroring.ts index aa5815b..a0b667d 100644 --- a/src/commands/enable-mirroring.ts +++ b/src/commands/enable-mirroring.ts @@ -24,10 +24,16 @@ export class EnableMirroringCommand extends BaseCommand { }); table = Option.String({ required: true, name: "table" }); + lockTimeout = Option.String("--lock-timeout", "5s", { + description: "Bound on how long to wait for the table lock", + }); override async perform(pgslice: Pgslice): Promise { await pgslice.start(async (tx) => - pgslice.enableMirroring(tx, { table: this.table }), + pgslice.enableMirroring(tx, { + table: this.table, + lockTimeout: this.lockTimeout, + }), ); this.context.stdout.write(`Mirroring triggers enabled for ${this.table}\n`); diff --git a/src/commands/enable-retired-mirroring.ts b/src/commands/enable-retired-mirroring.ts index 6f606b3..ca8f488 100644 --- a/src/commands/enable-retired-mirroring.ts +++ b/src/commands/enable-retired-mirroring.ts @@ -34,10 +34,17 @@ export class EnableRetiredMirroringCommand extends BaseCommand { }); table = Option.String({ required: true, name: "table" }); + lockTimeout = Option.String("--lock-timeout", "5s", { + description: "Bound on how long to wait for the table lock", + }); override async perform(pgslice: Pgslice): Promise { await pgslice.start(async (tx) => - pgslice.enableMirroring(tx, { table: this.table, targetType: "retired" }), + pgslice.enableMirroring(tx, { + table: this.table, + targetType: "retired", + lockTimeout: this.lockTimeout, + }), ); this.context.stdout.write( diff --git a/src/commands/status.test.ts b/src/commands/status.test.ts index 47dd460..477d7c5 100644 --- a/src/commands/status.test.ts +++ b/src/commands/status.test.ts @@ -28,7 +28,9 @@ describe("StatusCommand", () => { intermediateExists: false, partitionCount: 0, mirrorTriggerExists: false, + mirrorTriggerEnabled: false, retiredMirrorTriggerExists: false, + retiredMirrorTriggerEnabled: false, originalIsPartitioned: false, }); }); diff --git a/src/mirroring.test.ts b/src/mirroring.test.ts index d206735..218c736 100644 --- a/src/mirroring.test.ts +++ b/src/mirroring.test.ts @@ -1,5 +1,5 @@ import { describe, expect } from "vitest"; -import { sql } from "slonik"; +import { DatabaseTransactionConnection, sql } from "slonik"; import { z } from "zod"; import { pgsliceTest as test } from "./testing/index.js"; @@ -245,9 +245,12 @@ describe("Pgslice.disableMirroring", () => { `); }); - test("drops trigger function", async ({ pgslice, transaction }) => { + test("drops trigger function with drop", async ({ pgslice, transaction }) => { await pgslice.enableMirroring(transaction, { table: "posts" }); - await pgslice.disableMirroring(transaction, { table: "posts" }); + await pgslice.disableMirroring(transaction, { + table: "posts", + drop: true, + }); const count = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` @@ -258,9 +261,12 @@ describe("Pgslice.disableMirroring", () => { expect(count).toBe(0); }); - test("drops trigger", async ({ pgslice, transaction }) => { + test("drops trigger with drop", async ({ pgslice, transaction }) => { await pgslice.enableMirroring(transaction, { table: "posts" }); - await pgslice.disableMirroring(transaction, { table: "posts" }); + await pgslice.disableMirroring(transaction, { + table: "posts", + drop: true, + }); const count = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` @@ -336,7 +342,10 @@ describe("Pgslice.disableMirroring", () => { `); await pgslice.enableMirroring(transaction, { table: "myschema.posts" }); - await pgslice.disableMirroring(transaction, { table: "myschema.posts" }); + await pgslice.disableMirroring(transaction, { + table: "myschema.posts", + drop: true, + }); const count = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` @@ -358,3 +367,303 @@ describe("Pgslice.disableMirroring", () => { }); }); }); + +describe("Pgslice.disableMirroring (disable vs drop)", () => { + const triggerCount = async ( + transaction: DatabaseTransactionConnection, + name: string, + ) => + transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM pg_trigger WHERE tgname = ${name} + `, + ); + + const enabledTriggerCount = async ( + transaction: DatabaseTransactionConnection, + name: string, + ) => + transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM pg_trigger + WHERE tgname = ${name} AND tgenabled <> 'D' + `, + ); + + const functionCount = async ( + transaction: DatabaseTransactionConnection, + name: string, + ) => + transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM pg_proc WHERE proname = ${name} + `, + ); + + describe("plain table", () => { + test.beforeEach(async ({ pgslice, transaction }) => { + await transaction.query(sql.unsafe` + CREATE TABLE posts ( + id SERIAL PRIMARY KEY, + title TEXT NOT NULL, + created_at DATE NOT NULL + ) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_retired ( + id SERIAL PRIMARY KEY, + title TEXT NOT NULL, + created_at DATE NOT NULL + ) + `); + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + }); + + test("turns the trigger off but keeps the trigger and its function", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(1); + expect( + await enabledTriggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(0); + expect(await functionCount(transaction, "posts_mirror_to_retired")).toBe( + 1, + ); + }); + + test("stops mirroring writes once disabled", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + await transaction.query(sql.unsafe` + INSERT INTO posts (title, created_at) VALUES ('Hello', '2024-01-15') + `); + + const count = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM posts_retired + `, + ); + expect(count).toBe(0); + }); + + test("enableMirroring turns a disabled trigger back on in place", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(1); + expect( + await enabledTriggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(1); + + await transaction.query(sql.unsafe` + INSERT INTO posts (title, created_at) VALUES ('Hello', '2024-01-15') + `); + const count = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM posts_retired + `, + ); + expect(count).toBe(1); + }); + + test("drop removes the trigger and its function", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + drop: true, + }); + + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(0); + expect(await functionCount(transaction, "posts_mirror_to_retired")).toBe( + 0, + ); + }); + + test("is idempotent when the trigger is already gone", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + drop: true, + }); + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + drop: true, + }); + + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(0); + }); + + test("bounds the lock wait with the requested lock_timeout", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + lockTimeout: "7s", + }); + + const lockTimeout = await transaction.oneFirst( + sql.type(z.object({ lock_timeout: z.string() }))`SHOW lock_timeout`, + ); + expect(lockTimeout).toBe("7s"); + }); + }); + + describe("partitioned table", () => { + test.beforeEach(async ({ pgslice, transaction }) => { + await transaction.query(sql.unsafe` + CREATE TABLE posts ( + id SERIAL, + title TEXT NOT NULL, + created_at DATE NOT NULL + ) PARTITION BY RANGE (created_at) + `); + await transaction.query(sql.unsafe` + CREATE UNIQUE INDEX posts_replica_identity_uidx ON posts (id, created_at) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_202401 PARTITION OF posts + FOR VALUES FROM ('2024-01-01') TO ('2024-02-01') + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_202402 PARTITION OF posts + FOR VALUES FROM ('2024-02-01') TO ('2024-03-01') + `); + await transaction.query(sql.unsafe` + ALTER TABLE posts_202401 ADD PRIMARY KEY (id) + `); + await transaction.query(sql.unsafe` + ALTER TABLE posts_202402 ADD PRIMARY KEY (id) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_retired ( + id INTEGER PRIMARY KEY, + title TEXT NOT NULL, + created_at DATE NOT NULL + ) + `); + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + }); + + test("disables the cloned trigger on every partition", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + const rows = await transaction.any( + sql.type(z.object({ relname: z.string(), tgenabled: z.string() }))` + SELECT c.relname, t.tgenabled + FROM pg_trigger t JOIN pg_class c ON c.oid = t.tgrelid + WHERE t.tgname = 'posts_retired_mirror_trigger' + ORDER BY c.relname + `, + ); + expect(rows).toEqual([ + { relname: "posts", tgenabled: "D" }, + { relname: "posts_202401", tgenabled: "D" }, + { relname: "posts_202402", tgenabled: "D" }, + ]); + + await transaction.query(sql.unsafe` + INSERT INTO posts (title, created_at) VALUES ('Hello', '2024-02-15') + `); + const count = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM posts_retired + `, + ); + expect(count).toBe(0); + }); + + test("re-enables the cloned trigger on every partition", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + const disabled = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM pg_trigger + WHERE tgname = 'posts_retired_mirror_trigger' AND tgenabled = 'D' + `, + ); + expect(disabled).toBe(0); + }); + + test("drop removes the trigger from the parent and every partition", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + drop: true, + }); + + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(0); + expect(await functionCount(transaction, "posts_mirror_to_retired")).toBe( + 0, + ); + }); + }); +}); diff --git a/src/mirroring.ts b/src/mirroring.ts index 1f470bc..0223520 100644 --- a/src/mirroring.ts +++ b/src/mirroring.ts @@ -1,4 +1,4 @@ -import { DatabaseTransactionConnection } from "slonik"; +import { CommonQueryMethods, DatabaseTransactionConnection } from "slonik"; import { Table } from "./table.js"; import { sql } from "./sql-utils.js"; @@ -14,6 +14,13 @@ interface MirroringOptions { * Manages database mirroring triggers between a source table and a target table. * These triggers ensure that INSERT, UPDATE, and DELETE operations on the source * table are automatically replicated to the target table. + * + * When the source is partitioned, Postgres clones the trigger onto every + * partition, so changing it touches the parent and all of its partitions. Every + * operation here locks the parent first. A statement already running finishes + * with the locks it holds, and no new statement can take the parent and then + * wait on a partition, which is the cycle that makes a bare `DROP TRIGGER` on a + * busy partitioned parent deadlock against live traffic. */ export class Mirroring { readonly #source: Table; @@ -25,7 +32,9 @@ export class Mirroring { } /** - * Enables mirroring by creating a trigger function and trigger on the source table. + * Enables mirroring by creating (or refreshing) the trigger function and + * creating the trigger on the source table. A trigger that exists but was + * disabled is re-enabled in place rather than dropped and recreated. */ async enable( tx: DatabaseTransactionConnection, @@ -41,18 +50,78 @@ export class Mirroring { ); await tx.query(sql.typeAlias("void")`${functionSql}`); - await tx.query(sql.typeAlias("void")`${this.#dropTriggerSql}`); - await tx.query(sql.typeAlias("void")`${this.#createTriggerSql}`); + + if (await this.exists(tx)) { + await this.#setEnabled(tx, true); + } else { + await tx.query(sql.typeAlias("void")`${this.#createTriggerSql}`); + } } /** - * Disables mirroring by dropping the trigger and function. + * Disables mirroring by turning the trigger off. The trigger and its function + * stay in place, so mirroring can be re-enabled and the only lock taken is + * SHARE ROW EXCLUSIVE on each relation, parent first: reads are never blocked + * and writes wait only for the statement itself. */ async disable(tx: DatabaseTransactionConnection): Promise { + if (!(await this.exists(tx))) { + return; + } + + await this.#setEnabled(tx, false); + } + + /** + * Removes the trigger and its function. Locks the source first: `DROP + * TRIGGER` on a partitioned table otherwise locks the partitions before the + * parent and deadlocks against statements entering through the parent. + */ + async drop(tx: DatabaseTransactionConnection): Promise { + await tx.query( + sql.typeAlias("void")` + LOCK TABLE ${this.#source.sqlIdentifier} IN ACCESS EXCLUSIVE MODE + `, + ); await tx.query(sql.typeAlias("void")`${this.#dropTriggerSql}`); await tx.query(sql.typeAlias("void")`${this.#dropFunctionSql}`); } + /** + * Whether the mirroring trigger exists on the source table, enabled or not. + */ + async exists(connection: CommonQueryMethods): Promise { + return this.#source.triggerExists( + connection, + Mirroring.triggerNameFor(this.#source, this.#targetType), + ); + } + + /** + * Whether the mirroring trigger exists on the source table and fires. + */ + async enabled(connection: CommonQueryMethods): Promise { + return this.#source.triggerEnabled( + connection, + Mirroring.triggerNameFor(this.#source, this.#targetType), + ); + } + + async #setEnabled( + tx: DatabaseTransactionConnection, + enabled: boolean, + ): Promise { + const action = enabled ? sql.fragment`ENABLE` : sql.fragment`DISABLE`; + + // ALTER TABLE ... ENABLE/DISABLE TRIGGER on a partitioned table recurses + // to the cloned triggers on its partitions. + await tx.query( + sql.typeAlias("void")` + ALTER TABLE ${this.#source.sqlIdentifier} ${action} TRIGGER ${this.#triggerName} + `, + ); + } + get #functionName() { const suffix = this.#targetType === "intermediate" diff --git a/src/pgslice.test.ts b/src/pgslice.test.ts index 9002755..d87268b 100644 --- a/src/pgslice.test.ts +++ b/src/pgslice.test.ts @@ -680,7 +680,9 @@ describe("Pgslice.status", () => { intermediateExists: false, partitionCount: 0, mirrorTriggerExists: false, + mirrorTriggerEnabled: false, retiredMirrorTriggerExists: false, + retiredMirrorTriggerEnabled: false, originalIsPartitioned: false, }); }); @@ -706,7 +708,9 @@ describe("Pgslice.status", () => { partitionCount: 0, originalIsPartitioned: false, mirrorTriggerExists: false, + mirrorTriggerEnabled: false, retiredMirrorTriggerExists: false, + retiredMirrorTriggerEnabled: false, }); }); }); diff --git a/src/pgslice.ts b/src/pgslice.ts index 2139d4e..478a670 100644 --- a/src/pgslice.ts +++ b/src/pgslice.ts @@ -124,6 +124,22 @@ export class Pgslice { return this.pool.connect(handler); } + /** + * Bounds how long DDL in this transaction may wait for a table lock, so a + * contended run fails fast and is retried instead of queueing live traffic + * behind it. Uses the same default as `swap`. + */ + async #setLockTimeout( + tx: DatabaseTransactionConnection, + lockTimeout: string | undefined, + ): Promise { + await tx.query( + sql.typeAlias( + "void", + )`SET LOCAL lock_timeout = ${sql.literalValue(lockTimeout ?? "5s")}`, + ); + } + async #withLock( tx: CommonQueryMethods, table: Table, @@ -803,6 +819,7 @@ export class Pgslice { throw new Error(`Table not found: ${target.toString()}`); } + await this.#setLockTimeout(tx, options.lockTimeout); await new Mirroring({ source: table, targetType }).enable(tx, target); }), ); @@ -826,7 +843,13 @@ export class Pgslice { throw new Error(`Table not found: ${table.toString()}`); } - await new Mirroring({ source: table, targetType }).disable(tx); + await this.#setLockTimeout(tx, options.lockTimeout); + const mirroring = new Mirroring({ source: table, targetType }); + if (options.drop) { + await mirroring.drop(tx); + } else { + await mirroring.disable(tx); + } }), ); } @@ -1014,14 +1037,20 @@ export class Pgslice { partitionCount = partitions.length; } - const mirrorTriggerExists = await table.triggerExists( - this.pool, - Mirroring.triggerNameFor(table, "intermediate"), - ); + const mirroring = new Mirroring({ + source: table, + targetType: "intermediate", + }); + const retiredMirroring = new Mirroring({ + source: table, + targetType: "retired", + }); - const retiredMirrorTriggerExists = await table.triggerExists( + const mirrorTriggerExists = await mirroring.exists(this.pool); + const mirrorTriggerEnabled = await mirroring.enabled(this.pool); + const retiredMirrorTriggerExists = await retiredMirroring.exists(this.pool); + const retiredMirrorTriggerEnabled = await retiredMirroring.enabled( this.pool, - Mirroring.triggerNameFor(table, "retired"), ); const originalIsPartitioned = await table.isPartitioned(this.pool); @@ -1030,7 +1059,9 @@ export class Pgslice { intermediateExists, partitionCount, mirrorTriggerExists, + mirrorTriggerEnabled, retiredMirrorTriggerExists, + retiredMirrorTriggerEnabled, originalIsPartitioned, }; } diff --git a/src/swapper.ts b/src/swapper.ts index a8e4776..b3f7aa6 100644 --- a/src/swapper.ts +++ b/src/swapper.ts @@ -114,7 +114,7 @@ export class Swapper { const targetType = this.#direction === "forward" ? "intermediate" : "retired"; - await new Mirroring({ source: this.#table, targetType }).disable(tx); + await new Mirroring({ source: this.#table, targetType }).drop(tx); } /** diff --git a/src/table.ts b/src/table.ts index 5f16d3b..2eafd42 100644 --- a/src/table.ts +++ b/src/table.ts @@ -260,6 +260,29 @@ export class Table { return result.count > 0; } + /** + * Checks if a trigger with the given name exists on this table and fires. + * A trigger turned off with ALTER TABLE ... DISABLE TRIGGER still exists but + * does not fire. + */ + async triggerEnabled( + connection: CommonQueryMethods, + triggerName: string, + ): Promise { + const result = await connection.one( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*) FROM pg_trigger t + JOIN pg_class c ON t.tgrelid = c.oid + JOIN pg_namespace n ON c.relnamespace = n.oid + WHERE n.nspname = ${this.schema} + AND c.relname = ${this.name} + AND t.tgname = ${triggerName} + AND t.tgenabled <> 'D' + `, + ); + return result.count > 0; + } + /** * Gets column metadata for this table (excluding generated columns). */ diff --git a/src/types.ts b/src/types.ts index 4f2bf0d..81fed59 100644 --- a/src/types.ts +++ b/src/types.ts @@ -97,6 +97,8 @@ export type { MirroringTargetType }; export interface EnableMirroringOptions { table: string; targetType?: MirroringTargetType; + /** Bound on how long to wait for the table lock (default: 5s). */ + lockTimeout?: string; } /** @@ -105,6 +107,14 @@ export interface EnableMirroringOptions { export interface DisableMirroringOptions { table: string; targetType?: MirroringTargetType; + /** Bound on how long to wait for the table lock (default: 5s). */ + lockTimeout?: string; + /** + * Remove the trigger and its function instead of turning the trigger off. + * Takes an ACCESS EXCLUSIVE lock on the source table for the duration of the + * drop; the default only disables the trigger under a lighter lock. + */ + drop?: boolean; } /** @@ -228,7 +238,11 @@ export interface TableStatus { intermediateExists: boolean; partitionCount: number; mirrorTriggerExists: boolean; + /** The mirror trigger exists and fires; false when it exists but is disabled. */ + mirrorTriggerEnabled: boolean; retiredMirrorTriggerExists: boolean; + /** The retired mirror trigger exists and fires; false when disabled. */ + retiredMirrorTriggerEnabled: boolean; originalIsPartitioned: boolean; } From 5dd1389799f7bfdf0e2d553a52cc70d7c6b310b0 Mon Sep 17 00:00:00 2001 From: Stacy Curry Date: Mon, 12 Oct 2026 10:03:01 -0500 Subject: [PATCH 2/6] Show trigger enabled state in plain status and pin the drop's lock order in tests Plain `status` now prints whether each mirroring trigger is enabled, so a disabled trigger is distinguishable from an active one without --json. Status tests walk enable, disable, re-enable and drop for both targets, and the drop test asserts LOCK TABLE on the parent is issued before DROP TRIGGER. Co-Authored-By: Claude Fable 5.1 --- src/commands/status.test.ts | 31 ++++++++++++++++ src/commands/status.ts | 6 ++++ src/mirroring.test.ts | 37 +++++++++++++++++++ src/pgslice.test.ts | 71 +++++++++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+) diff --git a/src/commands/status.test.ts b/src/commands/status.test.ts index 477d7c5..00cb762 100644 --- a/src/commands/status.test.ts +++ b/src/commands/status.test.ts @@ -43,5 +43,36 @@ describe("StatusCommand", () => { expect(output).toContain("Table: posts"); expect(output).toContain("Intermediate exists:"); expect(output).toContain("Original is partitioned:"); + expect(output).toContain("Mirror trigger enabled: false"); + expect(output).toContain("Retired mirror trigger enabled: false"); + }); + + test("distinguishes a disabled trigger from an active one", async ({ + cli, + commandContext, + pgslice, + transaction, + }) => { + await transaction.query(sql.unsafe` + CREATE TABLE posts_retired ( + id SERIAL PRIMARY KEY, + created_at DATE NOT NULL + ) + `); + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + const exitCode = await cli.run(["status", "posts"], commandContext); + + expect(exitCode).toBe(0); + const output = commandContext.stdout.read()?.toString(); + expect(output).toContain("Retired mirror trigger exists: true"); + expect(output).toContain("Retired mirror trigger enabled: false"); }); }); diff --git a/src/commands/status.ts b/src/commands/status.ts index 1fd0162..054418e 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -45,9 +45,15 @@ export class StatusCommand extends BaseCommand { this.context.stdout.write( `Mirror trigger exists: ${status.mirrorTriggerExists}\n`, ); + this.context.stdout.write( + `Mirror trigger enabled: ${status.mirrorTriggerEnabled}\n`, + ); this.context.stdout.write( `Retired mirror trigger exists: ${status.retiredMirrorTriggerExists}\n`, ); + this.context.stdout.write( + `Retired mirror trigger enabled: ${status.retiredMirrorTriggerEnabled}\n`, + ); } } } diff --git a/src/mirroring.test.ts b/src/mirroring.test.ts index 218c736..52a1d40 100644 --- a/src/mirroring.test.ts +++ b/src/mirroring.test.ts @@ -3,6 +3,8 @@ import { DatabaseTransactionConnection, sql } from "slonik"; import { z } from "zod"; import { pgsliceTest as test } from "./testing/index.js"; +import { Mirroring } from "./mirroring.js"; +import { Table } from "./table.js"; describe("Pgslice.enableMirroring", () => { test.beforeEach(async ({ transaction }) => { @@ -648,6 +650,41 @@ describe("Pgslice.disableMirroring (disable vs drop)", () => { expect(disabled).toBe(0); }); + test("drop locks the parent before it drops anything", async ({ + transaction, + }) => { + const issued: string[] = []; + const recording: DatabaseTransactionConnection = { + ...transaction, + async query(query) { + issued.push(query.sql.replace(/\s+/g, " ").trim()); + return transaction.query(query); + }, + }; + + await new Mirroring({ + source: Table.parse("posts"), + targetType: "retired", + }).drop(recording); + + expect(issued[0]).toMatch( + /^LOCK TABLE "public"\."posts" IN ACCESS EXCLUSIVE MODE$/, + ); + expect(issued[1]).toMatch(/^DROP TRIGGER IF EXISTS/); + expect(issued[2]).toMatch(/^DROP FUNCTION IF EXISTS/); + + const held = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM pg_locks + WHERE relation = 'posts'::regclass + AND pid = pg_backend_pid() + AND mode = 'AccessExclusiveLock' + AND granted + `, + ); + expect(held).toBe(1); + }); + test("drop removes the trigger from the parent and every partition", async ({ pgslice, transaction, diff --git a/src/pgslice.test.ts b/src/pgslice.test.ts index d87268b..215579d 100644 --- a/src/pgslice.test.ts +++ b/src/pgslice.test.ts @@ -715,6 +715,77 @@ describe("Pgslice.status", () => { }); }); +describe("Pgslice.status mirroring flags", () => { + test.beforeEach(async ({ transaction }) => { + await transaction.query(sql.unsafe` + CREATE TABLE posts ( + id SERIAL PRIMARY KEY, + created_at DATE NOT NULL + ) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_intermediate ( + id SERIAL PRIMARY KEY, + created_at DATE NOT NULL + ) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_retired ( + id SERIAL PRIMARY KEY, + created_at DATE NOT NULL + ) + `); + }); + + for (const targetType of ["intermediate", "retired"] as const) { + const exists = + targetType === "intermediate" + ? "mirrorTriggerExists" + : "retiredMirrorTriggerExists"; + const enabled = + targetType === "intermediate" + ? "mirrorTriggerEnabled" + : "retiredMirrorTriggerEnabled"; + + test(`tracks the ${targetType} trigger through enable, disable, re-enable and drop`, async ({ + pgslice, + transaction, + }) => { + const flags = async () => { + const status = await pgslice.status({ table: "posts" }); + return { exists: status[exists], enabled: status[enabled] }; + }; + + expect(await flags()).toEqual({ exists: false, enabled: false }); + + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType, + }); + expect(await flags()).toEqual({ exists: true, enabled: true }); + + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType, + }); + expect(await flags()).toEqual({ exists: true, enabled: false }); + + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType, + }); + expect(await flags()).toEqual({ exists: true, enabled: true }); + + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType, + drop: true, + }); + expect(await flags()).toEqual({ exists: false, enabled: false }); + }); + } +}); + describe("Pgslice.synchronize", () => { test("synchronizes data", async ({ pgslice, transaction }) => { // Create source table with data From bc20425496c84e0201ecb47efcf5abd265d243cf Mon Sep 17 00:00:00 2001 From: Stacy Curry Date: Mon, 12 Oct 2026 10:48:48 -0500 Subject: [PATCH 3/6] fix: Drop mirroring triggers under an explicit parent lock instead of adding a disable mode Remove the disable-vs-drop split, the trigger enabled state in status, and the --drop flag. The deadlock fix is the lock order alone: lock the source table before DROP TRIGGER so the walk over the partitions can never run against a statement that already holds the parent. Keep --lock-timeout on the enable and disable mirroring commands. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 3 +- README.md | 4 +- src/commands/disable-mirroring.test.ts | 50 -- src/commands/disable-mirroring.ts | 25 +- .../disable-retired-mirroring.test.ts | 56 --- src/commands/disable-retired-mirroring.ts | 26 +- src/commands/status.test.ts | 33 -- src/commands/status.ts | 6 - src/mirroring.test.ts | 429 +++++------------- src/mirroring.ts | 77 +--- src/pgslice.test.ts | 75 --- src/pgslice.ts | 27 +- src/swapper.ts | 2 +- src/table.ts | 23 - src/types.ts | 10 - 15 files changed, 139 insertions(+), 707 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 920152e..89a7f4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,7 @@ ## Unreleased -- Changed `disable_mirroring` and `disable_retired_mirroring` to turn the trigger off with `ALTER TABLE ... DISABLE TRIGGER` instead of dropping it. On a partitioned table the drop locked every partition before the parent and deadlocked against live traffic; disabling locks the parent first, never blocks reads, and leaves the trigger and function in place so `enable_mirroring` can turn them back on. Pass `--drop` to remove the trigger and function, which now takes an explicit ACCESS EXCLUSIVE lock on the table first. +- Fixed `disable_mirroring` and `disable_retired_mirroring` deadlocking against live traffic on a partitioned table. `DROP TRIGGER` on a partitioned parent locks every partition before the parent while statements lock the parent first; the source table is now locked explicitly before the trigger is dropped. - Added `--lock-timeout` (default `5s`) to the enable and disable mirroring commands, matching `swap`. -- Added `mirrorTriggerEnabled` and `retiredMirrorTriggerEnabled` to `status`, so a disabled trigger is distinguishable from an active one. - Added support for managing existing (already-partitioned) tables: bounds-anchored extension that recognizes legacy-named partitions and extends contiguously without renaming, gaps, or overlaps - Added a weekly (ISO-week, Monday-aligned) period - Added composite / parent-owned primary key support to `add_partitions` (skips the redundant per-partition key when the parent owns one) diff --git a/README.md b/README.md index a6628a8..4a38e3a 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ pgslice disable_mirroring
After the swap, the original mirroring triggers are no longer needed since the tables have been swapped. -This turns the trigger off rather than dropping it, which never blocks reads and cannot deadlock against live traffic on a partitioned table. Pass `--drop` to remove the trigger and its function; that takes an ACCESS EXCLUSIVE lock on the table, acquired before anything else so it is safe on a partitioned table too. Both forms honour `--lock-timeout` (default: `5s`). +The table is locked before the trigger is dropped, so this is safe to run against a busy partitioned table. The lock wait is bounded by `--lock-timeout` (default: `5s`). 11. Enable Reverse Mirroring (now-partitioned table to retired table) @@ -170,7 +170,7 @@ pgslice fill
--swapped pgslice disable_retired_mirroring
``` -Once you're confident the retired table is no longer needed, disable the retired mirroring triggers. This turns the trigger off under a light lock, parent first, so it is safe to run against a busy partitioned table; `status` then reports the trigger as present but disabled. When you drop the retired table, run it again with `--drop` to remove the trigger and its function. Both forms honour `--lock-timeout` (default: `5s`). +Once you're confident the retired table is no longer needed and you're ready to drop it, disable the retired mirroring triggers. The table is locked before the trigger is dropped, so this is safe to run against a busy partitioned table. The lock wait is bounded by `--lock-timeout` (default: `5s`). 14. Back up the retired table with a tool like [pg_dump](https://www.postgresql.org/docs/current/static/app-pgdump.html) and drop it diff --git a/src/commands/disable-mirroring.test.ts b/src/commands/disable-mirroring.test.ts index ca658e1..df07948 100644 --- a/src/commands/disable-mirroring.test.ts +++ b/src/commands/disable-mirroring.test.ts @@ -1,6 +1,5 @@ import { describe, expect } from "vitest"; import { sql } from "slonik"; -import { z } from "zod"; import { commandTest as test } from "../testing/index.js"; import { DisableMirroringCommand } from "./disable-mirroring.js"; @@ -77,55 +76,6 @@ describe("DisableMirroringCommand", () => { const output = commandContext.stdout.read()?.toString(); expect(output).toContain("Mirroring triggers disabled for posts"); }); - test("keeps the trigger in place but disabled by default", async ({ - cli, - commandContext, - pgslice, - transaction, - }) => { - await pgslice.enableMirroring(transaction, { table: "posts" }); - - const exitCode = await cli.run( - ["mirroring", "disable", "posts"], - commandContext, - ); - - expect(exitCode).toBe(0); - const trigger = await transaction.one( - sql.type(z.object({ tgenabled: z.string() }))` - SELECT tgenabled FROM pg_trigger WHERE tgname = 'posts_mirror_trigger' - `, - ); - expect(trigger.tgenabled).toBe("D"); - }); - - test("drops the trigger and function with --drop", async ({ - cli, - commandContext, - pgslice, - transaction, - }) => { - await pgslice.enableMirroring(transaction, { table: "posts" }); - - const exitCode = await cli.run( - ["mirroring", "disable", "posts", "--drop"], - commandContext, - ); - - expect(exitCode).toBe(0); - const output = commandContext.stdout.read()?.toString(); - expect(output).toContain("Mirroring triggers dropped for posts"); - - const remaining = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT ( - (SELECT COUNT(*) FROM pg_trigger WHERE tgname = 'posts_mirror_trigger') - + (SELECT COUNT(*) FROM pg_proc WHERE proname = 'posts_mirror_to_intermediate') - )::int AS count - `, - ); - expect(remaining).toBe(0); - }); }); describe("schema-qualified tables", () => { diff --git a/src/commands/disable-mirroring.ts b/src/commands/disable-mirroring.ts index ba1766b..99c0560 100644 --- a/src/commands/disable-mirroring.ts +++ b/src/commands/disable-mirroring.ts @@ -9,15 +9,12 @@ export class DisableMirroringCommand extends BaseCommand { static override usage = Command.Usage({ description: "Disable mirroring triggers after partitioning is complete", details: ` - This command turns off the trigger on the source table that mirrors - operations to the intermediate table. Use this after partitioning is - complete. + This command removes the triggers on the source table that mirror operations + to the intermediate table. Use this after partitioning is complete. - The trigger is disabled, not dropped: on a partitioned table that takes a - light lock on the parent first and never blocks reads. Pass --drop to - remove the trigger and its function instead, which takes an ACCESS - EXCLUSIVE lock on the table for the duration of the drop. Either way the - lock wait is bounded by --lock-timeout. + The table is locked before the trigger is dropped, so this is safe on a + partitioned table under live traffic. The lock wait is bounded by + --lock-timeout; a timeout aborts cleanly and can be retried. `, examples: [ ["Disable mirroring for a table", "$0 mirroring disable posts"], @@ -25,10 +22,6 @@ export class DisableMirroringCommand extends BaseCommand { "Disable mirroring with explicit schema", "$0 mirroring disable myschema.posts", ], - [ - "Remove the trigger and function entirely", - "$0 mirroring disable posts --drop", - ], [ "With custom lock timeout", "$0 mirroring disable posts --lock-timeout 10s", @@ -40,21 +33,15 @@ export class DisableMirroringCommand extends BaseCommand { lockTimeout = Option.String("--lock-timeout", "5s", { description: "Bound on how long to wait for the table lock", }); - drop = Option.Boolean("--drop", false, { - description: "Drop the trigger and its function instead of disabling", - }); override async perform(pgslice: Pgslice): Promise { await pgslice.start(async (tx) => { await pgslice.disableMirroring(tx, { table: this.table, lockTimeout: this.lockTimeout, - drop: this.drop, }); this.context.stdout.write( - this.drop - ? `Mirroring triggers dropped for ${this.table}\n` - : `Mirroring triggers disabled for ${this.table}\n`, + `Mirroring triggers disabled for ${this.table}\n`, ); }); } diff --git a/src/commands/disable-retired-mirroring.test.ts b/src/commands/disable-retired-mirroring.test.ts index 28b5d3c..527f7fd 100644 --- a/src/commands/disable-retired-mirroring.test.ts +++ b/src/commands/disable-retired-mirroring.test.ts @@ -1,6 +1,5 @@ import { describe, expect } from "vitest"; import { sql } from "slonik"; -import { z } from "zod"; import { commandTest as test } from "../testing/index.js"; import { DisableRetiredMirroringCommand } from "./disable-retired-mirroring.js"; @@ -85,61 +84,6 @@ describe("DisableRetiredMirroringCommand", () => { const output = commandContext.stdout.read()?.toString(); expect(output).toContain("Retired mirroring triggers disabled for posts"); }); - test("keeps the trigger in place but disabled by default", async ({ - cli, - commandContext, - pgslice, - transaction, - }) => { - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - const exitCode = await cli.run( - ["mirroring", "disable-retired", "posts"], - commandContext, - ); - - expect(exitCode).toBe(0); - const trigger = await transaction.one( - sql.type(z.object({ tgenabled: z.string() }))` - SELECT tgenabled FROM pg_trigger WHERE tgname = 'posts_retired_mirror_trigger' - `, - ); - expect(trigger.tgenabled).toBe("D"); - }); - - test("drops the trigger and function with --drop", async ({ - cli, - commandContext, - pgslice, - transaction, - }) => { - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - const exitCode = await cli.run( - ["mirroring", "disable-retired", "posts", "--drop"], - commandContext, - ); - - expect(exitCode).toBe(0); - const output = commandContext.stdout.read()?.toString(); - expect(output).toContain("Retired mirroring triggers dropped for posts"); - - const remaining = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT ( - (SELECT COUNT(*) FROM pg_trigger WHERE tgname = 'posts_retired_mirror_trigger') - + (SELECT COUNT(*) FROM pg_proc WHERE proname = 'posts_mirror_to_retired') - )::int AS count - `, - ); - expect(remaining).toBe(0); - }); }); describe("schema-qualified tables", () => { diff --git a/src/commands/disable-retired-mirroring.ts b/src/commands/disable-retired-mirroring.ts index b251c56..cacd465 100644 --- a/src/commands/disable-retired-mirroring.ts +++ b/src/commands/disable-retired-mirroring.ts @@ -12,15 +12,13 @@ export class DisableRetiredMirroringCommand extends BaseCommand { static override usage = Command.Usage({ description: "Disable retired mirroring triggers", details: ` - This command turns off the trigger on the source table that mirrors - operations to the retired table. Use this when you no longer need to keep - the retired table in sync with the main table. + This command removes the triggers on the source table that mirror operations + to the retired table. Use this when you no longer need to keep the retired + table in sync with the main table. - The trigger is disabled, not dropped: on a partitioned table that takes a - light lock on the parent first and never blocks reads. Pass --drop to - remove the trigger and its function instead, which takes an ACCESS - EXCLUSIVE lock on the table for the duration of the drop. Either way the - lock wait is bounded by --lock-timeout. + The table is locked before the trigger is dropped, so this is safe on a + partitioned table under live traffic. The lock wait is bounded by + --lock-timeout; a timeout aborts cleanly and can be retried. `, examples: [ [ @@ -31,10 +29,6 @@ export class DisableRetiredMirroringCommand extends BaseCommand { "Disable retired mirroring with explicit schema", "$0 mirroring disable-retired myschema.posts", ], - [ - "Remove the trigger and function entirely", - "$0 mirroring disable-retired posts --drop", - ], [ "With custom lock timeout", "$0 mirroring disable-retired posts --lock-timeout 10s", @@ -46,9 +40,6 @@ export class DisableRetiredMirroringCommand extends BaseCommand { lockTimeout = Option.String("--lock-timeout", "5s", { description: "Bound on how long to wait for the table lock", }); - drop = Option.Boolean("--drop", false, { - description: "Drop the trigger and its function instead of disabling", - }); override async perform(pgslice: Pgslice): Promise { await pgslice.start(async (tx) => { @@ -56,12 +47,9 @@ export class DisableRetiredMirroringCommand extends BaseCommand { table: this.table, targetType: "retired", lockTimeout: this.lockTimeout, - drop: this.drop, }); this.context.stdout.write( - this.drop - ? `Retired mirroring triggers dropped for ${this.table}\n` - : `Retired mirroring triggers disabled for ${this.table}\n`, + `Retired mirroring triggers disabled for ${this.table}\n`, ); }); } diff --git a/src/commands/status.test.ts b/src/commands/status.test.ts index 00cb762..47dd460 100644 --- a/src/commands/status.test.ts +++ b/src/commands/status.test.ts @@ -28,9 +28,7 @@ describe("StatusCommand", () => { intermediateExists: false, partitionCount: 0, mirrorTriggerExists: false, - mirrorTriggerEnabled: false, retiredMirrorTriggerExists: false, - retiredMirrorTriggerEnabled: false, originalIsPartitioned: false, }); }); @@ -43,36 +41,5 @@ describe("StatusCommand", () => { expect(output).toContain("Table: posts"); expect(output).toContain("Intermediate exists:"); expect(output).toContain("Original is partitioned:"); - expect(output).toContain("Mirror trigger enabled: false"); - expect(output).toContain("Retired mirror trigger enabled: false"); - }); - - test("distinguishes a disabled trigger from an active one", async ({ - cli, - commandContext, - pgslice, - transaction, - }) => { - await transaction.query(sql.unsafe` - CREATE TABLE posts_retired ( - id SERIAL PRIMARY KEY, - created_at DATE NOT NULL - ) - `); - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - const exitCode = await cli.run(["status", "posts"], commandContext); - - expect(exitCode).toBe(0); - const output = commandContext.stdout.read()?.toString(); - expect(output).toContain("Retired mirror trigger exists: true"); - expect(output).toContain("Retired mirror trigger enabled: false"); }); }); diff --git a/src/commands/status.ts b/src/commands/status.ts index 054418e..1fd0162 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -45,15 +45,9 @@ export class StatusCommand extends BaseCommand { this.context.stdout.write( `Mirror trigger exists: ${status.mirrorTriggerExists}\n`, ); - this.context.stdout.write( - `Mirror trigger enabled: ${status.mirrorTriggerEnabled}\n`, - ); this.context.stdout.write( `Retired mirror trigger exists: ${status.retiredMirrorTriggerExists}\n`, ); - this.context.stdout.write( - `Retired mirror trigger enabled: ${status.retiredMirrorTriggerEnabled}\n`, - ); } } } diff --git a/src/mirroring.test.ts b/src/mirroring.test.ts index 52a1d40..c8bbbc3 100644 --- a/src/mirroring.test.ts +++ b/src/mirroring.test.ts @@ -247,12 +247,9 @@ describe("Pgslice.disableMirroring", () => { `); }); - test("drops trigger function with drop", async ({ pgslice, transaction }) => { + test("drops trigger function", async ({ pgslice, transaction }) => { await pgslice.enableMirroring(transaction, { table: "posts" }); - await pgslice.disableMirroring(transaction, { - table: "posts", - drop: true, - }); + await pgslice.disableMirroring(transaction, { table: "posts" }); const count = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` @@ -263,12 +260,9 @@ describe("Pgslice.disableMirroring", () => { expect(count).toBe(0); }); - test("drops trigger with drop", async ({ pgslice, transaction }) => { + test("drops trigger", async ({ pgslice, transaction }) => { await pgslice.enableMirroring(transaction, { table: "posts" }); - await pgslice.disableMirroring(transaction, { - table: "posts", - drop: true, - }); + await pgslice.disableMirroring(transaction, { table: "posts" }); const count = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` @@ -344,10 +338,7 @@ describe("Pgslice.disableMirroring", () => { `); await pgslice.enableMirroring(transaction, { table: "myschema.posts" }); - await pgslice.disableMirroring(transaction, { - table: "myschema.posts", - drop: true, - }); + await pgslice.disableMirroring(transaction, { table: "myschema.posts" }); const count = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` @@ -370,7 +361,7 @@ describe("Pgslice.disableMirroring", () => { }); }); -describe("Pgslice.disableMirroring (disable vs drop)", () => { +describe("Pgslice.disableMirroring (partitioned table)", () => { const triggerCount = async ( transaction: DatabaseTransactionConnection, name: string, @@ -381,326 +372,116 @@ describe("Pgslice.disableMirroring (disable vs drop)", () => { `, ); - const enabledTriggerCount = async ( - transaction: DatabaseTransactionConnection, - name: string, - ) => - transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM pg_trigger - WHERE tgname = ${name} AND tgenabled <> 'D' - `, + test.beforeEach(async ({ pgslice, transaction }) => { + await transaction.query(sql.unsafe` + CREATE TABLE posts ( + id SERIAL, + title TEXT NOT NULL, + created_at DATE NOT NULL + ) PARTITION BY RANGE (created_at) + `); + await transaction.query(sql.unsafe` + CREATE UNIQUE INDEX posts_replica_identity_uidx ON posts (id, created_at) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_202401 PARTITION OF posts + FOR VALUES FROM ('2024-01-01') TO ('2024-02-01') + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_202402 PARTITION OF posts + FOR VALUES FROM ('2024-02-01') TO ('2024-03-01') + `); + await transaction.query(sql.unsafe` + ALTER TABLE posts_202401 ADD PRIMARY KEY (id) + `); + await transaction.query(sql.unsafe` + ALTER TABLE posts_202402 ADD PRIMARY KEY (id) + `); + await transaction.query(sql.unsafe` + CREATE TABLE posts_retired ( + id INTEGER PRIMARY KEY, + title TEXT NOT NULL, + created_at DATE NOT NULL + ) + `); + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + }); + + test("clones the trigger onto every partition", async ({ transaction }) => { + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(3); + }); + + test("locks the parent before it drops anything", async ({ transaction }) => { + const issued: string[] = []; + const recording: DatabaseTransactionConnection = { + ...transaction, + async query(query) { + issued.push(query.sql.replace(/\s+/g, " ").trim()); + return transaction.query(query); + }, + }; + + await new Mirroring({ + source: Table.parse("posts"), + targetType: "retired", + }).disable(recording); + + expect(issued[0]).toMatch( + /^LOCK TABLE "public"\."posts" IN ACCESS EXCLUSIVE MODE$/, ); + expect(issued[1]).toMatch(/^DROP TRIGGER IF EXISTS/); + expect(issued[2]).toMatch(/^DROP FUNCTION IF EXISTS/); - const functionCount = async ( - transaction: DatabaseTransactionConnection, - name: string, - ) => - transaction.oneFirst( + const held = await transaction.oneFirst( sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM pg_proc WHERE proname = ${name} + SELECT COUNT(*)::int FROM pg_locks + WHERE relation = 'posts'::regclass + AND pid = pg_backend_pid() + AND mode = 'AccessExclusiveLock' + AND granted `, ); - - describe("plain table", () => { - test.beforeEach(async ({ pgslice, transaction }) => { - await transaction.query(sql.unsafe` - CREATE TABLE posts ( - id SERIAL PRIMARY KEY, - title TEXT NOT NULL, - created_at DATE NOT NULL - ) - `); - await transaction.query(sql.unsafe` - CREATE TABLE posts_retired ( - id SERIAL PRIMARY KEY, - title TEXT NOT NULL, - created_at DATE NOT NULL - ) - `); - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - }); - - test("turns the trigger off but keeps the trigger and its function", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - expect( - await triggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(1); - expect( - await enabledTriggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(0); - expect(await functionCount(transaction, "posts_mirror_to_retired")).toBe( - 1, - ); - }); - - test("stops mirroring writes once disabled", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - await transaction.query(sql.unsafe` - INSERT INTO posts (title, created_at) VALUES ('Hello', '2024-01-15') - `); - - const count = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM posts_retired - `, - ); - expect(count).toBe(0); - }); - - test("enableMirroring turns a disabled trigger back on in place", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - expect( - await triggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(1); - expect( - await enabledTriggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(1); - - await transaction.query(sql.unsafe` - INSERT INTO posts (title, created_at) VALUES ('Hello', '2024-01-15') - `); - const count = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM posts_retired - `, - ); - expect(count).toBe(1); - }); - - test("drop removes the trigger and its function", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - drop: true, - }); - - expect( - await triggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(0); - expect(await functionCount(transaction, "posts_mirror_to_retired")).toBe( - 0, - ); - }); - - test("is idempotent when the trigger is already gone", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - drop: true, - }); - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - drop: true, - }); - - expect( - await triggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(0); - }); - - test("bounds the lock wait with the requested lock_timeout", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - lockTimeout: "7s", - }); - - const lockTimeout = await transaction.oneFirst( - sql.type(z.object({ lock_timeout: z.string() }))`SHOW lock_timeout`, - ); - expect(lockTimeout).toBe("7s"); - }); + expect(held).toBe(1); }); - describe("partitioned table", () => { - test.beforeEach(async ({ pgslice, transaction }) => { - await transaction.query(sql.unsafe` - CREATE TABLE posts ( - id SERIAL, - title TEXT NOT NULL, - created_at DATE NOT NULL - ) PARTITION BY RANGE (created_at) - `); - await transaction.query(sql.unsafe` - CREATE UNIQUE INDEX posts_replica_identity_uidx ON posts (id, created_at) - `); - await transaction.query(sql.unsafe` - CREATE TABLE posts_202401 PARTITION OF posts - FOR VALUES FROM ('2024-01-01') TO ('2024-02-01') - `); - await transaction.query(sql.unsafe` - CREATE TABLE posts_202402 PARTITION OF posts - FOR VALUES FROM ('2024-02-01') TO ('2024-03-01') - `); - await transaction.query(sql.unsafe` - ALTER TABLE posts_202401 ADD PRIMARY KEY (id) - `); - await transaction.query(sql.unsafe` - ALTER TABLE posts_202402 ADD PRIMARY KEY (id) - `); - await transaction.query(sql.unsafe` - CREATE TABLE posts_retired ( - id INTEGER PRIMARY KEY, - title TEXT NOT NULL, - created_at DATE NOT NULL - ) - `); - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - }); - - test("disables the cloned trigger on every partition", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - const rows = await transaction.any( - sql.type(z.object({ relname: z.string(), tgenabled: z.string() }))` - SELECT c.relname, t.tgenabled - FROM pg_trigger t JOIN pg_class c ON c.oid = t.tgrelid - WHERE t.tgname = 'posts_retired_mirror_trigger' - ORDER BY c.relname - `, - ); - expect(rows).toEqual([ - { relname: "posts", tgenabled: "D" }, - { relname: "posts_202401", tgenabled: "D" }, - { relname: "posts_202402", tgenabled: "D" }, - ]); - - await transaction.query(sql.unsafe` - INSERT INTO posts (title, created_at) VALUES ('Hello', '2024-02-15') - `); - const count = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM posts_retired - `, - ); - expect(count).toBe(0); - }); - - test("re-enables the cloned trigger on every partition", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType: "retired", - }); - - const disabled = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM pg_trigger - WHERE tgname = 'posts_retired_mirror_trigger' AND tgenabled = 'D' - `, - ); - expect(disabled).toBe(0); + test("removes the trigger from the parent and every partition", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", }); - test("drop locks the parent before it drops anything", async ({ - transaction, - }) => { - const issued: string[] = []; - const recording: DatabaseTransactionConnection = { - ...transaction, - async query(query) { - issued.push(query.sql.replace(/\s+/g, " ").trim()); - return transaction.query(query); - }, - }; - - await new Mirroring({ - source: Table.parse("posts"), - targetType: "retired", - }).drop(recording); - - expect(issued[0]).toMatch( - /^LOCK TABLE "public"\."posts" IN ACCESS EXCLUSIVE MODE$/, - ); - expect(issued[1]).toMatch(/^DROP TRIGGER IF EXISTS/); - expect(issued[2]).toMatch(/^DROP FUNCTION IF EXISTS/); + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(0); + const functions = await transaction.oneFirst( + sql.type(z.object({ count: z.coerce.number() }))` + SELECT COUNT(*)::int FROM pg_proc WHERE proname = 'posts_mirror_to_retired' + `, + ); + expect(functions).toBe(0); + }); - const held = await transaction.oneFirst( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*)::int FROM pg_locks - WHERE relation = 'posts'::regclass - AND pid = pg_backend_pid() - AND mode = 'AccessExclusiveLock' - AND granted - `, - ); - expect(held).toBe(1); + test("bounds the lock wait with the requested lock_timeout", async ({ + pgslice, + transaction, + }) => { + await pgslice.disableMirroring(transaction, { + table: "posts", + targetType: "retired", + lockTimeout: "7s", }); - test("drop removes the trigger from the parent and every partition", async ({ - pgslice, - transaction, - }) => { - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType: "retired", - drop: true, - }); - - expect( - await triggerCount(transaction, "posts_retired_mirror_trigger"), - ).toBe(0); - expect(await functionCount(transaction, "posts_mirror_to_retired")).toBe( - 0, - ); - }); + const lockTimeout = await transaction.oneFirst( + sql.type(z.object({ lock_timeout: z.string() }))`SHOW lock_timeout`, + ); + expect(lockTimeout).toBe("7s"); }); }); diff --git a/src/mirroring.ts b/src/mirroring.ts index 0223520..2e2bbe9 100644 --- a/src/mirroring.ts +++ b/src/mirroring.ts @@ -1,4 +1,4 @@ -import { CommonQueryMethods, DatabaseTransactionConnection } from "slonik"; +import { DatabaseTransactionConnection } from "slonik"; import { Table } from "./table.js"; import { sql } from "./sql-utils.js"; @@ -16,11 +16,11 @@ interface MirroringOptions { * table are automatically replicated to the target table. * * When the source is partitioned, Postgres clones the trigger onto every - * partition, so changing it touches the parent and all of its partitions. Every - * operation here locks the parent first. A statement already running finishes - * with the locks it holds, and no new statement can take the parent and then - * wait on a partition, which is the cycle that makes a bare `DROP TRIGGER` on a - * busy partitioned parent deadlock against live traffic. + * partition, and `DROP TRIGGER` removes the clones before the parent trigger, + * taking an ACCESS EXCLUSIVE lock on each partition before the parent. + * Statements enter through the parent and lock it first, so the two walk the + * same relations in opposite orders and deadlock under live traffic. Dropping + * therefore locks the parent explicitly before anything else. */ export class Mirroring { readonly #source: Table; @@ -32,9 +32,7 @@ export class Mirroring { } /** - * Enables mirroring by creating (or refreshing) the trigger function and - * creating the trigger on the source table. A trigger that exists but was - * disabled is re-enabled in place rather than dropped and recreated. + * Enables mirroring by creating a trigger function and trigger on the source table. */ async enable( tx: DatabaseTransactionConnection, @@ -50,34 +48,14 @@ export class Mirroring { ); await tx.query(sql.typeAlias("void")`${functionSql}`); - - if (await this.exists(tx)) { - await this.#setEnabled(tx, true); - } else { - await tx.query(sql.typeAlias("void")`${this.#createTriggerSql}`); - } + await tx.query(sql.typeAlias("void")`${this.#dropTriggerSql}`); + await tx.query(sql.typeAlias("void")`${this.#createTriggerSql}`); } /** - * Disables mirroring by turning the trigger off. The trigger and its function - * stay in place, so mirroring can be re-enabled and the only lock taken is - * SHARE ROW EXCLUSIVE on each relation, parent first: reads are never blocked - * and writes wait only for the statement itself. + * Disables mirroring by dropping the trigger and function. */ async disable(tx: DatabaseTransactionConnection): Promise { - if (!(await this.exists(tx))) { - return; - } - - await this.#setEnabled(tx, false); - } - - /** - * Removes the trigger and its function. Locks the source first: `DROP - * TRIGGER` on a partitioned table otherwise locks the partitions before the - * parent and deadlocks against statements entering through the parent. - */ - async drop(tx: DatabaseTransactionConnection): Promise { await tx.query( sql.typeAlias("void")` LOCK TABLE ${this.#source.sqlIdentifier} IN ACCESS EXCLUSIVE MODE @@ -87,41 +65,6 @@ export class Mirroring { await tx.query(sql.typeAlias("void")`${this.#dropFunctionSql}`); } - /** - * Whether the mirroring trigger exists on the source table, enabled or not. - */ - async exists(connection: CommonQueryMethods): Promise { - return this.#source.triggerExists( - connection, - Mirroring.triggerNameFor(this.#source, this.#targetType), - ); - } - - /** - * Whether the mirroring trigger exists on the source table and fires. - */ - async enabled(connection: CommonQueryMethods): Promise { - return this.#source.triggerEnabled( - connection, - Mirroring.triggerNameFor(this.#source, this.#targetType), - ); - } - - async #setEnabled( - tx: DatabaseTransactionConnection, - enabled: boolean, - ): Promise { - const action = enabled ? sql.fragment`ENABLE` : sql.fragment`DISABLE`; - - // ALTER TABLE ... ENABLE/DISABLE TRIGGER on a partitioned table recurses - // to the cloned triggers on its partitions. - await tx.query( - sql.typeAlias("void")` - ALTER TABLE ${this.#source.sqlIdentifier} ${action} TRIGGER ${this.#triggerName} - `, - ); - } - get #functionName() { const suffix = this.#targetType === "intermediate" diff --git a/src/pgslice.test.ts b/src/pgslice.test.ts index 215579d..9002755 100644 --- a/src/pgslice.test.ts +++ b/src/pgslice.test.ts @@ -680,9 +680,7 @@ describe("Pgslice.status", () => { intermediateExists: false, partitionCount: 0, mirrorTriggerExists: false, - mirrorTriggerEnabled: false, retiredMirrorTriggerExists: false, - retiredMirrorTriggerEnabled: false, originalIsPartitioned: false, }); }); @@ -708,84 +706,11 @@ describe("Pgslice.status", () => { partitionCount: 0, originalIsPartitioned: false, mirrorTriggerExists: false, - mirrorTriggerEnabled: false, retiredMirrorTriggerExists: false, - retiredMirrorTriggerEnabled: false, }); }); }); -describe("Pgslice.status mirroring flags", () => { - test.beforeEach(async ({ transaction }) => { - await transaction.query(sql.unsafe` - CREATE TABLE posts ( - id SERIAL PRIMARY KEY, - created_at DATE NOT NULL - ) - `); - await transaction.query(sql.unsafe` - CREATE TABLE posts_intermediate ( - id SERIAL PRIMARY KEY, - created_at DATE NOT NULL - ) - `); - await transaction.query(sql.unsafe` - CREATE TABLE posts_retired ( - id SERIAL PRIMARY KEY, - created_at DATE NOT NULL - ) - `); - }); - - for (const targetType of ["intermediate", "retired"] as const) { - const exists = - targetType === "intermediate" - ? "mirrorTriggerExists" - : "retiredMirrorTriggerExists"; - const enabled = - targetType === "intermediate" - ? "mirrorTriggerEnabled" - : "retiredMirrorTriggerEnabled"; - - test(`tracks the ${targetType} trigger through enable, disable, re-enable and drop`, async ({ - pgslice, - transaction, - }) => { - const flags = async () => { - const status = await pgslice.status({ table: "posts" }); - return { exists: status[exists], enabled: status[enabled] }; - }; - - expect(await flags()).toEqual({ exists: false, enabled: false }); - - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType, - }); - expect(await flags()).toEqual({ exists: true, enabled: true }); - - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType, - }); - expect(await flags()).toEqual({ exists: true, enabled: false }); - - await pgslice.enableMirroring(transaction, { - table: "posts", - targetType, - }); - expect(await flags()).toEqual({ exists: true, enabled: true }); - - await pgslice.disableMirroring(transaction, { - table: "posts", - targetType, - drop: true, - }); - expect(await flags()).toEqual({ exists: false, enabled: false }); - }); - } -}); - describe("Pgslice.synchronize", () => { test("synchronizes data", async ({ pgslice, transaction }) => { // Create source table with data diff --git a/src/pgslice.ts b/src/pgslice.ts index 478a670..6585e89 100644 --- a/src/pgslice.ts +++ b/src/pgslice.ts @@ -844,12 +844,7 @@ export class Pgslice { } await this.#setLockTimeout(tx, options.lockTimeout); - const mirroring = new Mirroring({ source: table, targetType }); - if (options.drop) { - await mirroring.drop(tx); - } else { - await mirroring.disable(tx); - } + await new Mirroring({ source: table, targetType }).disable(tx); }), ); } @@ -1037,20 +1032,14 @@ export class Pgslice { partitionCount = partitions.length; } - const mirroring = new Mirroring({ - source: table, - targetType: "intermediate", - }); - const retiredMirroring = new Mirroring({ - source: table, - targetType: "retired", - }); + const mirrorTriggerExists = await table.triggerExists( + this.pool, + Mirroring.triggerNameFor(table, "intermediate"), + ); - const mirrorTriggerExists = await mirroring.exists(this.pool); - const mirrorTriggerEnabled = await mirroring.enabled(this.pool); - const retiredMirrorTriggerExists = await retiredMirroring.exists(this.pool); - const retiredMirrorTriggerEnabled = await retiredMirroring.enabled( + const retiredMirrorTriggerExists = await table.triggerExists( this.pool, + Mirroring.triggerNameFor(table, "retired"), ); const originalIsPartitioned = await table.isPartitioned(this.pool); @@ -1059,9 +1048,7 @@ export class Pgslice { intermediateExists, partitionCount, mirrorTriggerExists, - mirrorTriggerEnabled, retiredMirrorTriggerExists, - retiredMirrorTriggerEnabled, originalIsPartitioned, }; } diff --git a/src/swapper.ts b/src/swapper.ts index b3f7aa6..a8e4776 100644 --- a/src/swapper.ts +++ b/src/swapper.ts @@ -114,7 +114,7 @@ export class Swapper { const targetType = this.#direction === "forward" ? "intermediate" : "retired"; - await new Mirroring({ source: this.#table, targetType }).drop(tx); + await new Mirroring({ source: this.#table, targetType }).disable(tx); } /** diff --git a/src/table.ts b/src/table.ts index 2eafd42..5f16d3b 100644 --- a/src/table.ts +++ b/src/table.ts @@ -260,29 +260,6 @@ export class Table { return result.count > 0; } - /** - * Checks if a trigger with the given name exists on this table and fires. - * A trigger turned off with ALTER TABLE ... DISABLE TRIGGER still exists but - * does not fire. - */ - async triggerEnabled( - connection: CommonQueryMethods, - triggerName: string, - ): Promise { - const result = await connection.one( - sql.type(z.object({ count: z.coerce.number() }))` - SELECT COUNT(*) FROM pg_trigger t - JOIN pg_class c ON t.tgrelid = c.oid - JOIN pg_namespace n ON c.relnamespace = n.oid - WHERE n.nspname = ${this.schema} - AND c.relname = ${this.name} - AND t.tgname = ${triggerName} - AND t.tgenabled <> 'D' - `, - ); - return result.count > 0; - } - /** * Gets column metadata for this table (excluding generated columns). */ diff --git a/src/types.ts b/src/types.ts index 81fed59..0b88c35 100644 --- a/src/types.ts +++ b/src/types.ts @@ -109,12 +109,6 @@ export interface DisableMirroringOptions { targetType?: MirroringTargetType; /** Bound on how long to wait for the table lock (default: 5s). */ lockTimeout?: string; - /** - * Remove the trigger and its function instead of turning the trigger off. - * Takes an ACCESS EXCLUSIVE lock on the source table for the duration of the - * drop; the default only disables the trigger under a lighter lock. - */ - drop?: boolean; } /** @@ -238,11 +232,7 @@ export interface TableStatus { intermediateExists: boolean; partitionCount: number; mirrorTriggerExists: boolean; - /** The mirror trigger exists and fires; false when it exists but is disabled. */ - mirrorTriggerEnabled: boolean; retiredMirrorTriggerExists: boolean; - /** The retired mirror trigger exists and fires; false when disabled. */ - retiredMirrorTriggerEnabled: boolean; originalIsPartitioned: boolean; } From e591c713343194d2920a4c4792ac426cb21b6737 Mon Sep 17 00:00:00 2001 From: Stacy Curry Date: Mon, 12 Oct 2026 10:59:25 -0500 Subject: [PATCH 4/6] fix: Inline the mirroring lock timeout to match add_partitions Co-Authored-By: Claude Fable 5.1 --- src/pgslice.ts | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/src/pgslice.ts b/src/pgslice.ts index 6585e89..ac41607 100644 --- a/src/pgslice.ts +++ b/src/pgslice.ts @@ -124,22 +124,6 @@ export class Pgslice { return this.pool.connect(handler); } - /** - * Bounds how long DDL in this transaction may wait for a table lock, so a - * contended run fails fast and is retried instead of queueing live traffic - * behind it. Uses the same default as `swap`. - */ - async #setLockTimeout( - tx: DatabaseTransactionConnection, - lockTimeout: string | undefined, - ): Promise { - await tx.query( - sql.typeAlias( - "void", - )`SET LOCAL lock_timeout = ${sql.literalValue(lockTimeout ?? "5s")}`, - ); - } - async #withLock( tx: CommonQueryMethods, table: Table, @@ -819,7 +803,11 @@ export class Pgslice { throw new Error(`Table not found: ${target.toString()}`); } - await this.#setLockTimeout(tx, options.lockTimeout); + await tx.query( + sql.typeAlias( + "void", + )`SET LOCAL lock_timeout = ${sql.literalValue(options.lockTimeout ?? "5s")}`, + ); await new Mirroring({ source: table, targetType }).enable(tx, target); }), ); @@ -843,7 +831,11 @@ export class Pgslice { throw new Error(`Table not found: ${table.toString()}`); } - await this.#setLockTimeout(tx, options.lockTimeout); + await tx.query( + sql.typeAlias( + "void", + )`SET LOCAL lock_timeout = ${sql.literalValue(options.lockTimeout ?? "5s")}`, + ); await new Mirroring({ source: table, targetType }).disable(tx); }), ); From e8c1956ce50a40b0197e31e4b1264b8d46765910 Mon Sep 17 00:00:00 2001 From: Stacy Curry Date: Mon, 12 Oct 2026 11:40:21 -0500 Subject: [PATCH 5/6] fix: Lock the table before enable drops and recreates the mirroring trigger Re-running enable_mirroring or enable_retired_mirroring on a partitioned table drops the existing trigger first, which walks the partitions before the parent exactly as disable did. Take the same parent lock up front. Co-Authored-By: Claude Fable 5.1 --- src/mirroring.test.ts | 41 +++++++++++++++++++++++++++++++++++++++++ src/mirroring.ts | 12 +++++++++--- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/mirroring.test.ts b/src/mirroring.test.ts index c8bbbc3..d8bb17d 100644 --- a/src/mirroring.test.ts +++ b/src/mirroring.test.ts @@ -449,6 +449,47 @@ describe("Pgslice.disableMirroring (partitioned table)", () => { expect(held).toBe(1); }); + test("enable locks the parent before it drops and recreates the trigger", async ({ + transaction, + }) => { + const issued: string[] = []; + const recording: DatabaseTransactionConnection = { + ...transaction, + async query(query) { + issued.push(query.sql.replace(/\s+/g, " ").trim()); + return transaction.query(query); + }, + }; + + await new Mirroring({ + source: Table.parse("posts"), + targetType: "retired", + }).enable(recording, Table.parse("posts_retired")); + + const lockIndex = issued.findIndex((statement) => + /^LOCK TABLE "public"\."posts" IN ACCESS EXCLUSIVE MODE$/.test(statement), + ); + const dropIndex = issued.findIndex((statement) => + /^DROP TRIGGER IF EXISTS/.test(statement), + ); + expect(lockIndex).toBeGreaterThanOrEqual(0); + expect(dropIndex).toBeGreaterThan(lockIndex); + }); + + test("re-running enable leaves one trigger on the parent and each partition", async ({ + pgslice, + transaction, + }) => { + await pgslice.enableMirroring(transaction, { + table: "posts", + targetType: "retired", + }); + + expect( + await triggerCount(transaction, "posts_retired_mirror_trigger"), + ).toBe(3); + }); + test("removes the trigger from the parent and every partition", async ({ pgslice, transaction, diff --git a/src/mirroring.ts b/src/mirroring.ts index 2e2bbe9..7f097fd 100644 --- a/src/mirroring.ts +++ b/src/mirroring.ts @@ -20,7 +20,8 @@ interface MirroringOptions { * taking an ACCESS EXCLUSIVE lock on each partition before the parent. * Statements enter through the parent and lock it first, so the two walk the * same relations in opposite orders and deadlock under live traffic. Dropping - * therefore locks the parent explicitly before anything else. + * therefore locks the parent explicitly before anything else, and so does + * enabling, which drops any existing trigger before recreating it. */ export class Mirroring { readonly #source: Table; @@ -47,6 +48,7 @@ export class Mirroring { target, ); + await this.#lockSource(tx); await tx.query(sql.typeAlias("void")`${functionSql}`); await tx.query(sql.typeAlias("void")`${this.#dropTriggerSql}`); await tx.query(sql.typeAlias("void")`${this.#createTriggerSql}`); @@ -56,13 +58,17 @@ export class Mirroring { * Disables mirroring by dropping the trigger and function. */ async disable(tx: DatabaseTransactionConnection): Promise { + await this.#lockSource(tx); + await tx.query(sql.typeAlias("void")`${this.#dropTriggerSql}`); + await tx.query(sql.typeAlias("void")`${this.#dropFunctionSql}`); + } + + async #lockSource(tx: DatabaseTransactionConnection): Promise { await tx.query( sql.typeAlias("void")` LOCK TABLE ${this.#source.sqlIdentifier} IN ACCESS EXCLUSIVE MODE `, ); - await tx.query(sql.typeAlias("void")`${this.#dropTriggerSql}`); - await tx.query(sql.typeAlias("void")`${this.#dropFunctionSql}`); } get #functionName() { From 0d4d2c8891ed7bc563248bb22df0f18b3ca7c122 Mon Sep 17 00:00:00 2001 From: Stacy Curry Date: Tue, 13 Oct 2026 09:29:57 -0500 Subject: [PATCH 6/6] make comments more accurate --- README.md | 4 ++-- src/commands/disable-mirroring.ts | 8 +++++--- src/commands/disable-retired-mirroring.ts | 8 +++++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 4a38e3a..d43007e 100644 --- a/README.md +++ b/README.md @@ -144,7 +144,7 @@ pgslice disable_mirroring
After the swap, the original mirroring triggers are no longer needed since the tables have been swapped. -The table is locked before the trigger is dropped, so this is safe to run against a busy partitioned table. The lock wait is bounded by `--lock-timeout` (default: `5s`). +The table is locked before the trigger is dropped, parent first and then its partitions, which is the order application statements lock in, so the drop cannot deadlock against live traffic. While the command waits for the lock, new statements on the table queue behind it; the wait is bounded by `--lock-timeout` (default: `5s`), and a timeout aborts the command cleanly so it can be retried. 11. Enable Reverse Mirroring (now-partitioned table to retired table) @@ -170,7 +170,7 @@ pgslice fill
--swapped pgslice disable_retired_mirroring
``` -Once you're confident the retired table is no longer needed and you're ready to drop it, disable the retired mirroring triggers. The table is locked before the trigger is dropped, so this is safe to run against a busy partitioned table. The lock wait is bounded by `--lock-timeout` (default: `5s`). +Once you're confident the retired table is no longer needed and you're ready to drop it, disable the retired mirroring triggers. The table is locked before the trigger is dropped, parent first and then its partitions, which is the order application statements lock in, so the drop cannot deadlock against live traffic. While the command waits for the lock, new statements on the table queue behind it; the wait is bounded by `--lock-timeout` (default: `5s`), and a timeout aborts the command cleanly so it can be retried. 14. Back up the retired table with a tool like [pg_dump](https://www.postgresql.org/docs/current/static/app-pgdump.html) and drop it diff --git a/src/commands/disable-mirroring.ts b/src/commands/disable-mirroring.ts index 99c0560..b0f8f73 100644 --- a/src/commands/disable-mirroring.ts +++ b/src/commands/disable-mirroring.ts @@ -12,9 +12,11 @@ export class DisableMirroringCommand extends BaseCommand { This command removes the triggers on the source table that mirror operations to the intermediate table. Use this after partitioning is complete. - The table is locked before the trigger is dropped, so this is safe on a - partitioned table under live traffic. The lock wait is bounded by - --lock-timeout; a timeout aborts cleanly and can be retried. + The table is locked before the trigger is dropped, parent first and + then its partitions, the same order application statements lock in, + so the drop cannot deadlock against live traffic. Statements on the + table queue behind the lock for the duration; the wait is bounded by + --lock-timeout, and a timeout aborts cleanly and can be retried. `, examples: [ ["Disable mirroring for a table", "$0 mirroring disable posts"], diff --git a/src/commands/disable-retired-mirroring.ts b/src/commands/disable-retired-mirroring.ts index cacd465..96249c3 100644 --- a/src/commands/disable-retired-mirroring.ts +++ b/src/commands/disable-retired-mirroring.ts @@ -16,9 +16,11 @@ export class DisableRetiredMirroringCommand extends BaseCommand { to the retired table. Use this when you no longer need to keep the retired table in sync with the main table. - The table is locked before the trigger is dropped, so this is safe on a - partitioned table under live traffic. The lock wait is bounded by - --lock-timeout; a timeout aborts cleanly and can be retried. + The table is locked before the trigger is dropped, parent first and + then its partitions, the same order application statements lock in, + so the drop cannot deadlock against live traffic. Statements on the + table queue behind the lock for the duration; the wait is bounded by + --lock-timeout, and a timeout aborts cleanly and can be retried. `, examples: [ [