database: drop old delete_system function#2229
Open
TenSt wants to merge 1 commit into
Open
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the database schema migration version and adds a new migration to drop the legacy delete_system function if it exists. 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:
- Consider schema-qualifying the function in the migration (e.g.,
DROP FUNCTION IF EXISTS public.delete_system(...)) to avoid ambiguity if multiple schemas define adelete_systemfunction. - If your migration process supports rollbacks, you may want to add a corresponding down migration that re-creates
delete_systemto keep the migration set reversible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider schema-qualifying the function in the migration (e.g., `DROP FUNCTION IF EXISTS public.delete_system(...)`) to avoid ambiguity if multiple schemas define a `delete_system` function.
- If your migration process supports rollbacks, you may want to add a corresponding down migration that re-creates `delete_system` to keep the migration set reversible.
## Individual Comments
### Comment 1
<location path="database_admin/migrations/158_drop_old_delete_system_function.up.sql" line_range="1" />
<code_context>
+DROP FUNCTION IF EXISTS delete_system(inventory_id_in varchar);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Schema-qualify the function name and verify the dropped signature matches all existing overloads.
Relying on the search_path can drop the wrong function when multiple schemas or similarly named functions exist. Also, if other delete_system overloads exist (e.g., text vs varchar), they’ll be left behind. Consider explicitly qualifying the schema in the DROP statement and ensuring all relevant overloads are removed as needed.
Suggested implementation:
```
-- Explicitly drop all known overloads of delete_system, schema-qualified
-- to avoid relying on search_path and accidentally dropping the wrong function.
-- NOTE: adjust the schema name and overloads here if they differ in your environment.
DROP FUNCTION IF EXISTS public.delete_system(inventory_id_in varchar);
-- Example additional overloads; uncomment or adjust as needed:
-- DROP FUNCTION IF EXISTS public.delete_system(inventory_id_in text);
-- DROP FUNCTION IF EXISTS public.delete_system(inventory_id_in uuid);
```
1. Confirm the actual schema that owns `delete_system` (e.g., `public`, `inventory`, or another app schema) and replace `public` with the correct schema.
2. Query the catalog (e.g., `SELECT proname, oid::regprocedure FROM pg_proc WHERE proname = 'delete_system';`) to identify all existing overloads.
3. For each overload you find, add a corresponding `DROP FUNCTION IF EXISTS schema.delete_system(<arg_types>);` line to this migration, removing or adjusting the example overloads I included.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MichaelMraka
approved these changes
Jun 4, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2229 +/- ##
==========================================
- Coverage 58.46% 58.44% -0.03%
==========================================
Files 139 139
Lines 8925 8925
==========================================
- Hits 5218 5216 -2
- Misses 3159 3161 +2
Partials 548 548
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:
|
auto-merge was automatically disabled
June 4, 2026 14:57
Branch protection rule check failed
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.
We found out that there is an old
delete_system(inventory_id_in varchar)function that was living in our database for a long time.