On Fri, Jan 29, 2021 at 08:56:47PM +0300, Alexey Kondratov wrote: > On 2021-01-28 14:42, Alexey Kondratov wrote: >> No, you are right, we are doing REINDEX with AccessExclusiveLock as it >> was before. This part is a more specific one. It only applies to >> partitioned indexes, which do not hold any data, so we do not reindex >> them directly, only their leafs. However, if we are doing a TABLESPACE >> change, we have to record it in their pg_class entry, so all future >> leaf partitions were created in the proper tablespace. >> >> That way, we open partitioned index relation only for a reference, >> i.e. read-only, but modify pg_class entry under a proper lock >> (RowExclusiveLock). That's why I thought that ShareLock will be >> enough. >> >> IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even >> for relations with no storage, since AlterTableGetLockLevel() chooses >> it if AT_SetTableSpace is met. This is very similar to our case, so >> probably we should do the same? >> >> Actually it is not completely clear for me why >> ShareUpdateExclusiveLock is sufficient for newly added >> SetRelationTableSpace() as Michael wrote in the comment.
Nay, it was not fine. That's something Alvaro has mentioned, leading to 2484329. This also means that the main patch of this thread should refresh the comments at the top of CheckRelationTableSpaceMove() and SetRelationTableSpace() to mention that this is used by REINDEX CONCURRENTLY with a lower lock. > Changed patch to use AccessExclusiveLock in this part for now. This is what > 'ALTER TABLE/INDEX ... SET TABLESPACE' and 'REINDEX' usually do. Anyway, all > real leaf partitions are processed in the independent transactions later. + if (partkind == RELKIND_PARTITIONED_INDEX) + { + Relation iRel = index_open(partoid, AccessExclusiveLock); + + if (CheckRelationTableSpaceMove(iRel, params->tablespaceOid)) + SetRelationTableSpace(iRel, + params->tablespaceOid, + InvalidOid); + index_close(iRel, NoLock); Are you sure that this does not represent a risk of deadlocks as EAL is not taken consistently across all the partitions? A second issue here is that this breaks the assumption of REINDEX CONCURRENTLY kicked on partitioned relations that should use ShareUpdateExclusiveLock for all its steps. This would make the first transaction invasive for the user, but we don't want that. This makes me really wonder if we would not be better to restrict this operation for partitioned relation as part of REINDEX as a first step. Another thing, mentioned upthread, is that we could do this part of the switch at the last transaction, or we could silently *not* do the switch for partitioned indexes in the flow of REINDEX, letting users handle that with an extra ALTER TABLE SET TABLESPACE once REINDEX has finished on all the partitions, cascading the command only on the partitioned relation of a tree. It may be interesting to look as well at if we could lower the lock used for partitioned relations with ALTER TABLE SET TABLESPACE from AEL to SUEL, choosing AEL only if at least one partition with storage is involved in the command, CheckRelationTableSpaceMove() discarding anything that has no need to change. -- Michael
signature.asc
Description: PGP signature