Обсуждение: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.

The following bug has been logged on the website:

Bug reference:      16594
Logged by:          Jan Mussler
Email address:      jan.mussler@zalando.de
PostgreSQL version: 12.3
Operating system:   Ubuntu
Description:

Hello everyone,

earlier today we observed a user trying to drop an index concurrently
yielding the following error message:

DROP INDEX CONCURRENTLY IF EXISTS some_idx;
ERROR:  DROP INDEX CONCURRENTLY must be first action in transaction

Continuing down this road the user was asked double check what was going on
and try with a rollback first, yielding the following example output:

=> rollback;
WARNING:  there is no transaction in progress
ROLLBACK
=> DROP INDEX CONCURRENTLY IF EXISTS some_idx;
ERROR:  DROP INDEX CONCURRENTLY must be first action in transaction
=>

After more digging the user elaborated that this is in fact a partitioned
table.

While a person with deeper knowledge may understand that there is more going
on when dropping an index on a partitioned table from a normal user
perspective this error message is confusing.

We have some internal discussion if this is a bug or lack of documentation.
I am siding though with bug believing the otherwise excellent Postgres error
messages can be more helpful here.

Open for feedback and we can also see to contribute to the docs if needed.

-- Jan

On Wed, Aug 26, 2020 at 08:11:16PM +0000, PG Bug reporting form wrote:
> After more digging the user elaborated that this is in fact a partitioned
> table.

This error happens in index_drop() as of index.c, where we need to
assume that the index drop is the first action happening in a
transaction.  Now comes the tricky problem: when you drop the index of
a partitioned table, the dependency machinery needs to find the set of
dependent objects, and this assigns a transaction ID for the list of
partitions.

Still, it seems to me that we are far from being able to support fully
this operation on partitioned indexes, and we need to do more than
what we have now if we want this feature.  So I think that the
concurrent drop of partitioned indexes should respect the following
flow:
1) Drop all the dependencies for the partition tree in a single,
first, transaction.
2) Drop each index after that, in its own two sets of transactions for
each entry, one to clear indisvalid and a second one to do the actual
drop.

The whole operation should make sure that we still have a fully
consistent state in the catalogs for any transaction, so as we have no
risk of finishing with a semi-broken partition tree if the thing fails
while processing for a reason or another.  As long as 1) is not
implemented, I don't think that this can really be safe, still this
requires careful thinking in the way we gather the list of indexes to
drop beforehand.

The error message is really confusing though, so for now I would
recommend to just drop an error if trying the operation on a
partitioned table, and we also do that now for CREATE INDEX
CONCURRENTLY.
--
Michael

On 2020-Aug-27, Michael Paquier wrote:

> The error message is really confusing though, so for now I would
> recommend to just drop an error if trying the operation on a
> partitioned table, and we also do that now for CREATE INDEX
> CONCURRENTLY.

Yeah, let's throw an error if the table is partitioned.  My bug -- I'll
go fix it now.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Aug-27, Alvaro Herrera wrote:

> On 2020-Aug-27, Michael Paquier wrote:
> 
> > The error message is really confusing though, so for now I would
> > recommend to just drop an error if trying the operation on a
> > partitioned table, and we also do that now for CREATE INDEX
> > CONCURRENTLY.
> 
> Yeah, let's throw an error if the table is partitioned.  My bug -- I'll
> go fix it now.

... as attached.

I first tried to add a hack directly in index_drop, but that doesn't
really work because there's no way to tell whether the partitioned index
is going to be dropped first or the index partition -- as that code runs
after the dependency tree has been walked.  The condition has to be
checked before starting the object-drop code proper.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Aug 27, 2020 at 03:22:18PM -0400, Alvaro Herrera wrote:
> I first tried to add a hack directly in index_drop, but that doesn't
> really work because there's no way to tell whether the partitioned index
> is going to be dropped first or the index partition -- as that code runs
> after the dependency tree has been walked.  The condition has to be
> checked before starting the object-drop code proper.

Yes, adding that to RemoveRelations() makes sense.  Thanks for the
patch.
--
Michael

On Fri, Aug 28, 2020 at 08:22:42AM +0900, Michael Paquier wrote:
> Yes, adding that to RemoveRelations() makes sense.  Thanks for the
> patch.

I got some room to test the patch, and the place of the check looks
good to me.  I think that I would move the new check before we set
PERFORM_DELETION_CONCURRENTLY for non-temporary relations though, as a
partition tree can be temporary as long as all its members are
temporary.
--
Michael

On 2020-Aug-29, Michael Paquier wrote:

> On Fri, Aug 28, 2020 at 08:22:42AM +0900, Michael Paquier wrote:
> > Yes, adding that to RemoveRelations() makes sense.  Thanks for the
> > patch.
> 
> I got some room to test the patch, and the place of the check looks
> good to me.  I think that I would move the new check before we set
> PERFORM_DELETION_CONCURRENTLY for non-temporary relations though, as a
> partition tree can be temporary as long as all its members are
> temporary.

Good point. Will push shortly with that change.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2020-Aug-29, Michael Paquier wrote:

> On Fri, Aug 28, 2020 at 08:22:42AM +0900, Michael Paquier wrote:
> > Yes, adding that to RemoveRelations() makes sense.  Thanks for the
> > patch.
> 
> I got some room to test the patch, and the place of the check looks
> good to me.  I think that I would move the new check before we set
> PERFORM_DELETION_CONCURRENTLY for non-temporary relations though, as a
> partition tree can be temporary as long as all its members are
> temporary.

Actually I think you're wrong; if I put it before the check, then if I
do "drop index concurrently some_temp_partitioned_index" then it would
fail; but if I put it after the check, then it does a normal
non-concurrent index and it works.  I'm not sure it's necessary to break
a case that otherwise works ...

(But for that to work I need to test the flag in the bitmask rather than
the option in the command, as in the attached).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Oh, and this:


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Aug 31, 2020 at 09:25:53PM -0400, Alvaro Herrera wrote:
> Actually I think you're wrong; if I put it before the check, then if I
> do "drop index concurrently some_temp_partitioned_index" then it would
> fail; but if I put it after the check, then it does a normal
> non-concurrent index and it works.  I'm not sure it's necessary to break
> a case that otherwise works ...

Hmm.  Right.  I agree that it would be better to not break that case.
And it means that there is a gap in the regression tests here, so I'd
like to add a test.  Please see the attached to achieve that, which
includes your own code changes and the doc parts (I didn't see a point
in changing the new sentence for temporary relations as the follow-up
<para> mentions that).

> (But for that to work I need to test the flag in the bitmask rather than
> the option in the command, as in the attached).

Make sense.
--
Michael

On 2020-Sep-01, Michael Paquier wrote:

> On Mon, Aug 31, 2020 at 09:25:53PM -0400, Alvaro Herrera wrote:
> > Actually I think you're wrong; if I put it before the check, then if I
> > do "drop index concurrently some_temp_partitioned_index" then it would
> > fail; but if I put it after the check, then it does a normal
> > non-concurrent index and it works.  I'm not sure it's necessary to break
> > a case that otherwise works ...
> 
> Hmm.  Right.  I agree that it would be better to not break that case.
> And it means that there is a gap in the regression tests here, so I'd
> like to add a test.  Please see the attached to achieve that, which
> includes your own code changes and the doc parts

Agreed -- thanks for that.

> (I didn't see a point in changing the new sentence for temporary
> relations as the follow-up <para> mentions that).

Yeah, I had come to the same conclusion.

Pushed now to all branches, thanks.

Thanks, Jan, for reporting this bug.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 01, 2020 at 01:44:08PM -0400, Alvaro Herrera wrote:
> Pushed now to all branches, thanks.

Thanks for taking care of it.
--
Michael