Re: DROP INDEX CONCURRENTLY on partitioned index

On Wed, Oct 28, 2020 at 6:14 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> 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.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh