Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
От | Amit Kapila |
---|---|
Тема | Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager |
Дата | |
Msg-id | CAA4eK1LAr-_1JURScy1DQuqZUrWxP0V=89jieQYJJLzbdfXCXg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager (Kuntal Ghosh <kuntalghosh.2007@gmail.com>) |
Ответы |
Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
(Dilip Kumar <dilipbalaut@gmail.com>)
Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager (Kuntal Ghosh <kuntalghosh.2007@gmail.com>) |
Список | pgsql-hackers |
On Thu, Mar 12, 2020 at 7:50 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote: > > On Thu, Mar 12, 2020 at 5:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > > > > On Thu, Mar 12, 2020 at 11:15 AM Dilip Kumar <dilipbalaut@gmail.com> wrote: > > > > > > I have fixed this in the attached patch set. > > > > > > > I have modified your > > v4-0003-Conflict-Extension-Page-lock-in-group-member patch. The > > modifications are (a) Change src/backend/storage/lmgr/README to > > reflect new behaviour, (b) Introduce a new macro LOCK_LOCKTAG which > > slightly simplifies the code, (c) moved the deadlock.c check a few > > lines up and (d) changed a few comments. > > > > It might be better if we can move the checks related to extension and > > page lock in a separate API or macro. What do you think? > > > I think moving them inside a macro is a good idea. Also, I think we > should move all the Assert related code inside some debugging macro > similar to this: > #ifdef LOCK_DEBUG > .... > #endif > If we move it under some macro, then those Asserts will be only enabled when that macro is defined. I think we want there Asserts to be enabled always in assert enabled build, these will be like any other Asserts in the code. What is the advantage of doing those under macro? > + /* > + * The relation extension or page lock can never participate in actual > + * deadlock cycle. See Asserts in LockAcquireExtended. So, there is > + * no advantage in checking wait edges from it. > + */ > + if ((LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND) || > + (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE)) > + return false; > + > Since this is true, we can also avoid these kind of locks in > ExpandConstraints, right? > Yes, I had also thought about it but left it to avoid sprinkling such checks at more places than absolutely required. > It'll certainly reduce some complexity in > topological sort. > I think you mean to say TopoSort will have to look at fewer members in the wait queue, otherwise, there is nothing from the perspective of code which we can remove/change there. I think there will be hardly any chance that such locks will participate here because we take those for some work and release them (basically, they are unlike other heavyweight locks which can be released at the end). Having said that, I am not against putting those checks at the place you are suggesting, it is just that I thought that it won't be of much use. > /* > + * The relation extension or page lock conflict even between the group > + * members. > + */ > + if ((LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND) || > + (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE)) > + { > + PROCLOCK_PRINT("LockCheckConflicts: conflicting (group)", > + proclock); > + return true; > + } > This check includes the heavyweight locks that conflict even under > same parallel group. It also has another property that they can never > participate in deadlock cycles. And, the number of locks under this > category is likely to increase in future with new parallel features. > Hence, it could be used in multiple places. Should we move the > condition inside a macro and just call it from here? > Right, this is what I have suggested upthread. Do you have any suggestions for naming such a macro or function? I could think of something like LocksConflictAmongGroupMembers or LocksNotParticipateInDeadlock. The first one suits more for its usage in LockCheckConflicts and the second in the deadlock.c code. So none of those sound perfect to me. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Andy FanДата:
Сообщение: Re: [PATCH] Erase the distinctClause if the result is unique by definition
Следующее
От: Dilip KumarДата:
Сообщение: Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager