Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: e0e4e62 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 |
📝 WalkthroughWalkthroughAdds two ENSDb SDK database migrations: one enables PostgreSQL's pg_trgm extension, the other installs a PL/pgSQL event trigger that removes orphaned rows from Changes
Sequence Diagram(s)sequenceDiagram
participant User as DDL Executor
participant Migrator as Migration Runner
participant Postgres as PostgreSQL
participant Function as ensnode.handle_schema_dropped()
participant Metadata as ensnode.metadata
User->>Migrator: apply migrations
Migrator->>Postgres: execute 0001_enable_ext_pg_trgm.sql
Migrator->>Postgres: execute 0002_manage_orphaned_ensnode_metadata.sql
note right of Postgres: creates function & event trigger
User->>Postgres: DROP SCHEMA some_schema;
Postgres->>Function: fire event trigger (sql_drop)
Function->>Metadata: DELETE FROM ensnode.metadata WHERE ens_indexer_schema_name = 'some_schema';
Function-->>Postgres: return
Postgres-->>User: statement completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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 Drizzle migrations to
Confidence Score: 4/5Safe to merge if superuser access is guaranteed in all target deployments; otherwise the cleanup feature will silently not install with no retry path. Migration 0001 is clean and idempotent. Migration 0002 correctly addresses the privilege-failure mode with a DO block, but the RAISE NOTICE silent-success path means Drizzle permanently marks the migration done even when the trigger was never created — a real P1 for deployments on managed providers. Score reflects one confirmed P1 blocking concern. packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql
|
| Filename | Overview |
|---|---|
| packages/ensdb-sdk/migrations/0001_enable_ext_pg_trgm.sql | Idempotent CREATE EXTENSION IF NOT EXISTS pg_trgm; safe for PostgreSQL 13+ where pg_trgm is a trusted extension. |
| packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql | Adds event trigger for orphan cleanup; privilege exception is now caught gracefully, but silent success means Drizzle marks the migration done even when the trigger was never installed — cleanup feature permanently absent in non-superuser environments with no actionable signal. |
| packages/ensdb-sdk/migrations/meta/_journal.json | Journal correctly registers two new migration entries (idx 1 and 2) in sequential order. |
| packages/ensdb-sdk/migrations/meta/0001_snapshot.json | Drizzle snapshot for migration 0001; table schema unchanged as expected for an extension-only migration. |
| packages/ensdb-sdk/migrations/meta/0002_snapshot.json | Drizzle snapshot for migration 0002; table schema unchanged, as function/trigger objects are not tracked in Drizzle snapshots. |
Sequence Diagram
sequenceDiagram
participant D as Drizzle Migrator
participant PG as PostgreSQL
participant ET as Event Trigger
D->>PG: Run 0001: CREATE EXTENSION IF NOT EXISTS pg_trgm
PG-->>D: OK
D->>PG: Run 0002: CREATE OR REPLACE FUNCTION handle_schema_dropped()
PG-->>D: OK
D->>PG: DO $$ BEGIN CREATE EVENT TRIGGER ... END $$
alt Superuser environment
PG-->>D: OK (trigger installed)
Note over ET: cleanup_ensnode_metadata_on_schema_drop active
else Non-superuser environment
PG-->>D: RAISE NOTICE (insufficient_privilege caught)
Note over ET: Trigger NOT installed — silent absence
end
Note over D: Migration marked as applied in both cases
rect rgb(200, 230, 200)
Note over ET,PG: On DROP SCHEMA (superuser path only)
PG->>ET: sql_drop event fired
ET->>PG: DELETE FROM ensnode.metadata WHERE ens_indexer_schema_name = dropped_schema
end
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql
Outdated
Show resolved
Hide resolved
| 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.
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}
packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql (1)
20-23:⚠️ Potential issue | 🟠 MajorHarden event-trigger creation for reruns and restricted DB roles.
On Line 20, unconditional
CREATE EVENT TRIGGERcan abort migrations in recovery/rerun scenarios and in environments where the migration role lacks superuser privileges. Consider making creation guarded and handling privilege errors explicitly.🛠️ Proposed hardening patch
-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 + IF NOT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_event_trigger + WHERE evtname = 'cleanup_ensnode_metadata_on_schema_drop' + ) THEN + CREATE EVENT TRIGGER cleanup_ensnode_metadata_on_schema_drop + ON sql_drop + WHEN TAG IN ('DROP SCHEMA') + EXECUTE FUNCTION "ensnode"."handle_schema_dropped"(); + END IF; +EXCEPTION + WHEN insufficient_privilege THEN + RAISE NOTICE 'Skipping cleanup_ensnode_metadata_on_schema_drop: superuser privileges required.'; +END +$$;PostgreSQL documentation check: Does CREATE EVENT TRIGGER support IF NOT EXISTS, and what privileges are required to create an event trigger?🤖 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 20 - 23, The unconditional CREATE EVENT TRIGGER for cleanup_ensnode_metadata_on_schema_drop should be made idempotent and resilient to privilege errors: check pg_event_trigger (or pg_event_trigger.evtevent) for an existing trigger named cleanup_ensnode_metadata_on_schema_drop and only create it if absent, and wrap the creation in a DO block that catches exceptions such as insufficient_privilege or duplicate_object so a rerun or restricted DB role won’t abort the migration; reference the event trigger name cleanup_ensnode_metadata_on_schema_drop and the target function "ensnode"."handle_schema_dropped" when implementing the existence check and the exception handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql`:
- Around line 20-23: The unconditional CREATE EVENT TRIGGER for
cleanup_ensnode_metadata_on_schema_drop should be made idempotent and resilient
to privilege errors: check pg_event_trigger (or pg_event_trigger.evtevent) for
an existing trigger named cleanup_ensnode_metadata_on_schema_drop and only
create it if absent, and wrap the creation in a DO block that catches exceptions
such as insufficient_privilege or duplicate_object so a rerun or restricted DB
role won’t abort the migration; reference the event trigger name
cleanup_ensnode_metadata_on_schema_drop and the target function
"ensnode"."handle_schema_dropped" when implementing the existence check and the
exception handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cab53b0-a548-4a33-afad-35e79e9a19ba
📒 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 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
Clean up records referencing the schema being deleted.
tk-o
force-pushed
the
feat/extend-ensdb-migrations
branch
from
967396b to
e0e4e62
Compare
|
@greptile review |
| 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; |
There was a problem hiding this comment.
When insufficient_privilege is caught, the DO block exits cleanly with only a RAISE NOTICE. Drizzle records this migration as applied and will never re-run it. In non-superuser managed environments, ensnode.handle_schema_dropped is installed but the event trigger is never wired up — orphan cleanup is silently non-functional with no mechanism to retry short of a manual SQL intervention.
Consider replacing RAISE NOTICE with RAISE WARNING and adding a post-migration check (or logging at application start) so operators know this feature is inactive. Alternatively, document an upgrade path (e.g., a separate idempotent script) for when superuser access becomes available later.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql (1)
29-43:⚠️ Potential issue | 🟡 MinorAdd
DROP EVENT TRIGGER IF EXISTSfor idempotency in recovery scenarios.The function uses
CREATE OR REPLACE(idempotent), butCREATE EVENT TRIGGERwill fail with aduplicate_objecterror if the trigger already exists. While Drizzle tracks executed migrations, recovery scenarios (manual re-runs, corrupted tracking table) could fail. The current exception handler only catchesinsufficient_privilege, notduplicate_object.🛠️ Proposed fix
DO $$ BEGIN + DROP EVENT TRIGGER IF EXISTS cleanup_ensnode_metadata_on_schema_drop; + 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; $$;🤖 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 29 - 43, Make the migration idempotent by ensuring the event trigger is removed before attempting to create it: invoke DROP EVENT TRIGGER IF EXISTS cleanup_ensnode_metadata_on_schema_drop; prior to the CREATE EVENT TRIGGER statement (which references the function "ensnode"."handle_schema_dropped"), and/or extend the EXCEPTION block to also handle duplicate_object so a pre-existing trigger doesn't cause the migration to fail in recovery scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/ensdb-sdk/migrations/0002_manage_orphaned_ensnode_metadata.sql`:
- Around line 29-43: Make the migration idempotent by ensuring the event trigger
is removed before attempting to create it: invoke DROP EVENT TRIGGER IF EXISTS
cleanup_ensnode_metadata_on_schema_drop; prior to the CREATE EVENT TRIGGER
statement (which references the function "ensnode"."handle_schema_dropped"),
and/or extend the EXCEPTION block to also handle duplicate_object so a
pre-existing trigger doesn't cause the migration to fail in recovery scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bcc9d4e3-9864-4c3d-b215-6013f5cae2e5
📒 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
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Directionally this looks nice but we should enhance the execution of the idea
| @@ -0,0 +1,43 @@ | |||
| -- This migration adds an event trigger to handle cleanup of orphaned records | |||
| -- in the `ensnode.metadata` table when an ENSIndexer Schema is dropped. | |||
There was a problem hiding this comment.
I note that this triggers on any schema that is not the ENSNode schema. We should document this more precisely.
| @@ -0,0 +1,43 @@ | |||
| -- This migration adds an event trigger to handle cleanup of orphaned records | |||
| -- in the `ensnode.metadata` table when an ENSIndexer Schema is dropped. | |||
| -- Note: Running this migration requires superuser privileges due to | |||
There was a problem hiding this comment.
We should explicitly document what the consequence is of running this migration without superuser priviliges.
| @@ -0,0 +1,3 @@ | |||
| -- This migration enables the pg_trgm extension, which is used for | |||
| -- trigram-based indexing and searching in PostgreSQL. | |||
| CREATE EXTENSION IF NOT EXISTS pg_trgm; | |||
There was a problem hiding this comment.
Does this action also require special priviliges?
|
@lightwalker-eth thanks for sharing feedback 👍 I now have concerns about the direction, though. The superuser privilege, in our case, is required to track schemas being dropped. I looked more into what superuser privileges look like across database provider platforms. Claude says multiple providers won’t support that direction as they don’t grant the superuser privileges for real.
We need to follow the principle to avoid using strategies that many people won't be able to access or that create misalignments between how ENSNode works for different people. Therefor, we won't implement the automated cleanup for orphaned records in ENSNode Metadata table on the ENSDb instance level. |
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)