Repository navigation
fix(0.6): claim batch index check, SET-only migrator grants, and review follow-ups - #701
Merged
Merged
Conversation
…batch index build `ensure_claim_batch_job_index` decided a claim partition was already indexed by matching `USING gin (job_ids)` in the index definition text. A partial GIN index on `job_ids` matched, so claim prune never built the real one, and with `quote_all_identifiers` on the text did not match at all. An index of any definition already holding the target name made `CREATE INDEX IF NOT EXISTS` a silent no-op that still reported the slot indexed. The check now reads `pg_index` directly: a valid GIN index with no predicate and no expressions on `job_ids` alone, with the default `array_ops` opclass. When something else already has the target name, the function raises a warning naming it, returns false and leaves it in place; the slot keeps scanning until an operator drops or renames it. The index is created without `IF NOT EXISTS`, so a name taken concurrently also reports false. The generated function pins `quote_all_identifiers = off`. The patch probe now requires the new helper and index function, so a 0.6.15 database sees `claim_batch_job_index` as pending and `awa migrate` replaces the function. The patch still replaces functions only and takes no relation locks.
…the transaction ends The 0.6.15 release notes, the 0.6.14 to 0.6.15 upgrade guide, the lane contention troubleshooting entry and the insert module comment said `awa.insert_jobs_compat_batch` holds a lane's enqueue lock only while it writes that lane, or only for the write and the commit. The lock is transaction-scoped: the batch takes it when it reaches that lane, no longer at its first ready row, and it stays held until the calling transaction commits or rolls back, including any work the caller does after the batch.
…mode `awa.insert_jobs_compat_batch` documents that with `p_skip_unique_conflicts = false` a unique conflict fails the whole batch with a unique violation. The Rust entry points pass `true` whenever a batch carries a unique key, so nothing exercised that mode with a conflict. The new test calls the function directly on queue storage and on canonical storage, with a key that is already claimed and with a key repeated inside the batch. Each fails with `23505` and leaves no row behind; the same batch with skipping inserts the rest.
… to the table owner `awa.refresh_claim_batch_job_index_helper` transferred `ensure_claim_batch_job_index` to the owner of `lease_claim_batches` and then revoked `EXECUTE` from everyone but the owner and granted it to the roles holding `TRUNCATE`. A migration role that owned the function but was a member of the table owner only `WITH INHERIT FALSE, SET TRUE` (PostgreSQL 16+) could make the transfer, but afterwards held neither ownership nor grant options, so the revoke and grant only raised warnings and `PUBLIC` kept `EXECUTE`. The helper now resets `EXECUTE` while the running role still owns the function, then transfers ownership; `ALTER FUNCTION ... OWNER TO` keeps the ACL. Running the patch with the schema owner as the effective role, as the database-roles guide says, already produced the right ACL; the new test covers both that and the transfer path with a SET-only membership.
…benchmark The benchmark added the batch size to its job count for every batch, so a skipping insert (`insert_many_skip_unique_conflicts`, `insert_many_copy`) that left out a duplicate still counted it, inflating the reported rate, and the integrity check only asked that at least the measured window's count had landed. Each mode now reports the rows its insert returned, the row-at-a-time fallback counts the rows that went in, and throughput uses those. The output shows inserted against requested jobs, the JSON record carries both, and the integrity check requires the rows that landed to equal the jobs the inserts returned over the whole run.
hardbyte
added
the
full-ci
Codex Review Summary: ✅ CompletedThis 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. |
|
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
 |
…6.16-review-fixes # Conflicts: # CHANGELOG.md
![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: be1aa3cad3
ℹ️ 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".
…rship transfer Resetting `EXECUTE` before handing `ensure_claim_batch_job_index` to the table owner lost the former owner's access: its grant to itself was its owner entry, and `ALTER FUNCTION ... OWNER TO` rewrites that entry to the new owner. A former owner that holds `TRUNCATE` on `lease_claim_batches`, and runs claim prune, could then no longer execute the function, so its prunes skipped the index build. The helper now transfers ownership first and resets `EXECUTE` as a role with the owner's privileges. A role that may only `SET ROLE` to the table owner (PostgreSQL 16+ `INHERIT FALSE, SET TRUE`) switches to it with a transaction-local `role` setting for the reset and then restores its previous `role` value, so a `role` set through `PGOPTIONS` is kept. The final ACL on every path is the roles holding `TRUNCATE`, including a former owner, and not `PUBLIC`. The SET-only test now gives the migration role `TRUNCATE`, also covers an ordinary member refreshing as itself, and checks the caller's role is unchanged afterwards.
This was referenced Nov 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from reviewing the 0.6.15 forward-port that apply to the 0.6 line.
Summary
ensure_claim_batch_job_indexnow checks the catalog for a valid GIN index onjob_idsalone (no predicate, no expressions, default opclass) instead of matching definition text, so a partial index no longer counts. A different index already using the target name raises a warning naming it and is left in place. The function pinsquote_all_identifiers = off. The pending probe requires the new body, so 0.6.15 databases re-applyclaim_batch_job_indexonawa migrate.INHERIT FALSE, SET TRUE, refreshing as itself, handed the function to the table owner before resettingEXECUTE, then could no longer revoke, soPUBLICkeptEXECUTE. The helper now resetsEXECUTEbefore the ownership transfer. The documentedrole=<owner>path was already correct.awa.insert_jobs_compat_batchwith skipping off: a claimed or repeated key fails the batch with23505and leaves nothing behind.Testing
New tests fail before and pass after (claim index shapes, SET-only migrator on PG16+).
queue_storage_runtime_testclaim-index cases,insert_many_batch_test,lane_ownership_roles_test,migration_test,insert_many_schema_fallback_test,migrate_patch_cli_test,prepare_schema_reset_guard_testandawa-modellib tests pass on PG17; the contention gate passes with exact reconciliation.patch_upgrade_rehearsal_testwas updated for the re-applied patch but only compiled locally (it needs released wheels); CI runs it. Conflicts expected with #700 in the CHANGELOG, the 0.6.16 upgrade guide and the rehearsal's pending-patch assertion.