DROP INDEX CONCURRENTLY on partitioned index

Hi Michael,

On 10/27/20 8:44 PM, Justin Pryzby wrote:

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.

It appears there are still some issues to be resolved with this patch,
but the next step seems to be for you to have a look at Justin's most
recent patch.

Regards,
--
-David
david@pgmasters.net

On Fri, Mar 05, 2021 at 09:27:05AM -0500, David Steele wrote:

It appears there are still some issues to be resolved with this patch, but
the next step seems to be for you to have a look at Justin's most recent
patch.

Not sure if I'll be able to do that by the end of this month. Looking
quicky at the patch, I am not much a fan of the new code path
introduced for the deletion of dependent objects in the partition
tree, so my gut is telling me that we need to think harder about the
problem at hand first.
--
Michael

On Wed, Oct 28, 2020 at 6:14 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

Forking this thread, since the existing CFs have been closed.
/messages/by-id/20200914143102.GX18552@telsasoft.com

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

On Wed, Jul 14, 2021 at 09:18:12PM +0530, vignesh C wrote:

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

I'm withdrawing this patch at least until the corresponding patch for CIC is
progressed.

https://commitfest.postgresql.org/33/2815/

--
Justin