On Wed, Oct 28, 2020 at 6:14 AM Justin Pryzby <pry...@telsasoft.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
