RHINENG-26348: add ability to repair system_advisories_0 partition - #2287
Open
TenSt wants to merge 1 commit into
Open
RHINENG-26348: add ability to repair system_advisories_0 partition#2287TenSt wants to merge 1 commit into
TenSt wants to merge 1 commit into
Conversation
Reviewer's GuideAdds a gated, one-off database_admin flag and migration path to detach/drop/recreate the system_advisories_0 partition and clear related bucket-0 caches, plus documents how and where to use it in configuration and runbook docs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In repair_system_advisories_0.sql, the existence check and detach logic rely only on relname without schema qualification; consider filtering on the expected schema (e.g., JOIN pg_namespace and check nspname) to avoid accidental interaction with identically named tables in other schemas.
- The new partition definition in repair_system_advisories_0.sql hardcodes the modulus (32) and partition options; you may want to derive these from the existing system_advisories partitioned table metadata or reuse a shared template to keep this in sync if partitioning parameters change in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In repair_system_advisories_0.sql, the existence check and detach logic rely only on relname without schema qualification; consider filtering on the expected schema (e.g., JOIN pg_namespace and check nspname) to avoid accidental interaction with identically named tables in other schemas.
- The new partition definition in repair_system_advisories_0.sql hardcodes the modulus (32) and partition options; you may want to derive these from the existing system_advisories partitioned table metadata or reuse a shared template to keep this in sync if partitioning parameters change in the future.
## Individual Comments
### Comment 1
<location path="docs/md/major-migration-runbook.md" line_range="156" />
<code_context>
+|---|---|
+| **Config key** | `repair_system_advisories_0` (boolean, default `false`) |
+| **Where** | `DATABASE_ADMIN_CONFIG` on the **db-migration Job** only |
+| **Effect** | After migrate CONTINUE/MIGRATE, runs `prepareForMigration`, then `database_admin/schema/repair_system_advisories_0.sql`: detach/drop/recreate empty `system_advisories_0`, clear bucket-0 `advisory_account_data` and `account_advisory`. **Destructive** for hash remainder 0. |
+
+**Enable when:** one-off recovery from corrupt/unreadable `system_advisories_0`. Combine with `terminate_db_sessions=true` if detach/drop is blocked by app sessions.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider rephrasing “After migrate CONTINUE/MIGRATE” for grammatical clarity.
The wording is confusing because “migrate” reads as a verb without a subject. Please consider a clearer alternative such as “After the CONTINUE/MIGRATE migration step runs” or “After CONTINUE/MIGRATE mode is used,” depending on what you intend to describe.
```suggestion
| **Effect** | After the CONTINUE/MIGRATE migration step runs, executes `prepareForMigration`, then `database_admin/schema/repair_system_advisories_0.sql`: detach/drop/recreate empty `system_advisories_0`, clear bucket-0 `advisory_account_data` and `account_advisory`. **Destructive** for hash remainder 0. |
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2287 +/- ##
=======================================
Coverage 59.27% 59.27%
=======================================
Files 148 148
Lines 9488 9488
=======================================
Hits 5624 5624
Misses 3276 3276
Partials 588 588
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
TenSt
force-pushed
the
stepan/RHINENG-26348-repair-system-advisories-table
branch
from
July 30, 2026 17:25
b4cd6fb to
0a79236
Compare
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.
This PR:
repair_system_advisories_0flag (default off) for db-migration Jobsystem_advisories_0and clears bucket-0 AAD/AA cachesclowdapp.yamland major-migration runbookTesting:
I've tested locally and all looks good. We will tests everything in stage before going to production even that stage doesn't have the corrupted partition.
Summary by Sourcery
Add a one-off, configurable repair pathway for the system_advisories_0 partition in the db-migration job, including cache cleanup for bucket-0 accounts.
New Features:
Enhancements:
Documentation:
Chores: