Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager - Mailing list pgsql-hackers
From | Dilip Kumar |
---|---|
Subject | Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager |
Date | |
Msg-id | CAFiTN-sRp4TcJqRfifBTXEX1_vjR3b70FLUat6Nu9CUS3kZzCA@mail.gmail.com Whole thread Raw |
In response to | Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager (Amit Kapila <amit.kapila16@gmail.com>) |
Responses |
Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
|
List | pgsql-hackers |
On Fri, Mar 13, 2020 at 8:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote: > > 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. I am not sure I understand this part. Because topological sort will work on the soft edges we have created when we found the cycle, but for relation extension/page lock we are completely ignoring hard/soft edge then it will never participate in topo sort as well. Am I missing something? -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
pgsql-hackers by date: