Repository navigation
Conversation
The repaired events and delivery_log catalogs end in an empty catch-all
from 2027-01-01 that the merged manager only audits. Add opt-in
maintenance (BUZZ_PARTITION_MANAGER_ADVANCE_ENABLED, default false) that
keeps six dedicated monthlies after the current month by replacing the
empty catch-all with canonical monthlies and a later <table>_p_future,
at startup and on every audit interval.
Each table's change runs in one transaction that takes a schema-scoped
advisory lock (losers report skipped_locked), locks foreign-key
counterparts ACCESS EXCLUSIVE NOWAIT in OID order before ONLY the parent
under a 2s lock_timeout and the catch-all NOWAIT, re-plans from a
locked re-audit, and verifies bounds, trigger parity, and index and
constraint inheritance before commit. Statement, idle-in-transaction,
and a 10s client deadline bound every attempt; a deadline-abandoned
connection is closed rather than pooled. A populated catch-all,
misaligned bound, unsafe catalog, or name collision is refused as
operator_required before any lock.
Uncovered-month creation now uses the same locked path. The relay and
buzz-admin audit horizon becomes six months, and maintenance outcomes
are exported as buzz_partition_maintenance_runs_total{table,outcome}.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ian Oberst <ioberst@block.xyz>
- Refuse to replace a catch-all that carries child-only triggers; the replacement partitions inherit only parent triggers. - Re-plan from a fresh audit once the maintenance advisory lock is held, before any table lock, so a runner that lost a race sees the winner's layout as a no-op instead of reporting its monthlies as collisions. Name collisions on the refusal path are still reported before any lock. - Retry a read-only table audit (up to twice) when a partition it listed was dropped concurrently: pg_get_expr renders from current catalog state and yields NULL for a relation missing since the snapshot, and the emptiness probe then fails with 42P01. - Scope the deadline test to the backend blocked by its own holder, and let concurrent-runner convergence tolerate a fail-fast lock_timeout from the other runner's audit probing the catch-all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ian Oberst <ioberst@block.xyz>
There was a problem hiding this comment.
Review of this PR at ee1fb7e4ad8d3d75dda12ce4d9da949e38075ea9.
The direction is good. Partition DDL now goes through one advisory-locked transaction that re-plans under its locks and verifies before commit. Refusing a populated catch-all keeps row moves an operator job. The re-plan and verify logic and the close_on_drop deadline path held up under review.
One P1 (inline at maintenance.rs L545): the lock order and mode in apply_change can deadlock with foreground events → communities traffic. This was reproduced against a real relay on PostgreSQL 17.11, where maintenance was the 40P01 victim. From the source, the reverse interleaving can instead abort a foreground ingest write. The details and suggested fix are inline. There are also several P2s inline.
Evidence at this SHA:
cargo test -p buzz-db --lib partition -- --ignored: 44/44 on PG17.11.- A live relay advanced the repaired layout to Jan–Apr 2027 monthlies with
*_p_futurefrom May 2027. A rerun was a no-op.
Separately, Run Codex Security Review failed because the workflow refuses to check out fork code under pull_request_target. No security review output exists for this PR.
Address review on catch-all advancement: - Lock the parent first, then counterparts in SHARE ROW EXCLUSIVE NOWAIT, then the catch-all. This matches the order writers take locks, so maintenance cannot deadlock with ingest, and readers and FK key checks on communities are no longer blocked. Classify 40P01 as lock_timeout. - Count only DDL and collision failures as per-month create errors. - Re-audit whenever a table planned DDL, and report skipped_disabled when the serialized re-plan finds only disabled work. - Make the create kill switch stop advancement too. - Fix the off-by-one in the advancement month cap. - Retry audits only on a dedicated PartitionDroppedMidAudit error. - Do not claim a deadline-abandoned attempt rolled back. - Remove the ensure_future_partitions wrapper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ian Oberst <ioberst@block.xyz>
TheSentinel454
left a comment
There was a problem hiding this comment.
🤖 Thanks for the replies and the quick fixes. Two small follow-ups at 2b53073f are inline, both P2.
Rollout note: the PR's fallback, "rerun the manual runbook", still locks communities first. That's the order that deadlocks with ingest. Whoever owns the runbook should switch it to the order used here: parent ACCESS EXCLUSIVE, then counterparts SHARE ROW EXCLUSIVE NOWAIT, then the catch-all NOWAIT.
| return Ok(LockedAttempt::Refused(collision_message(&name))); | ||
| } | ||
|
|
||
| let parent = maintenance_parent(&mut transaction, table).await?; |
There was a problem hiding this comment.
🤖 P2: maintenance_parent reads the FK counterparts before the parent lock, and the locked re-audit at L615 only re-checks the partition layout. If an FK to or from events is committed between this read and the parent lock, the new table isn't in counterparts. CREATE … PARTITION OF then takes SHARE ROW EXCLUSIVE on it with a wait (no NOWAIT) while holding the parent's ACCESS EXCLUSIVE, which is the stall this ordering is meant to rule out. The window is small, since FK DDL on events itself needs a lock that conflicts with the parent lock. Re-running maintenance_parent after the parent lock and returning Refused if the result changed would close it. Keep this early call as the cheap pre-lock refusal.
There was a problem hiding this comment.
🤖 Confirmed and fixed in 7b6b199. I went a little further than you suggested. Once the parent is locked, maintenance reads the counterparts again and uses that set:
- if the new read now refuses (for example, a new FK that references
events), the run returnsRefused; - if the set has grown, the new counterparts are locked
SHARE ROW EXCLUSIVE NOWAITlike the rest.
Refusing whenever the set changed would report a harmless race as operator_required. The parent-locked read is enough on its own: any FK DDL on or to events needs a lock that conflicts with the parent's ACCESS EXCLUSIVE, so the set can't change after that point. The transaction is READ COMMITTED, so the second read sees what committed while we waited. The early read stays as the cheap pre-lock refusal.
New test: foreign_key_committed_while_waiting_on_the_parent_is_locked_nowait.
- A migration holds an uncommitted
ALTER TABLE events ADD FOREIGN KEY … REFERENCES reviewers. - A
reviewerswriter queues behind the migration. - Maintenance queues on the parent, then the migration commits.
Maintenance now fails with events lock_timeout within 1 s, and delivery_log still advances. Without the re-read, it held the parent for 2.06 s waiting on reviewers, and the test fails.
| ), | ||
| ) | ||
| .await?; | ||
| // Attaching a partition adds its foreign key under SHARE ROW EXCLUSIVE on |
There was a problem hiding this comment.
🤖 P2 (doc): this explains SHARE ROW EXCLUSIVE only by what attaching needs. It would help to also say why dropping the catch-all can't need more. Suggested addition: "Dropping the catch-all removes only its inherited constraint and check triggers, which lock the catch-all itself, never the referenced table (PG14–17)." Without that line, it's natural to suspect the DROP upgrades the lock on communities. We did, and the probes showed it doesn't.
There was a problem hiding this comment.
🤖 Added in 7b6b199: dropping the catch-all removes only its inherited foreign key and check triggers, which lock the catch-all itself, never the referenced table. I cited the versions we probed ("lock probes on PostgreSQL 16 and 17") rather than 14–17, since 14 and 15 weren't measured.
The counterpart set was read before the parent lock, and the locked re-audit checks only the partition layout. A foreign key on or to the parent that committed while maintenance waited for the parent lock was missing from the set, so attaching a partition waited on its table without NOWAIT while holding the parent ACCESS EXCLUSIVE, the stall the lock order is meant to rule out. Read the counterparts again once the parent is locked and use that set. Foreign-key DDL on or to the parent conflicts with the parent lock, so the set is stable from there. The pre-lock read remains as the cheap refusal. A new test commits a foreign key while maintenance waits on the parent, with a writer on the new counterpart: maintenance now fails fast with lock_timeout instead of waiting out the 2 s lock timeout under the parent. Also document why dropping the catch-all needs no stronger counterpart lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ian Oberst <ioberst@block.xyz>
|
🤖 Re: the rollout note in the latest review (link) that the manual runbook still locks The runbook keeps The per-environment execution records now carry a "do not reuse this SQL" banner. Their rendered SQL is unchanged so the recorded hashes still match. The PR's fallback line now points to the revised order. |
TheSentinel454
left a comment
There was a problem hiding this comment.
Two optional nits: in the new test, asserting message.contains("reviewers") would be a stronger check than the 1 s bound. That bound also covers delivery_log's whole advance, and it failed 3 of 20 runs under 4× load. And the post-lock refusal branch at :597-600 has no test.
| lock( | ||
| &mut transaction, | ||
| &format!( | ||
| "LOCK TABLE ONLY {} IN ACCESS EXCLUSIVE MODE NOWAIT", |
There was a problem hiding this comment.
An FK added with ALTER TABLE ONLY <catch-all> ADD FOREIGN KEY … REFERENCES x is missed in two places. maintenance_parent misses it because it's not on the parent. The child-only-trigger refusal misses it because RI triggers are tgisinternal. The parent lock doesn't block adding it. DROP TABLE at :657 then takes AccessExclusiveLock on x without NOWAIT while holding the parent (2.06 s against a plain x reader on PG17.11). With no contention, it silently drops the FK. Suggest refusing here, after the catch-all lock, if the catch-all has any FK with conparentid = 0 or is referenced by one. Use the existing "lock set is not proven" refusal, and qualify the comment at :605-607 and PR body line 19.
There was a problem hiding this comment.
🤖 Confirmed and fixed in d2178b0. Maintenance now refuses with the existing "lock set is not proven" reason when the catch-all has a foreign key with conparentid = 0, or is referenced by one. The refusal message names the constraints.
The check runs twice:
- Before any lock. A standing case is then refused without taking the parent outage on every run.
- After the catch-all lock, as you suggested. That's the authoritative point, because FK DDL on the catch-all conflicts with that lock but not with the parent's.
The DROP comment and the PR description now say the no-stronger-lock claim covers inherited keys only, and that other keys are refused.
Tests:
catch_all_foreign_keys_outside_the_parent_are_refusedcovers both anALTER TABLE ONLY <catch-all> ADD FOREIGN KEYand a table that references the catch-all. A reader holds the parent throughout, so the refusal has to come before the parent lock. The key survives, and once it's removed the advance goes through.catch_all_foreign_key_committed_while_waiting_on_the_parent_is_refusedcommits the catch-all key while maintenance waits on the parent, so only the post-lock check can see it.
Removing either check fails its test.
A foreign key added to the catch-all alone, or one referencing it, is invisible to the parent's counterpart set and to the child-only trigger refusal, since foreign-key triggers are internal, and adding it conflicts with no parent lock. Dropping the catch-all then removed it silently and, under contention, locked its other table ACCESS EXCLUSIVE without NOWAIT while holding the parent. Refuse such a catch-all with the existing "lock set is not proven" reason: once before any lock, so a standing case never takes the parent outage, and again after the catch-all lock, where the check is authoritative because foreign-key DDL on the catch-all conflicts with it. Tests: - Both kinds of key are refused without reaching the parent lock, and the key survives. - A catch-all key committed while maintenance waits on the parent is refused by the post-lock check. - A foreign key referencing the parent, committed while maintenance waits, hits the post-lock counterpart refusal, which had no test. - The FK-check writer test also writes to communities, so locking the counterparts SHARE ROW EXCLUSIVE before the parent now deadlocks it. - The committed-counterpart test asserts that NOWAIT names the new table instead of a 1 s bound that also covered delivery_log's advance. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ian Oberst <ioberst@block.xyz>
|
🤖 Re: the two optional nits in the latest review (link). Took both in d2178b0:
The partition Postgres suite (52 tests) passed five times in a row. The four concurrent tests and the writer test passed 20 of 20 runs. That was without your 4× load, but the assertion no longer depends on timing. |
Summary
Adds bounded, serialized advancement of an empty right-edge partition catch-all for
eventsanddelivery_log. The repaired catalogs currently end in an empty catch-all from 2027-01-01, and the merged manager (#6515) only audits them. Without this change, January writes land in the catch-all, after which only an operator can move them.When
BUZZ_PARTITION_MANAGER_ADVANCE_ENABLED=true, each relay keeps six dedicated monthlies after the current month. It does this at startup and on every audit interval by replacing the empty catch-all with canonical monthlies and a new canonical<table>_p_futureafter the horizon. From today's catalogs the first run creates January–March 2027 and moves the catch-all to April 2027.Safety design
Refuse before locking. The following return
operator_requiredwithout taking any table lock:DEFAULT, anomalous or pending-detach child;One winner. A schema-scoped
pg_try_advisory_xact_lockis taken before any DDL. Losers reportskipped_lockedand never issue DDL. The winner re-plans from a fresh audit before taking any table lock and checks for collisions. A runner that lost a race therefore sees the committed layout as a no-op, not as colliding names.Writer lock order. First
ONLYthe parent is lockedACCESS EXCLUSIVEunder a 2 slock_timeout. Then each foreign-key counterpart (today onlycommunities) is locked in OID orderSHARE ROW EXCLUSIVE NOWAIT, then the catch-allNOWAIT.NOWAITtoo, so attaching never waits on it under the parent lock. The read before the lock remains as a cheap early refusal.communities), so maintenance cannot deadlock with ingest. The internal manual runbook used to lockcommunitiesfirst; it now uses this order too.SHARE ROW EXCLUSIVEis the strongest lock the DDL takes oncommunities; measured on PG 16 and 17, the catch-allDROPtakes none, as long as the catch-all's only foreign keys are those inherited from the parent. A catch-all with its own foreign key, or one that another table's foreign key references, is refused: once before any lock, and again after the catch-all lock, where the check is authoritative. Readers and other writers'FOR KEY SHAREchecks proceed. A concurrentcommunitieswriter makes the run fail fast withlock_timeout.40P01deadlock victim is also reported aslock_timeout.Re-plan under lock. The catalog is re-audited under those locks. The transaction proceeds only if the plan is unchanged.
Bounded.
statement_timeout5 s,idle_in_transaction_session_timeout5 s, and a 10 s client deadline per table. A deadline-abandoned connection is closed, never returned to the pool, and the server rolls back anything uncommitted. If the deadline fires afterCOMMITwas sent, the change may have landed; the next audit reports the truth. The orphaned backend keeps its locks until its in-flight statement ends, at moststatement_timeout.Verify before commit. The changed catalog is re-audited inside the transaction for bounds, canonical kinds, trigger parity and catch-all emptiness, and serving safety must not regress. It also checks that every new child has valid, ready clones of each parent index and validated clones of each PK/unique/FK constraint.
Audits tolerate a concurrent advance. Catalog rows come from the statement snapshot, but
pg_get_exprrenders from current catalog state. So an audit that overlaps another relay'sDROPof a catch-all can see a partition that no longer exists. The read-only audit detects this (a NULL bound, or42P01from the emptiness probe) and retries up to twice with a fresh snapshot.Fail fast on audit probes. If another relay's audit is probing the catch-all's emptiness, the catch-all
NOWAITlock fails and the run reportslock_timeout. The parent lock is then released immediately, and the next runner or interval completes the advance.Uncovered-month creation (
BUZZ_PARTITION_MANAGER_CREATE_ENABLED, defaulttrue) now runs through the same locked transaction, replacing the unlocked per-monthCREATE. That flag remains the kill switch for all automatic partition DDL: with it off, advancement is disabled too. With advancement disabled (the default) the periodic loop stays read-only and startup behaves as before, apart from the locking.A runner that planned DDL always re-audits before returning, even when a peer did the work, so it never reports the pre-maintenance layout. A serialized re-plan that finds only disabled work reports
skipped_disabled.Behavior changes
buzz-admin partition-auditdefault are now 6 months (PARTITION_MANAGER_MONTHS_AHEAD), up from 3. Until advancement is enabled, months already covered by the catch-all makedegradedtrue. That correctly reports a runway under six months.buzz_partition_maintenance_runs_total{table,outcome}with outcomesnoop | skipped_disabled | created | advanced | skipped_locked | operator_required | lock_timeout | deadline | error.buzz_partition_create_attempts_totalkeeps its per-monthcreated | skipped_covered | errorlabels.errorthere still means a DDL or collision failure;lock_timeoutanddeadlineruns add nothing to it, and onlybuzz_partition_maintenance_runs_totalrecords them.ensure_future_partitions(free function andDbmethod) is removed;maintain_partitionsis the one entry point.lock_type="partition_maintenance".Rollout
Enable
BUZZ_PARTITION_MANAGER_ADVANCE_ENABLEDper environment in BPCI:bb-block-stagingfirst, then production. The staging canary proves a real advance because the six-month horizon already extends past January. Fallback: rerun the internal manual runbook, which uses the same parent-first lock order as of 2026-10-05. It keepsACCESS EXCLUSIVEon the counterparts, because its detach/attach can drop FK triggers on them.Testing
cargo test -p buzz-db --lib partition: 17 unit tests. The 10 new ones cover:cargo test -p buzz-db --lib partition -- --ignored(PostgreSQL 17): 48/48 on 9 consecutive runs, including 17 new tests:communitieswriter making the run fail fast and roll back;communitiesreader not blocking the advance;communities, behind queued maintenance completing without a deadlock;lock_timeouton that table instead of waiting under the parent lock;events, committed while maintenance waits on the parent, being refused under the parent lock;40P01deadlock victim being classified aslock_timeout;lock_timeout;skipped_disabled;ACCESS EXCLUSIVEorder fails the reader test (lock_timeout) and the writer test (a ~1 s deadlock wait);NOWAITfrom the counterpart lock, or allowing a populated catch-all, fails the matching tests;SHARE ROW EXCLUSIVEbefore the parent fails the writer test;hanging_redis_peer_times_out_before_the_health_listener_binds;readiness_check_cancellation_balances_waiter_and_inflight_connection.cargo clippy -p buzz-db -p buzz-relay -p buzz-admin --all-targets -D warningsandcargo fmt --checkare clean.Related
Refs #2396.
This PR conflicts textually with #7994 (partition audit observability follow-ups) in
crates/buzz-db/src/store/partition.rsandcrates/buzz-relay/src/main.rs. Whichever PR merges second will be rebased onto the other.🤖 Generated with Claude Code