Conversation
Benchstat (RLS)Base: 📊 7 minor regression(s) (all within 5% threshold)
✅ 2 improvement(s)
Full benchstat output |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdded a SQL migration that deletes rows from Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Benchstat (Other)Base: ✅ No significant performance changes detectedFull benchstat output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
functions/external_role_duplicate_rows_fix.sql (1)
1-11: Consider soft delete and explicit transaction handling.The current implementation uses a hard
DELETE, which permanently removes data and loses the audit trail. Theexternal_rolestable has adeleted_atcolumn (per the model definition), suggesting soft deletes are supported. Additionally, explicit transaction handling with error recovery would improve reliability.♻️ Alternative approach using soft delete
DO $$ +DECLARE + affected_count INT; BEGIN IF EXISTS ( SELECT 1 FROM pg_tables WHERE schemaname = 'public' AND tablename = 'external_roles' ) THEN - DELETE FROM external_roles WHERE array_length(aliases, 1) = 1 AND updated_at IS NULL AND role_type in ('ClusterRole','Role') AND created_at < '2026-05-01' ; + -- Soft delete instead of hard delete to preserve audit trail + UPDATE external_roles + SET deleted_at = NOW() + WHERE array_length(aliases, 1) = 1 + AND updated_at IS NULL + AND role_type IN ('ClusterRole','Role') + AND created_at < '2026-05-01' + AND deleted_at IS NULL; + + GET DIAGNOSTICS affected_count = ROW_COUNT; + RAISE NOTICE 'Soft deleted % duplicate external_roles rows', affected_count; END IF; +EXCEPTION + WHEN OTHERS THEN + RAISE NOTICE 'Error during external_roles cleanup: %', SQLERRM; + RAISE; END $$;This approach:
- Preserves data for audit purposes
- Avoids foreign key constraint issues (soft-deleted rows can still be referenced)
- Includes error handling and logging
- Can be reverted if needed by setting
deleted_at = NULL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@functions/external_role_duplicate_rows_fix.sql` around lines 1 - 11, Replace the hard DELETE with a soft-delete UPDATE on external_roles: set deleted_at = current_timestamp (and optionally updated_at = current_timestamp) for rows matching array_length(aliases,1) = 1, updated_at IS NULL, role_type IN ('ClusterRole','Role') and created_at < '2026-05-01'; wrap the operation in an explicit transaction block (BEGIN; ... COMMIT;) and add an EXCEPTION handler to ROLLBACK on error and RAISE or RAISE NOTICE the error for logging so failures are recoverable and auditable; keep the initial IF EXISTS (...) check to avoid running when the table is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@functions/external_role_duplicate_rows_fix.sql`:
- Line 9: The DELETE currently runs silently; modify the statement to use
RETURNING to capture deleted rows and persist observability (either by inserting
deleted row metadata into an audit table like external_role_deletions or at
minimum emitting a RAISE NOTICE with the count). Concretely, wrap the DELETE
FROM external_roles ... in a CTE or transaction, use DELETE ... RETURNING id,
aliases, role_type, created_at, updated_at to collect deleted rows, then INSERT
those rows into an audit/log table (external_role_deletions) with a timestamp
and actor, or use RAISE NOTICE to log the total deleted count; ensure the SQL in
functions/external_role_duplicate_rows_fix.sql references the same DELETE and
RETURNING logic so you can verify what was removed.
---
Nitpick comments:
In `@functions/external_role_duplicate_rows_fix.sql`:
- Around line 1-11: Replace the hard DELETE with a soft-delete UPDATE on
external_roles: set deleted_at = current_timestamp (and optionally updated_at =
current_timestamp) for rows matching array_length(aliases,1) = 1, updated_at IS
NULL, role_type IN ('ClusterRole','Role') and created_at < '2026-05-01'; wrap
the operation in an explicit transaction block (BEGIN; ... COMMIT;) and add an
EXCEPTION handler to ROLLBACK on error and RAISE or RAISE NOTICE the error for
logging so failures are recoverable and auditable; keep the initial IF EXISTS
(...) check to avoid running when the table is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ebfe151-dd8a-4718-96e4-ce182bd4fc63
📒 Files selected for processing (1)
functions/external_role_duplicate_rows_fix.sql
| WHERE schemaname = 'public' | ||
| AND tablename = 'external_roles' | ||
| ) THEN | ||
| DELETE FROM external_roles WHERE array_length(aliases, 1) = 1 AND updated_at IS NULL AND role_type in ('ClusterRole','Role') AND created_at < '2026-05-01' ; |
Contributor
There was a problem hiding this comment.
Add observability for the deletion operation.
The deletion happens silently with no logging or output. This makes it difficult to verify what was deleted, troubleshoot issues, or maintain an audit trail.
📊 Proposed fix to add logging
DO $$
+DECLARE
+ deleted_count INT;
BEGIN
IF EXISTS (
SELECT 1
FROM pg_tables
WHERE schemaname = 'public'
AND tablename = 'external_roles'
) THEN
+ -- Log rows to be deleted
+ RAISE NOTICE 'Deleting duplicate external_roles with criteria: array_length(aliases,1)=1, updated_at IS NULL, role_type IN (ClusterRole,Role), created_at < 2026-05-01';
+
+ SELECT COUNT(*) INTO deleted_count
+ FROM external_roles
+ WHERE array_length(aliases, 1) = 1
+ AND updated_at IS NULL
+ AND role_type IN ('ClusterRole','Role')
+ AND created_at < '2026-05-01';
+
+ RAISE NOTICE 'Found % rows matching deletion criteria', deleted_count;
+
DELETE FROM external_roles WHERE array_length(aliases, 1) = 1 AND updated_at IS NULL AND role_type in ('ClusterRole','Role') AND created_at < '2026-05-01' ;
+
+ RAISE NOTICE 'Successfully deleted % external_roles rows', deleted_count;
END IF;
END $$;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/external_role_duplicate_rows_fix.sql` at line 9, The DELETE
currently runs silently; modify the statement to use RETURNING to capture
deleted rows and persist observability (either by inserting deleted row metadata
into an audit table like external_role_deletions or at minimum emitting a RAISE
NOTICE with the count). Concretely, wrap the DELETE FROM external_roles ... in a
CTE or transaction, use DELETE ... RETURNING id, aliases, role_type, created_at,
updated_at to collect deleted rows, then INSERT those rows into an audit/log
table (external_role_deletions) with a timestamp and actor, or use RAISE NOTICE
to log the total deleted count; ensure the SQL in
functions/external_role_duplicate_rows_fix.sql references the same DELETE and
RETURNING logic so you can verify what was removed.
yashmehrotra
force-pushed
the
fix-ext-role
branch
from
0e661de to
1f297ed
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
2 participants
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.
Summary by CodeRabbit