Repository navigation
fix(queue-storage): create queue lanes as the schema owner (0.6) - #638
Conversation
A lane's enqueue and claim sequences were created by whichever role first touched the lane, through SECURITY INVOKER helpers, so only that role and the roles its default privileges named could use them. Another runtime role then failed with "permission denied for sequence" on enqueue, claim or promotion. Each queue-storage schema now has two SECURITY DEFINER functions owned by the schema owner, provision_lane and provision_lane_sequences. The lane helpers create a missing sequence through them, and each new sequence gets the privileges of the head table it backs. ensure_lane_inserts calls provision_lane when it exists and is executable, inside a savepoint, and otherwise writes the lane rows as before. The definer_lane_provisioning schema patch refreshes the helpers and repairs existing lane sequences one at a time, reassigning and granting only what the applying role may and reporting the rest. awa.lane_sequence_problems and the new `awa storage lane-sequences` command list what remains with the statements to run; awa.repair_lane_sequences and `--repair` rerun the repair.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a8d6532dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AND ((SELECT count(DISTINCT p.proname) FROM pg_proc AS p \ | ||
| WHERE p.pronamespace = n.oid \ | ||
| AND p.proname IN ('provision_lane', 'provision_lane_sequences') \ | ||
| AND p.prosecdef \ | ||
| AND oidvectortypes(p.proargtypes) = 'text, smallint, smallint') < 2 \ |
There was a problem hiding this comment.
When the first migration is run by a role that can create these functions but cannot SET ROLE to the schema owner, refresh_lane_sequence_helpers only warns and leaves both provisioners owned by the migrating role. This probe nevertheless considers the patch applied because it checks only the signatures and prosecdef; a later awa migrate under the correct owner therefore never refreshes them, despite the warning explicitly recommending that rerun. Every subsequently provisioned sequence remains owned by the wrong role and is continually reported for repair, so include proowner = n.nspowner for both functions in the pending condition.
Useful? React with 👍 / 👎.
…vision_lane Queue storage takes any queue name, including an empty one, and any smallint priority, so the provisioning functions only reject a null argument or a shard outside 0-63. The released-binary rehearsal now expects a 0.6.9 producer without CREATE to create a new lane, owned by the schema owner.
Once a handler returned, the executor removed the attempt from the heartbeat set and finalized it in a detached task that Client::shutdown did not wait for. When the last handler returned as the drain ended, shutdown cancelled the completion batcher before that finalize reached it, and the direct-finalize fallback raced the process exit. The attempt stayed open with no heartbeat, so stale-heartbeat rescue ran the finished job again after heartbeat_staleness. Track finalizers in a TaskTracker and wait for them, within the drain timeout, before stopping the completion batcher, heartbeat and maintenance services. This is the shutdown part of #521 on main.
…ded lock wait The definer_lane_provisioning patch reassigned lane sequences inside the patch transaction. ALTER SEQUENCE ... OWNER takes the sequence's ACCESS EXCLUSIVE lock until commit, with no lock_timeout, so an open enqueue on one lane could stall the patch and every repaired lane waited for it to commit. The patch now only refreshes the helpers. After it commits, awa migrate repairs each lane sequence with its own statement through awa.repair_lane_sequence(schema, sequence), which has lock_timeout = 300ms; on 55P03 the sequence is left unchanged and reported as busy. The results are in MigrationReport::lane_sequences. `awa storage lane-sequences --repair` uses the same path, and the set-returning awa.repair_lane_sequences(schema) is gone. Reassigning needs the current owner's privileges, which the schema owner lacks for sequences the migration role or an app role created. A migration role connected with role set to the owner therefore could not reassign its own sequences. The repair now retries such a sequence as the login role, lane_sequence_problems reports repair_roles (the login roles that can repair each sequence), and leftovers are reported with the next step to take. The docs state which role to run the repair as.
hardbyte
added
the
full-ci
Problem
A queue-storage lane,
(queue, priority, enqueue_shard), has an enqueue and a claim sequence. The head-table triggers create them throughSECURITY INVOKERhelpers, as whichever role first touches the lane. That role owns them, and only the roles its default privileges name get access. The first touch can come from several places:queue_meta.enqueue_shardswas raised, because promotion routes throughshard_for_enqueue;prepare_queue, run as whichever role runs the CLI.Suppose several application roles share a runtime group role and only the schema owner and the migration role have default privileges. A lane first created by one application role is then unusable by the others, which fail with
permission denied for sequenceon enqueue, claim or promotion. With theprepared_lane_sequencespatch, a runtime without schemaCREATEcan use lanes that were provisioned in advance, but it cannot create a lane at all.Change
Definer provisioning.
awa.refresh_lane_sequence_helpersnow installs two functions in each queue-storage schema:provision_lane_sequences(queue, priority, shard)creates a lane's two sequences.provision_lane(queue, priority, shard)creates the sequences, thequeue_lanesrow and both head rows.Both are
SECURITY DEFINER, owned by the schema owner, withsearch_path = pg_catalog, pg_temp.22023. Queue and priority get no further checks, because queue storage accepts any queue name, including an empty one, and anysmallintpriority, and the definer must not reject a lane the invoker path accepts.ensure_lane_insertsalready takes,{schema}:{queue}:{priority}:{shard}, before writing. The lock is re-entrant for the Rust caller, which holds it already, and keeps the existing lane lock order (fix(insert): take enqueue lane locks in one order across batch inserts #625).ON CONFLICT.EXECUTEtoPUBLIC. SchemaUSAGEgates access to these functions.SET ROLEto it, the refresh reassigns both functions to the owner. Otherwise it warns.The three lane helpers (
ensure_lane_sequencesand the two head-sync trigger functions) create a missing sequence throughprovision_lane_sequenceswhen the caller may execute it. Otherwise they keep the v049 behaviour: create the sequence with schemaCREATE, or raise the provisioning hint. So every path creates owner-owned sequences once the helpers are refreshed, including N-1 binaries and SQL callers ofinsert_job_compat.Grant model
A new lane sequence gets privileges mirrored from the head table it backs:
queue_enqueue_seq_*fromqueue_enqueue_heads,queue_claim_seq_*fromqueue_claim_heads. A grantee withINSERTorUPDATEon the table getsUSAGE, SELECT, UPDATE; one with onlySELECTgetsSELECT. The grants are explicit and do not depend on default privileges.AWA has no configured runtime-role name, and
PUBLICwould let any role in the database move cursors: sequence privilege checks do not need schemaUSAGEwhen the sequence is addressed by OID. Mirroring the head table captures exactly the roles that may already advance a lane, because the sequence is that row's cursor in another form. The documented runtime grants therefore work unchanged.Repair and check
awa.lane_sequence_problems(schema [, sequence])is read-only and executable byPUBLIC. For each lane sequence that is not owned by the schema owner or lacks a mirrored grant, it returns:GRANTstatements to run as the owner, and theALTER SEQUENCE ... OWNER TOstatement;repair_roles: the login roles, superusers aside, that hold the current owner's privileges and canSET ROLEto the schema owner (MEMBERbefore PG16), so can run both.awa.repair_lane_sequence(schema, sequence)repairs one sequence. It isSECURITY INVOKER, revoked fromPUBLIC, and haslock_timeout = 300msin its definition.pg_has_roleshows the caller can act as the current owner andSET ROLEto the schema owner, and adds grants when the caller can act as the owner. It catchesinsufficient_privilegeper statement.55P03it rolls back, leaving the sequence unchanged, and returnsbusy.repaired,owner_unchanged(grants complete),unrepairedorbusy, with the statements still to run andrepair_roles.definer_lane_provisioning, which is v053 onmain, refreshes the helpers in every installed queue-storage schema and does not reassign anything. Reassigning takes the sequence'sACCESS EXCLUSIVElock until the transaction ends, and the patch is one transaction, so in-transaction repair would stall each repaired lane until the whole upgrade committed. The probe is satisfied once the definitions are installed, so leftover sequences never keep the patch pending andawa migratekeeps exiting 0.awa migraterepairs each lane sequence with its own statement, so its own transaction. NoACCESS EXCLUSIVElock outlives the one sequence's repair, and none waits more than 300 ms.SET LOCAL ROLE NONE). The documentedPGOPTIONS='-c role=awa_owner'migration therefore reassigns the migration role's sequences: asawa_ownerit cannot act as the migration role, but as itself it can.awa storage lane-sequences --repair", "rerun as one of these roles", or, when no role can, the grant that would make the migration role able to. The run'sMigrationReportalso carries every result inlane_sequences.awa storage lane-sequences [--schema S] [--repair]. Without--repairit is read-only. It prints JSON, then the same next steps, and exits 2 while problems remain.--repairuses the same per-sequence path asawa migrate. External runners, which apply only the SQL, run it as the migration role after the migration; the docs also give a psql\gexecloop that repairs one sequence per transaction.SET ROLEto the owner, because the owner holds neither the migration role's nor an app role's privileges.Rollout
ensure_lane_insertsfolds a probe into the lane-lock round trip:has_function_privilege(to_regprocedure('{schema}.provision_lane(...)'), 'EXECUTE'). When the function is missing or not executable, it runs the existing statements unchanged. Only a positive answer is cached on the store. Lane creation is rare, so an absent function is probed again on the next new lane.42883, or42501when the definer lacks a privilege, the store rolls back to the savepoint, drops the cached answer, logs once, and writes the rows itself in the same transaction. The enqueue does not fail.Validation
awa/tests/lane_ownership_roles_test.rsbuilds a non-superuser cluster per test:Scenarios:
CREATE.enqueue_shardsraised live. The leader creates shard 1 first, then A and B produce over both shards.prepare_queueas an app role.EXECUTEonprovision_lanetakes the old path alongside the new path. Thenprovision_laneis dropped after a store has cached it.awa migrateruns as the migration role withroleset to the owner, as the docs describe. It adds the owner's grants as the owner, reassigns the migration role's sequences as the login role, and reports the app role's with norepair_rolesand a next step naming the grant.unrepaired. A delegate that can act as the app role but not as the owner adds the grants (owner_unchanged).repair_rolesand a repair reassigns everything.awa migrateruns.busy.busy, with a "rerun--repair" next step. After the transaction ends, a repair finishes it.lock_timeoutremoved fromrepair_lane_sequence, this test times out: the migration waits on the lock indefinitely.Fail-before, on
release/0.6.6without this change (subset that compiles there):permission denied for sequence(withCREATE),has not been provisioned(without)Existing suites updated where behaviour changed on purpose:
least_privilege_runtime_testnow expects a runtime withoutCREATEto create an unprepared lane, owned by the schema owner.EXECUTEon the provisioner to get one.migrate_patch_cli_test's psql helper tolerates psql closing stdin afterON_ERROR_STOP, which a larger pending bundle now reaches before the whole script is written.Notes
maincounterpart: fix(queue-storage): create queue lanes as the schema owner (v053) #637 (migration v053).prepared_lane_sequences. External runners pick them up through the repeatableR__definer_lane_provisioning.sql.docs/upgrade-0.6.13-to-0.6.14.md, and "Lane sequences" indocs/security.md.Independent review and validation
Includes the small #651 point-read fix. After a priority or queue move crossed a ready-ring wrap, retained tombstoned source rows could make
get_jobreturn old fields even after the batch succeeded. The available-candidate query now excludes full-identity tombstones (a 13-line SQL change), with priority and queue ring-wrap regressions. Both regressions fail before the fix and pass after it on PostgreSQL 17 and 18; existing aged-read and batch cases also pass. Required formatting, Clippy across all targets/features, and workspace build checks pass for the prepared source.Full CI passed all 29 jobs for
8804c72fea9a821f4d25c21723a4f3221a5c27d3.The current head
b5016b7db3e5f0c2294af840557ebd990803d17donly reconciles ancestry with the reviewed #643. Its treee1ece69997cc4a95de6b1657c8a79d1e08389e6ais byte-for-byte identical to8804c72fea9a821f4d25c21723a4f3221a5c27d3, which passed the full CI above.