DROP INDEX CONCURRENTLY on partitioned index

Forking this thread, since the existing CFs have been closed.
https://www.postgresql.org/message-id/flat/20200914143102.GX18552%40telsasoft.com#58b1056488451f8594b0f0ba40996afd

On Mon, Sep 14, 2020 at 09:31:03AM -0500, Justin Pryzby wrote:
> On Sat, Sep 12, 2020 at 10:35:34AM +0900, Michael Paquier wrote:
> > On Fri, Sep 11, 2020 at 07:13:01PM -0500, Justin Pryzby wrote:
> > > On Tue, Sep 08, 2020 at 01:31:05PM +0900, Michael Paquier wrote:
> > >> - CIC on partitioned relations. (Should we also care about DROP INDEX
> > >> CONCURRENTLY as well?)
> > >
> > > Do you have any idea what you think that should look like for DROP INDEX
> > > CONCURRENTLY ?
> >
> > Making the maintenance of the partition tree consistent to the user is
> > the critical part here, so my guess on this matter is:
> > 1) Remove each index from the partition tree and mark the indexes as
> > invalid in the same transaction. This makes sure that after commit no
> > indexes would get used for scans, and the partition dependency tree
> > pis completely removed with the parent table. That's close to what we
> > do in index_concurrently_swap() except that we just want to remove the
> > dependencies with the partitions, and not just swap them of course.
> > 2) Switch each index to INDEX_DROP_SET_DEAD, one per transaction
> > should be fine as that prevents inserts.
> > 3) Finish the index drop.
> >
> > Step 2) and 3) could be completely done for each index as part of
> > index_drop(). The tricky part is to integrate 1) cleanly within the
> > existing dependency machinery while still knowing about the list of
> > partitions that can be removed. I think that this part is not that
> > straight-forward, but perhaps we could just make this logic part of
> > RemoveRelations() when listing all the objects to remove.
>
> Thanks.
>
> I see three implementation ideas..
>
> 1. I think your way has an issue that the dependencies are lost. If there's an
> interruption, the user is maybe left with hundreds or thousands of detached
> indexes to clean up. This is strange since there's actually no detach command
> for indexes (but they're implicitly "attached" when a matching parent index is
> created). A 2nd issue is that DETACH currently requires an exclusive lock (but
> see Alvaro's WIP patch).

I think this is a critical problem with this approach. It's not ok if a
failure leaves behind N partition indexes not attached to any parent.
They may have pretty different names, which is a mess to clean up.

> 2. Maybe the easiest way is to mark all indexes invalid and then drop all
> partitions (concurrently) and then the partitioned parent. If interrupted,
> this would leave a parent index marked "invalid", and some child tables with no
> indexes. I think this may be "ok". The same thing is possible if a concurrent
> build is interrupted, right ?

I think adding the "invalid" mark in the simple/naive way isn't enough - it has
to do everything DROP INDEX CONCURRENTLY does (of course).

> 3. I have a patch which changes index_drop() to "expand" a partitioned index into
> its list of children. Each of these becomes a List:
> | indexId, heapId, userIndexRelation, userHeapRelation, heaplocktag, heaprelid, indexrelid
> The same process is followed as for a single index, but handling all partitions
> at once in two transactions total. Arguably, this is bad since that function
> currently takes a single Oid but would now ends up operating on a list of indexes.

This is what's implemented in the attached. It's very possible I've missed
opportunities for better simplification/integration.

--
Justin