Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 9de6a26 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR introduces two database migrations to the ENSDb SDK: one to enable PostgreSQL's trigram extension for text search capabilities, and another to establish automatic cleanup of orphaned metadata when database schemas are dropped via an event trigger. Changes
Possibly related PRs
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
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 |
Greptile SummaryThis PR adds two new Drizzle migrations to The migration comment now correctly documents the superuser requirement for event triggers. The Confidence Score: 5/5Safe to merge; the one new finding is a P2 defensive hardening suggestion that does not block correctness under the intended usage. Both migrations are functionally correct for the described use case. Prior P0/P1 concerns from earlier review rounds are addressed (superuser requirement is now documented). The only new finding is a P2 edge case where dropping the ensnode schema itself would orphan the event trigger — not a normal operational scenario and does not affect day-to-day schema cleanup. packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql — the event trigger function could silently break if the ensnode schema is ever dropped.
|
| Filename | Overview |
|---|---|
| packages/ensdb-sdk/migrations/0001_enable_ext_pg_trgm.sql | Enables the pg_trgm extension idempotently; straightforward and safe. |
| packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql | Adds an event trigger for schema-drop cleanup; function logic is correct, but if the ensnode schema itself is ever dropped the trigger becomes permanently broken for all future DROP SCHEMA commands. |
| packages/ensdb-sdk/migrations/meta/_journal.json | Journal correctly registers both new migrations with sequential indices and matching tags. |
| packages/ensdb-sdk/migrations/meta/0001_snapshot.json | Snapshot correctly reflects no schema-level changes (extension install is invisible to Drizzle's schema tracker). |
| packages/ensdb-sdk/migrations/meta/0002_snapshot.json | Snapshot correctly reflects no schema-level changes (event trigger and function are invisible to Drizzle's schema tracker). |
Sequence Diagram
sequenceDiagram
participant Client
participant PostgreSQL
participant EventTrigger as Event Trigger cleanup_ensnode_metadata_on_schema_drop
participant Function as ensnode.handle_schema_dropped()
participant Metadata as ensnode.metadata
Client->>PostgreSQL: DROP SCHEMA abc2 [CASCADE]
PostgreSQL->>PostgreSQL: Execute schema drop
PostgreSQL->>EventTrigger: Fire sql_drop (TAG = DROP SCHEMA)
EventTrigger->>Function: EXECUTE FUNCTION
Function->>PostgreSQL: pg_event_trigger_dropped_objects()
PostgreSQL-->>Function: object_type=schema, object_name=abc2
Function->>Metadata: DELETE WHERE ens_indexer_schema_name = 'abc2'
Metadata-->>Function: rows deleted
Function-->>EventTrigger: done
EventTrigger-->>PostgreSQL: trigger complete
PostgreSQL-->>Client: DROP SCHEMA succeeded
Reviews (2): Last reviewed commit: "docs(changeset): Added `manage_orphaned_..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop | ||
| ON sql_drop | ||
| WHEN TAG IN ('DROP SCHEMA') | ||
| EXECUTE FUNCTION handle_schema_dropped(); |
There was a problem hiding this comment.
CREATE EVENT TRIGGER requires superuser privileges
In PostgreSQL, event triggers can only be created by a superuser. If the database user running these migrations does not hold the SUPERUSER role — which is common on managed providers like AWS RDS, Neon, or Supabase — this migration will fail with ERROR: permission denied to create event trigger. The function definition on line 3 will succeed, but the trigger creation will abort the migration. Consider either documenting this requirement explicitly, providing a fallback, or checking whether the target deployment always runs as a superuser-equivalent role.
| CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop | ||
| ON sql_drop | ||
| WHEN TAG IN ('DROP SCHEMA') | ||
| EXECUTE FUNCTION handle_schema_dropped(); |
There was a problem hiding this comment.
The function uses CREATE OR REPLACE FUNCTION, but the event trigger itself uses a bare CREATE EVENT TRIGGER with no IF NOT EXISTS. PostgreSQL does not support OR REPLACE for event triggers before version 14 and never supports IF NOT EXISTS. If this migration is ever re-applied manually (e.g., disaster-recovery replay), it will fail with ERROR: event trigger "cleanup_ensnode_metadata_on_schema_drop" already exists. A DO $$ BEGIN ... EXCEPTION WHEN duplicate_object THEN NULL; END $$; guard around the trigger creation would make it resilient.
There was a problem hiding this comment.
Pull request overview
This PR extends the @ensnode/ensdb-sdk Drizzle migration set to (1) enable the pg_trgm Postgres extension and (2) add an event trigger that cleans up ensnode.metadata rows when an ENSIndexer schema is dropped, preventing orphaned metadata.
Changes:
- Add a migration to enable
pg_trgm. - Add an event-trigger-based cleanup migration for orphaned
ensnode.metadatarecords onDROP SCHEMA. - Update Drizzle migration metadata (
_journal.json+ snapshots) to include the new migrations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensdb-sdk/migrations/0001_enable_ext_pg_trgm.sql | Enables pg_trgm extension for trigram indexing/search. |
| packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql | Adds event trigger + handler function to delete metadata rows when schemas are dropped. |
| packages/ensdb-sdk/migrations/meta/_journal.json | Registers the two new migrations in Drizzle’s journal. |
| packages/ensdb-sdk/migrations/meta/0001_snapshot.json | Snapshot metadata for migration 0001. |
| packages/ensdb-sdk/migrations/meta/0002_snapshot.json | Snapshot metadata for migration 0002. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql
Outdated
Show resolved
Hide resolved
packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ensdb-sdk/migrations/0001_enable_ext_pg_trgm.sql`:
- Around line 1-3: The comment above the migration contains a minor typo
("PostgreSQL.g"); edit the SQL file comment so the sentence ends with a normal
period. Locate the top comment block where the CREATE EXTENSION IF NOT EXISTS
pg_trgm; statement appears and replace "PostgreSQL.g" with "PostgreSQL." to
remove the stray 'g'.
In `@packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql`:
- Around line 18-21: Ensure creation of the event trigger
cleanup_ensnode_metadata_on_schema_drop is idempotent by preceding the CREATE
EVENT TRIGGER call with a DROP EVENT TRIGGER IF EXISTS
cleanup_ensnode_metadata_on_schema_drop CASCADE; so re-running migrations or
recovering from a corrupted migrations table won’t fail, and add a note in
deployment/setup docs that creating event triggers (e.g.,
cleanup_ensnode_metadata_on_schema_drop which EXECUTE FUNCTION
handle_schema_dropped()) requires PostgreSQL superuser privileges so the DB role
running migrations must be superuser.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 50756160-ec47-442a-8ad0-d29e445c9c0b
📒 Files selected for processing (7)
.changeset/metal-donkeys-bake.md.changeset/six-garlics-wish.mdpackages/ensdb-sdk/migrations/0001_enable_ext_pg_trgm.sqlpackages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sqlpackages/ensdb-sdk/migrations/meta/0001_snapshot.jsonpackages/ensdb-sdk/migrations/meta/0002_snapshot.jsonpackages/ensdb-sdk/migrations/meta/_journal.json
| CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop | ||
| ON sql_drop | ||
| WHEN TAG IN ('DROP SCHEMA') | ||
| EXECUTE FUNCTION handle_schema_dropped(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does PostgreSQL CREATE EVENT TRIGGER support IF NOT EXISTS?
💡 Result:
No. PostgreSQL’s CREATE EVENT TRIGGER syntax does not include IF NOT EXISTS (nor OR REPLACE), so attempting CREATE EVENT TRIGGER IF NOT EXISTS ... is a syntax error. [1]
Workarounds:
- Check the system catalog first (
pg_catalog.pg_event_trigger) and only create if missing. [2] - Or wrap the
CREATE EVENT TRIGGERin aDOblock and ignore theduplicate_objectexception. - If your goal is idempotency in migrations,
DROP EVENT TRIGGER IF EXISTS ...is supported. [3]
Sources: [1] [2] [3]
🏁 Script executed:
# Find the migrations directory and other migration files
fd -e sql . packages/ensdb-sdk/migrations/ | head -20Repository: namehash/ensnode
Length of output: 251
🏁 Script executed:
# Check the migration file to see the full context
cat -n packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sqlRepository: namehash/ensnode
Length of output: 858
🏁 Script executed:
# Search for Drizzle configuration files
fd -e config -e json -e ts | grep -i drizzle | head -10Repository: namehash/ensnode
Length of output: 217
🏁 Script executed:
# Search for other event trigger creations in the codebase
rg "CREATE EVENT TRIGGER" --type sqlRepository: namehash/ensnode
Length of output: 192
🏁 Script executed:
# Search for database role/permission setup documentation
fd -e md | xargs grep -l "privilege\|superuser\|role" 2>/dev/null | head -5Repository: namehash/ensnode
Length of output: 42
🏁 Script executed:
# Check if there are any other migrations that handle similar idempotency concerns
rg "DROP.*IF EXISTS" packages/ensdb-sdk/migrations/ --type sqlRepository: namehash/ensnode
Length of output: 42
🏁 Script executed:
# Check drizzle configuration
cat packages/ensdb-sdk/drizzle.config.tsRepository: namehash/ensnode
Length of output: 290
🏁 Script executed:
# Check drizzle test file to see migration patterns
cat packages/ensdb-sdk/src/lib/drizzle.test.tsRepository: namehash/ensnode
Length of output: 7929
🏁 Script executed:
# Search for README or setup documentation
fd -e md . | head -20Repository: namehash/ensnode
Length of output: 688
🏁 Script executed:
# Look for Docker or environment configuration
fd -e dockerfile -e docker-compose -o -name ".env*" -o -name "*.env" | head -10Repository: namehash/ensnode
Length of output: 290
🏁 Script executed:
# Search for any documentation about database setup or roles
rg "superuser|privilege|role|permission" --type md | head -20Repository: namehash/ensnode
Length of output: 42
🏁 Script executed:
# Check if migrations can be re-run and what happens
rg "migration" --type ts packages/ensdb-sdk/src/ | head -15Repository: namehash/ensnode
Length of output: 1936
🏁 Script executed:
# Check the ensdb-writer implementation for migration execution
cat packages/ensdb-sdk/src/client/ensdb-writer.ts | head -100Repository: namehash/ensnode
Length of output: 2876
🏁 Script executed:
# Look for migration execution utilities
rg "runMigrations|migrate\(" --type ts packages/ensdb-sdk/src/Repository: namehash/ensnode
Length of output: 146
🏁 Script executed:
# Check the main README for database setup instructions
head -100 README.mdRepository: namehash/ensnode
Length of output: 5155
🏁 Script executed:
# Search for any migration runner or database setup documentation
fd -e ts packages/ensdb-sdk/src/client/ | xargs grep -l "migration" 2>/dev/nullRepository: namehash/ensnode
Length of output: 487
🏁 Script executed:
# Look for any scripts or documentation about database initialization
fd -e sh -o -e md | xargs grep -l "migrate\|migration" 2>/dev/null | head -10Repository: namehash/ensnode
Length of output: 176
🏁 Script executed:
# Check if there's any documentation about database setup or privileges
cat README.md | grep -A 20 -B 5 "database\|privilege\|role\|setup" | head -50Repository: namehash/ensnode
Length of output: 42
🏁 Script executed:
# Look for any deployment or infrastructure documentation
fd -e md apps/ensindexer/ | head -5 | xargs cat | grep -A 10 -B 5 "database\|privilege\|migration" | head -30Repository: namehash/ensnode
Length of output: 429
🏁 Script executed:
# Search for any documentation about running migrations
rg "migrateEnsNodeSchema\|migrate" --type md | head -20Repository: namehash/ensnode
Length of output: 42
🏁 Script executed:
# Check if there's any setup documentation in the ensdb-sdk package
fd -e md packages/ensdb-sdk/ | xargs cat 2>/dev/null | head -100Repository: namehash/ensnode
Length of output: 441
🏁 Script executed:
# Look for any environment or deployment configuration
fd -e env -o -e yaml -o -e yml | head -10Repository: namehash/ensnode
Length of output: 176
🏁 Script executed:
# Check docker configuration if it exists for database setup hints
fd -i dockerfileRepository: namehash/ensnode
Length of output: 161
🏁 Script executed:
# Check one of the Dockerfiles for database setup hints
cat apps/ensindexer/Dockerfile | head -80Repository: namehash/ensnode
Length of output: 480
🏁 Script executed:
# Look for any scripts that set up the database
fd . . -type f -name "*setup*" -o -name "*init*" | grep -i sqlRepository: namehash/ensnode
Length of output: 230
🏁 Script executed:
# Search for documentation about how migrations are tracked
rg "migrations" --type md apps/ensindexer/ | head -20Repository: namehash/ensnode
Length of output: 365
🏁 Script executed:
# Look for any PostgreSQL initialization scripts
fd . . -type f -name "*init*.sql" -o -name "*setup*.sql"Repository: namehash/ensnode
Length of output: 230
🏁 Script executed:
# Check for environment-specific configuration
fd -e json -e config.ts -o -e config.js | head -10Repository: namehash/ensnode
Length of output: 176
🏁 Script executed:
# Search for any comments about database requirements or setup
rg "superuser\|CREATE EVENT TRIGGER\|privilege" --type ts --type sqlRepository: namehash/ensnode
Length of output: 42
Consider event trigger idempotency in recovery scenarios and document superuser privilege requirement.
Two points to address:
-
Idempotency: While Drizzle tracks executed migrations,
CREATE EVENT TRIGGERlacksIF NOT EXISTSsupport. If migrations are manually re-run or the migration tracking table is corrupted, creation will fail. For robustness, consider usingDROP EVENT TRIGGER IF EXISTS cleanup_ensnode_metadata_on_schema_drop CASCADE;before theCREATE EVENT TRIGGERstatement to ensure recovery scenarios don't fail. -
Privilege requirements: Creating event triggers requires PostgreSQL superuser privileges. Document this requirement in deployment/setup documentation so operators know the database role running migrations must have superuser access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql`
around lines 18 - 21, Ensure creation of the event trigger
cleanup_ensnode_metadata_on_schema_drop is idempotent by preceding the CREATE
EVENT TRIGGER call with a DROP EVENT TRIGGER IF EXISTS
cleanup_ensnode_metadata_on_schema_drop CASCADE; so re-running migrations or
recovering from a corrupted migrations table won’t fail, and add a note in
deployment/setup docs that creating event triggers (e.g.,
cleanup_ensnode_metadata_on_schema_drop which EXECUTE FUNCTION
handle_schema_dropped()) requires PostgreSQL superuser privileges so the DB role
running migrations must be superuser.
Clean up records referencing the schema being deleted.
tk-o
force-pushed
the
feat/extend-ensdb-migrations
branch
from
68d5926 to
9de6a26
Compare
|
@greptile review |
|
Tip: Greploops — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop | ||
| ON sql_drop | ||
| WHEN TAG IN ('DROP SCHEMA') | ||
| EXECUTE FUNCTION "ensnode"."handle_schema_dropped"(); |
There was a problem hiding this comment.
| CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop | |
| ON sql_drop | |
| WHEN TAG IN ('DROP SCHEMA') | |
| EXECUTE FUNCTION "ensnode"."handle_schema_dropped"(); | |
| DO $ | |
| BEGIN | |
| CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop | |
| ON sql_drop | |
| WHEN TAG IN ('DROP SCHEMA') | |
| EXECUTE FUNCTION "ensnode"."handle_schema_dropped"(); | |
| EXCEPTION WHEN insufficient_privilege THEN | |
| -- Event trigger creation requires superuser privileges. | |
| -- This is expected in managed PostgreSQL environments. | |
| RAISE NOTICE 'Event trigger "cleanup_ensnode_metadata_on_schema_drop" could not be created due to insufficient privileges. This feature requires superuser access.'; | |
| END $; |
CREATE EVENT TRIGGER requires superuser privileges in PostgreSQL, causing migrations to fail in managed database environments that don't grant superuser access to application users
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
pg_trgmextension.Why
Testing
ENSINDEXER_SCHEMA_NAME=abc1 pnpm -F ensindexer startENSINDEXER_SCHEMA_NAME=abc2 pnpm -F ensindexer startENSINDEXER_SCHEMA_NAME=abc3 pnpm -F ensindexer startabc2schemaNotes for Reviewer (Optional)
Pre-Review Checklist (Blocking)