Обсуждение: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
От
PG Bug reporting form
Дата:
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
Re: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
От
Michael Paquier
Дата:
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
Вложения
Hi Alvaro,
thank you for looking into this on short notice. Looks better now with an error messaging hinting at the problem.
-- Jan
Am Do., 27. Aug. 2020 um 21:22 Uhr schrieb Alvaro Herrera <alvherre@2ndquadrant.com>:
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
Re: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
От
Michael Paquier
Дата:
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
Вложения
Re: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
От
Michael Paquier
Дата:
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
Вложения
Re: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
От
Michael Paquier
Дата:
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
Re: BUG #16594: DROP INDEX CONCURRENTLY fails on partitioned table with a non helpful error message.
От
Michael Paquier
Дата:
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