Re: REINDEX CONCURRENTLY unexpectedly fails
От | Michael Paquier |
---|---|
Тема | Re: REINDEX CONCURRENTLY unexpectedly fails |
Дата | |
Msg-id | 20191213034536.GD1942@paquier.xyz обсуждение исходный текст |
Ответ на | Re: REINDEX CONCURRENTLY unexpectedly fails (Andres Freund <andres@anarazel.de>) |
Ответы |
Re: REINDEX CONCURRENTLY unexpectedly fails
(Michael Paquier <michael@paquier.xyz>)
|
Список | pgsql-bugs |
On Thu, Dec 12, 2019 at 01:37:09PM -0800, Andres Freund wrote: > On 2019-11-20 12:54:08 +0900, Michael Paquier wrote: >> ON COMMIT DELETE ROWS does a physical truncation of the relation >> files. And as DROP INDEX CONCURRENTLY cannot be run in a transaction >> block, you would never face a case where you have no past TIDs which >> could be referred to when setting the index as invalid. > > It's probably not reachable, but it strikes me as really fragile and > dangerous. If e.g. somehow ON COMMIT DROP tables could exist when DROP > CONCURRENTLY were run, the index_concurrently_set_dead() could very well > target a row that's since been deleted in an earlier transaction. Hmm. That joins with your point downthread about future changes.. >> Now I don't actually object to enforce the non-concurrent path in >> index_drop() for *all* temporary relations. Anyway, that makes sense >> in itself on performance grounds, similarly to the create path, so did >> that by enforcing the flag in index_drop() (doDeletion would be >> tempting but I took the problem at its root). And added some tests >> for the drop path and an extra assertion. > > Cool. > > I still think we'd be well served to add a few CheckTableNotInUse() type > checks... Sure. We have already one in drop_index, so DROP INDEX is covered, as well as reindex_index() which is taken by all non-concurrent REINDEX commands. Adding one in ReindexRelationConcurrently() may make sense.. >> Considered that, but ON COMMIT DROP does not make sense because it >> requires a transaction context, which is why I did not add one. And >> it seems to me that there is not much value to just check after CIC or >> REINDEX's restriction to not run in a transaction block? I added >> tests for these two, but I am of the opinion that they don't bring >> much. > > I think because CIC now falls back to non-concurrent mode, it's > worthwhile to exercise this path. It seems far from unlikely that the > code gets moved around enough that suddenly CIC is allowed in > transactions when targetting temp tables. That's a good point, we have no guarantee that nobody would play with this area in the future. Well, the patch has those tests anyway since the last version, so I have not touched them. >> I think that documenting it is good for the end-user as well. > > Why? Even if using a temporary table, the commands are not allowed within a transaction block, but we still track them in wait events so seeing an event related only to a non-concurrent path when using CONCURRENTLY can be confusing. >> + /* >> + * Enforce non-concurrent drop if the relation does not support this >> + * option. >> + */ >> + if (!RelationSupportsConcurrently(get_rel_persistence(indexId))) >> + concurrent = false; >> + > > Echoing Alvaro, I'm less than convinced by this name. I would really keep "Relation" in this part of the naming as this can be used for an index or its parent table, so in the updated attached I have gone with RelationSupportsConcurrentIndexing(), which is a suggestion from Alvaro. > Copying this to some, but not all, the places where > RelationSupportsConcurrently() is called doesn't seem helpful... Not sure I follow your point here. The following code paths are currently checked in the patch using this routine: - index_drop, both used by DROP INDEX and REINDEX CONCURRENTLY. This routine is called basically via performMultipleDeletions(). For REINDEX CONCURRENTLY, this cannot be actually reached, but not for DROP INDEX CONCURRENTLY. The logic to decide which drop behavior to choose is done in RemoveRelations(). And while we don't support dropping multiple objects with CONCURRENTLY, we have no way to say now for each object which lock level should be used for the drop, so it seems safer to me now to enforce non-concurrent to be used directly in index_drop rather than doing so at a higher level. - ReindexIndex, ReindexTable and ReindexMultipleTables, to check if the non-concurrent or concurrent paths need to be called for respectively REINDEX INDEX, TABLE and SCHEMA/DATABASE/SYSTEM. - RelationSupportsConcurrentIndexing, as the entry point for CREATE INDEX. + /* + * Enforce non-concurrent build if the relation does not support this + * option. + */ Or are you suggesting to remove this comment from the two places where it is used because it does not prove to help much? > If we want to add docs, I'd say at most something like "For temporary > tables index creation is always non-concurrent, as no other session can > access them, and non-concurrent index creation is cheaper.". Sounds like a better wording to me. Documenting it still seems rather important to me as I suspect that it could surprise some users. -- Michael
Вложения
В списке pgsql-bugs по дате отправления:
Следующее
От: Michael PaquierДата:
Сообщение: Re: BUG #16160: Minor memory leak in case of starting postgresserver with SSL encryption