DROP INVALID INDEXES command

Started by Roman Khapov26 days ago11 messageshackers
Jump to latest
#1Roman Khapov
rkhapov@yandex-team.ru

Hi hackers!

Recently Kirill and I have faced several problems with
pg_repack, that resulted in list of invalid indexes on our installations,
and that required us to remove them ourselves.

We thought that it might be useful to have a command in PG, that
removes invalid indexes on specified table automatically, to reduce
the chances of any human-related mistakes. So we implemented a new
command which usage looks like:

postgres=# drop invalid indexes on table sas;
NOTICE: dropping index "idx2"
NOTICE: dropping index "idx5"
DROP INVALID INDEXES
postgres=#

We would like to know if this feature would be acceptable
for upstream inclusion.
POC patch attached, any feedback on the design and implementation is welcome :)

--
Best regards,
Roman Khapov

Attachments:

0001-Add-DROP-INVALID-INDEXES-ON-TABLE-command.patchapplication/octet-stream; name=0001-Add-DROP-INVALID-INDEXES-ON-TABLE-command.patch; x-unix-mode=0644Download+182-3
#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Roman Khapov (#1)
Re: DROP INVALID INDEXES command

Hello,

On 2026-Jun-30, Roman Khapov wrote:

Recently Kirill and I have faced several problems with
pg_repack, that resulted in list of invalid indexes on our installations,
and that required us to remove them ourselves.

I agree that this is unpleasant behavior.

We thought that it might be useful to have a command in PG, that
removes invalid indexes on specified table automatically, to reduce
the chances of any human-related mistakes.

I think what we should really do, is implement a mechanism to drop those
invalid indexes or unlink any unreferenced files, whenever a command
fails or the system crashes while running it. We had a POC patch in
that rough direction (see [1]https://www.postgr.es/m/CAEepm%3D0ULqYgM2aFeOnrx6YrtBg3xUdxALoyCG%2BXpssKqmezug%40mail.gmail.com) -- though that was never^W^W has not yet
been completed. My point is that I'm not sure we want to introduce new
DDL syntax for something that would become obsolete in the medium term,
if that other patch ever gets completed.

[1]: https://www.postgr.es/m/CAEepm%3D0ULqYgM2aFeOnrx6YrtBg3xUdxALoyCG%2BXpssKqmezug%40mail.gmail.com

I'm not saying you should go and finish that patch, because I've been
told that that project is insanely complicated. But I'm not saying you
shouldn't, either.

On the other hand, I wouldn't be opposed to having the feature you want,
using a straight function as user interface (SELECT
pg_drop_invalid_indexes()).

Are you sure this works reasonably when invoked concurrently with CREATE
INDEX CONCURRENTLY or other concurrent DDL commands? I think the patch
should include some tests. (Probably the behavior we want is that the
code simply ignores any indexes it cannot obtain an AEL on.)

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Debido a que la velocidad de la luz es mucho mayor que la del sonido,
algunas personas nos parecen brillantes un minuto antes
de escuchar las pelotudeces que dicen." (Roberto Fontanarrosa)

#3Kirill Reshke
reshkekirill@gmail.com
In reply to: Alvaro Herrera (#2)
Re: DROP INVALID INDEXES command

Hi!

On Tue, 30 Jun 2026 at 18:23, Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

On the other hand, I wouldn't be opposed to having the feature you want,
using a straight function as user interface (SELECT
pg_drop_invalid_indexes()).

straight function as user interface is good also. I think separate
syntax for DROP INVALID INDEXES is handy for psql session <tab>
completions.
But I see that syntax extensions are much less preferable.

Are you sure this works reasonably when invoked concurrently with CREATE
INDEX CONCURRENTLY or other concurrent DDL commands? I think the patch
should include some tests. (Probably the behavior we want is that the
code simply ignores any indexes it cannot obtain an AEL on.)

Yep, it doesn't apparently, we know about this issue, but did not yet
design a solution here.

```
reshke=# reindex index concurrently z_pkey ;
ERROR: deadlock detected
DETAIL: Process 139744 waits for ShareLock on virtual transaction
1/30; blocked by process 139581.
Process 139581 waits for ShareUpdateExclusiveLock on relation 16388 of
database 16384; blocked by process 139744.
HINT: See server log for query details.
Time: 3901.507 ms (00:03.902)
reshke=#
```

We didn't think much about what behaviour is needed here, but ignoring
invalid indexes looks like a reasonable workaround for me.

--
Best regards,
Kirill Reshke

#4Kirill Reshke
reshkekirill@gmail.com
In reply to: Kirill Reshke (#3)
Re: DROP INVALID INDEXES command

On Wed, 1 Jul 2026 at 10:20, Kirill Reshke <reshkekirill@gmail.com> wrote:

Hi!

On Tue, 30 Jun 2026 at 18:23, Álvaro Herrera <alvherre@kurilemu.de> wrote:

Hello,

On the other hand, I wouldn't be opposed to having the feature you want,
using a straight function as user interface (SELECT
pg_drop_invalid_indexes()).

straight function as user interface is good also. I think separate
syntax for DROP INVALID INDEXES is handy for psql session <tab>
completions.
But I see that syntax extensions are much less preferable.

Are you sure this works reasonably when invoked concurrently with CREATE
INDEX CONCURRENTLY or other concurrent DDL commands? I think the patch
should include some tests. (Probably the behavior we want is that the
code simply ignores any indexes it cannot obtain an AEL on.)

Yep, it doesn't apparently, we know about this issue, but did not yet
design a solution here.

```
reshke=# reindex index concurrently z_pkey ;
ERROR: deadlock detected
DETAIL: Process 139744 waits for ShareLock on virtual transaction
1/30; blocked by process 139581.
Process 139581 waits for ShareUpdateExclusiveLock on relation 16388 of
database 16384; blocked by process 139744.
HINT: See server log for query details.
Time: 3901.507 ms (00:03.902)
reshke=#
```

We didn't think much about what behaviour is needed here, but ignoring
invalid indexes looks like a reasonable workaround for me.

Also, we can add a SKIP LOCKED clause to this DDL (or function
argument of pg_drop_invalid_indexes) to make this explicit.

For not SKIP LOCKED case, maybe we should behave exactly like DROP
INDEX CONCURRENTLY which is called is the same time as REINDEX/CIC?

As for test - Roman will soon post patch with basic test for this

--
Best regards,
Kirill Reshke

#5Roman Khapov
rkhapov@yandex-team.ru
In reply to: Alvaro Herrera (#2)
Re: DROP INVALID INDEXES command

Hi! Thanks for review!

I think the patch should include some tests. (Probably the behavior we want is that the
code simply ignores any indexes it cannot obtain an AEL on.)

Attached V2 with simple test, that can be extended with test for parallel REINDEX CONCURRENTLY. On our tests it really creates deadlock... so the idea of

ignoring invalid indexes looks like a reasonable workaround for me

and the syntax of SKIP LOCKED looks good for me too, can add it in
next version.

On the other hand, I wouldn't be opposed to having the feature you want,
using a straight function as user interface (SELECT
pg_drop_invalid_indexes()).

Seems like current implementation can be expanded with support of
DROP INVALID INDEXES IN DATABASE/SCHEMA (ex: for table with partitions)
maybe it is an advantage over functions, in addition to <tab> completion.

Also, seems like lock behavior it can not be implemented with functions, but currently
we have no agreement with that, so maybe it is not the reason, to do it in DDL..

--
Best regards,
Roman Khapov

Attachments:

v2-0001-Add-DROP-INVALID-INDEXES-ON-TABLE-command.patchapplication/octet-stream; name=v2-0001-Add-DROP-INVALID-INDEXES-ON-TABLE-command.patch; x-unix-mode=0644Download+258-3
#6Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Kirill Reshke (#4)
Re: DROP INVALID INDEXES command

Hi

BTW keep in mind that indexes on partitioned tables are transiently
marked invalid too. Your RangeVarGetRelid callback will (I think)
reject an attempt to call this on that kind of table, but it may be a
bit obscure, and you don't have any comments on why.

I still think that adding ad-hoc DDL syntax is the wrong amount of
intrusion.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)

#7Andrey Borodin
amborodin@acm.org
In reply to: Roman Khapov (#5)
Re: DROP INVALID INDEXES command

On 1 Jul 2026, at 17:44, Roman Khapov <rkhapov@yandex-team.ru> wrote:

<v2-0001-Add-DROP-INVALID-INDEXES-ON-TABLE-command.patch>

Hi Roman,

Thanks for working on this - the pain is real. In our case the invalid
indexes are actually left by pg_repack: they are pre-repack indexes
whose ctids are only valid against the old heap, yet we keep inserting
into them and it looks like they can even be re-validated while those
ctids point to nowhere. That is really a pg_repack problem, but it is
what forces us to drop invalid indexes - and honestly, in that scenario
we cannot avoid an external script anyway.

So cleanup is worth having. But I think Alvaro is pointing at the right
thing: the proper fix is a mechanism that removes half-created files and
objects when a command fails or a backend crashes (the undo direction in
[0]: https://www.postgr.es/m/CAEepm%3D0ULqYgM2aFeOnrx6YrtBg3xUdxALoyCG%2BXpssKqmezug%40mail.gmail.com
would make any manual "drop the leftovers" tool unnecessary.

On the interface: a function (pg_drop_invalid_indexes(...)) is the least
intrusive surface, and I would lean that way. Its one real limit is the
one you noted - a function is a single transaction, so no DROP INDEX
CONCURRENTLY, and on a hot table you are back to an AccessExclusiveLock.
New DDL looks to me like heavy, permanent grammar to prop up something we
would rather solve systemically, so I am reluctant. The only thing that
would justify a command is a hard need for non-blocking removal, and then
I would model it on REINDEX rather than invent a statement.

One detail that is the crux, not a nice-to-have: an index being built by
a live CIC is also indisvalid=false and indistinguishable from a leftover
except by the lock. So "skip what you cannot lock" must be the default,
not an opt-in SKIP LOCKED - a default that takes AEL and deadlocks (as in
your example) is wrong. Plus a dry-run and a report of what was dropped.

So my suggestion, Roman: rather than the crutch, consider aiming at the
systemic side - cleanup of not-fully-created files and objects on
failure. It is harder, but it kills the class of problem instead of one
symptom, and I am happy to help think it through. If we keep a manual
tool meanwhile, I would keep it a plain function.

Thank you!

Best regards, Andrey Borodin.

[0]: https://www.postgr.es/m/CAEepm%3D0ULqYgM2aFeOnrx6YrtBg3xUdxALoyCG%2BXpssKqmezug%40mail.gmail.com

#8Kirill Reshke
reshkekirill@gmail.com
In reply to: Alvaro Herrera (#6)
Re: DROP INVALID INDEXES command

On Thu, 2 Jul 2026 at 15:20, Álvaro Herrera <alvherre@kurilemu.de> wrote:

I still think that adding ad-hoc DDL syntax is the wrong amount of
intrusion.

Ok, lets rewrite as function API

--
Best regards,
Kirill Reshke

#9Roman Khapov
rkhapov@yandex-team.ru
In reply to: Kirill Reshke (#8)
Re: DROP INVALID INDEXES command

On 2 Jul 2026, at 19:43, Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 2 Jul 2026 at 15:20, Álvaro Herrera <alvherre@kurilemu.de> wrote:

I still think that adding ad-hoc DDL syntax is the wrong amount of
intrusion.

Ok, lets rewrite as function API

Ok, will send the v3 soon

--
Best regards,
Roman Khapov

#10Roman Khapov
rkhapov@yandex-team.ru
In reply to: Alvaro Herrera (#6)
Re: DROP INVALID INDEXES command

I still think that adding ad-hoc DDL syntax is the wrong amount of
intrusion.

Hi!

Did v3 with new function - pg_drop_invalid_indexes, for now without
support of skip locked logic, due we currently have no agreement
about it..

--
Best regards,
Roman Khapov

Attachments:

v3-0001-introduce-pg_drop_invalid_indexes.patchapplication/octet-stream; name=v3-0001-introduce-pg_drop_invalid_indexes.patch; x-unix-mode=0644Download+172-2
#11Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Roman Khapov (#10)
Re: DROP INVALID INDEXES command

Hello,

On 2026-Jul-03, Roman Khapov wrote:

Did v3 with new function - pg_drop_invalid_indexes, for now without
support of skip locked logic, due we currently have no agreement
about it..

Two questions,

1. what are you expecting to cascade on? I don't remember offhand if
anything can depend on an invalid index, but seeing how you have no
tests for that case, I am inclined to suggest removing that argument
and code altogether (so it should be DROP_RESTRICT).

2. Why do you release the AEL after looking at indisvalid? I think
whatever lock you obtain should be kept until the end of the
transaction, as is normal for all DDL. In this coding, you take AEL
on all indexes first, then unlock all indexes, and lastly acquire AEL
again for performMultipleDeletions. Seems not optimal.

In fact the logic is convoluted for no good reason; or rather the reason
seems to be that you wanted to reuse RelationGetIndexList. Maybe it'd
be better to have your own pg_index scan instead.

You seem to have done nothing about partitioned tables, as I mentioned
earlier.

What does this comment mean?

+# This pauses on the injection point while populating catcache list
+# for functions with name "foofunc"

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Right now the sectors on the hard disk run clockwise, but I heard a rumor that
you can squeeze 0.2% more throughput by running them counterclockwise.
It's worth the effort. Recommended." (Gerry Pourwelle)