cleanup invalid indexes before reindex - #66
Conversation
WalkthroughAdds a pre-step in the reindex_btree_loop to run a new SQL script that drops invalid indexes before executing the existing reindex script. Updates the btree bloat SQL to select distinct table names and sort by the first column. Introduces a PL/pgSQL script to conditionally drop invalid indexes with non-blocking locks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Make as Make reindex_btree_loop
participant Psql as psql
participant SQL as drop_invalid_indexes.sql
participant ReIdx as reindex-bloated-btrees.sh
Make->>Psql: Run scripts/drop_invalid_indexes.sql
Psql->>SQL: Execute DO block
Note over SQL: For each invalid index:<br/>TRY lock parent table NOWAIT<br/>IF lock ok → DROP INDEX<br/>ELSE → LOG skipped
Make->>ReIdx: Run scripts/reindex-bloated-btrees.sh
sequenceDiagram
autonumber
participant SQL as drop_invalid_indexes.sql
participant Catalog as pg_catalog
loop Each invalid index
SQL->>Catalog: SELECT invalid indexes (indisvalid = false)
SQL->>Catalog: LOCK TABLE ... ACCESS EXCLUSIVE NOWAIT
alt Lock acquired
SQL->>Catalog: DROP INDEX CONCURRENTLY? (DROP INDEX)
Note right of SQL: NOTICE: Dropped invalid index
else Lock not available
Note right of SQL: NOTICE: Skipped (table busy)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)scripts/btree_bloat-superuser.sql(2 hunks)scripts/drop_invalid_indexes.sql(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
Makefile
📄 CodeRabbit inference engine (AGENTS.md)
Makefile: If you need intermediate result from other target in Makefile, split it into two and depend on the intermediate result.
Format target comments as self-documented Makefile, on same line:target: dependencies | order_only_deps ## Description
Remember that Makefile uses tabs.
Explain high-level architecture and quirks in Makefile.
Makefile: there are comments on the same line after each target separated by ## - they are used in debug graph visualization, need to be concise and descriptive of what's going on in the code itself.
Files:
Makefile
**/*.sql
📄 CodeRabbit inference engine (AGENTS.md)
**/*.sql: Format SQL queries in a way so it's easy to copy them out of the codebase and debug standalone.
SQL is lowercase, PostGIS functions follow their spelling from the manual (st_segmentize->ST_Segmentize).
SQL files should be idempotent: drop table if exists; add some comments to make people grasp queries faster.
Prefer indexed SQL operators when dealing with jsonb (tags @> '{"key": "value"}'instead oftags ->> 'key' = 'value').
Files:
scripts/drop_invalid_indexes.sqlscripts/btree_bloat-superuser.sql
🔇 Additional comments (2)
scripts/btree_bloat-superuser.sql (1)
28-105: Output deduping looks good.Selecting distinct table names and ordering on the first column makes the downstream loop deterministic without altering the underlying bloat detection.
Makefile (1)
30-31: Pre-step integration LGTM.Running the cleanup script ahead of the reindex loop tightens the maintenance flow without altering existing behavior.
| DO $$ | ||
| DECLARE | ||
| idx RECORD; | ||
| BEGIN | ||
| FOR idx IN | ||
| SELECT n.nspname AS schema_name, | ||
| c.relname AS index_name, | ||
| t.oid AS table_oid, | ||
| t.relname AS table_name | ||
| FROM pg_index i | ||
| JOIN pg_class c ON c.oid = i.indexrelid | ||
| JOIN pg_namespace n ON n.oid = c.relnamespace | ||
| JOIN pg_class t ON t.oid = i.indrelid | ||
| WHERE NOT i.indisvalid | ||
| LOOP | ||
| BEGIN | ||
| -- try to take an ACCESS EXCLUSIVE lock on the table, without waiting | ||
| EXECUTE format('LOCK TABLE %I.%I IN ACCESS EXCLUSIVE MODE NOWAIT', | ||
| idx.schema_name, idx.table_name); | ||
|
|
||
| -- if lock succeeds, drop index | ||
| RAISE NOTICE 'Dropping invalid index: %.%', idx.schema_name, idx.index_name; | ||
| EXECUTE format('DROP INDEX IF EXISTS %I.%I;', idx.schema_name, idx.index_name); | ||
|
|
||
| EXCEPTION WHEN lock_not_available THEN | ||
| RAISE NOTICE 'Skipped index %.% (table %.% is busy)', | ||
| idx.schema_name, idx.index_name, idx.schema_name, idx.table_name; | ||
| END; | ||
| END LOOP; | ||
| END; | ||
| $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Follow repository SQL casing conventions.
Per the project SQL guidelines, keywords should be lowercase. Please downcase DO, DECLARE, SELECT, LOCK, DROP, etc., in this script for consistency.
🤖 Prompt for AI Agents
In scripts/drop_invalid_indexes.sql around lines 1 to 31, the SQL script uses
uppercase SQL keywords; update the file to follow repository SQL casing
conventions by converting all SQL keywords (e.g., DO, DECLARE, BEGIN, FOR,
SELECT, JOIN, WHERE, LOOP, EXECUTE, FORMAT, RAISE NOTICE, EXCEPTION WHEN, END,
LANGUAGE, etc.) to lowercase while leaving identifiers (schema/table/index
names), string literals, and punctuation unchanged; ensure formatting and
indentation remain the same and verify the script still runs after the casing
changes.
| -- if lock succeeds, drop index | ||
| RAISE NOTICE 'Dropping invalid index: %.%', idx.schema_name, idx.index_name; | ||
| EXECUTE format('DROP INDEX IF EXISTS %I.%I;', idx.schema_name, idx.index_name); | ||
|
|
||
| EXCEPTION WHEN lock_not_available THEN | ||
| RAISE NOTICE 'Skipped index %.% (table %.% is busy)', | ||
| idx.schema_name, idx.index_name, idx.schema_name, idx.table_name; |
There was a problem hiding this comment.
Abort risk when invalid index backs a constraint.
DROP INDEX raises SQLSTATE 2BP01 (dependent_objects_still_exist) for primary-key/unique indexes that back constraints. Because the DO block doesn’t trap it, the first such index aborts the whole script and your Make target fails, leaving the reindex loop untouched. Please skip (or explicitly handle) indexes referenced by pg_constraint before issuing DROP INDEX.
Apply a guard in the index selection, e.g.:
- WHERE NOT i.indisvalid
+ WHERE NOT i.indisvalid
+ AND NOT EXISTS (
+ SELECT 1
+ FROM pg_constraint c
+ WHERE c.conindid = i.indexrelid
+ )Committable suggestion skipped: line range outside the PR's diff.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores