Re: REINDEX CONCURRENTLY unexpectedly fails
От | Heikki Linnakangas |
---|---|
Тема | Re: REINDEX CONCURRENTLY unexpectedly fails |
Дата | |
Msg-id | d62659d5-8860-075f-1e86-f17834f26495@iki.fi обсуждение исходный текст |
Ответ на | Re: REINDEX CONCURRENTLY unexpectedly fails (Michael Paquier <michael@paquier.xyz>) |
Ответы |
Re: REINDEX CONCURRENTLY unexpectedly fails
(Michael Paquier <michael@paquier.xyz>)
|
Список | pgsql-bugs |
On 09/01/2020 05:06, Michael Paquier wrote: > On Wed, Jan 08, 2020 at 05:19:30PM +0900, Michael Paquier wrote: >> 2) The handling of the patch within index_drop is too weak. As >> presented, the patch first locks the OID using a RangeVar. However >> for a temporary relation we would first take ShareUpdateExclusiveLock >> RemoveRelations() and then upgrade to a AccessExclusiveLock in >> index_drop(). I think that actually the check in index_drop() is not >> necessary, and that instead we had better do three things: >> a) In RangeVarCallbackForDropRelation(), if the relation is temporary, >> use AccessExclusiveLock all the time, and we know the OID of the >> relation here. >> b) After locking the OID with the RangeVar, re-check if the relation >> is temporary, and then remove PERFORM_DELETION_CONCURRENTLY is. >> c) Add an assertion in index_drop() to be sure that this code path is >> never invoked concurrently with a temporary relation. >> >> I am lacking of time today, I'll continue tomorrow. > > Okay, so here is an updated patch fixing those issues, with several > modifications done to the patch (docs, updates for the assertions, > some redesign). Considering again those aspects, I have come up with > the same conclusion as what's stated above, though you actually need > to make sure that it is RangeVarGetRelidExtended() that has to be > careful about the lock to use on the temporary relation, before > anything else is done. The callback RangeVarCallbackForDropRelation() > also needs to be careful about the relation it looks at and check if > the relation supports concurrent indexing. On the other hand, we > could also say that we don't care about lock upgrade risks when > working on temporary tables because these are not accessed by other > sessions, though that's not a sane base to rely on IMO. A solution > involving RangeVarGetRelidExtended() feels also like a sledgehammer to > smash a peanut, because it has a wider impact. I'm not a fan of all those changes in RangeVarCallbackForDropRelation() to ensure that you get an AccessExclusiveLock to begin with. It gets pretty complicated, and it feels like you need to special-case temporary tables in dozen different places. I liked the v3 of this patch better. It's true that you're upgrading the ShareUpdateExclusiveLock to AccessExclusiveLock, but since it's a temporary table, there really should be no other backend holding a lock on it. > If lock upgrade risks are not worth bothering, this needs to be > clearly documented in the patch with more comments. Yes, definitely. - Heikki
В списке pgsql-bugs по дате отправления: