Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

On Sun, Mar 15, 2020 at 5:58 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sun, Mar 15, 2020 at 1:15 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Sat, Mar 14, 2020 at 7:39 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Mar 13, 2020 at 7:02 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > Apart from that, I have also extended the solution for the page lock.
> > > > And, I have also broken down the 3rd patch in two parts for relation
> > > > extension and for the page lock.
> > > >
> > >
> > > Thanks, I have made a number of cosmetic changes and written
> > > appropriate commit messages for all patches. See the attached patch
> > > series and let me know your opinion? BTW, did you get a chance to test
> > > page locks by using the extension which I have posted above or by some
> > > other way? I think it is important to test page-lock related patches
> > > now.
> >
> > I have reviewed the updated patches and looks fine to me. Apart from
> > this I have done testing for the Page Lock using group locking
> > extension.
> >
> > --Setup
> > create table gin_test_tbl(i int4[]) with (autovacuum_enabled = off);
> > create index gin_test_idx on gin_test_tbl using gin (i);
> > create table gin_test_tbl1(i int4[]) with (autovacuum_enabled = off);
> > create index gin_test_idx1 on gin_test_tbl1 using gin (i);
> >
> > --session1:
> > select become_lock_group_leader();
> > select gin_clean_pending_list('gin_test_idx');
> >
> > --session2:
> > select become_lock_group_member(session1_pid);
> > select gin_clean_pending_list('gin_test_idx1');
> >
> > --session3:
> > select become_lock_group_leader();
> > select gin_clean_pending_list('gin_test_idx1');
> >
> > --session4:
> > select become_lock_group_member(session3_pid);
> > select gin_clean_pending_list('gin_test_idx');
> >
> > ERROR: deadlock detected
> > DETAIL: Process 61953 waits for ExclusiveLock on page 0 of relation
> > 16399 of database 13577; blocked by process 62197.
> > Process 62197 waits for ExclusiveLock on page 0 of relation 16400 of
> > database 13577; blocked by process 61953.
> > HINT: See server log for query details.
> >
> >
> > Session1 and Session3 acquire the PageLock on two different index's
> > meta-pages and blocked in gdb, meanwhile, their member tries to
> > acquire the page lock as shown in the above example and it detects the
> > deadlock which is solved after applying the patch.
> >
>
> So, in this test, you have first performed the actions from Session-1
> and Session-3 (blocked them via GDB after acquiring page lock) and
> then performed the actions from Session-2 and Session-4, right?

Yes

> Though this is not a very realistic case, it proves the point that
> page locks don't participate in the deadlock cycle after the patch. I
> think we can do a few more tests that test other aspects of the patch.
>
> 1. Group members wait for page locks. If you test that the leader
> acquires the page lock and then member also tries to acquire the same
> lock on the same index, it wouldn't block before the patch, but after
> the patch, the member should wait for the leader to release the lock.

Okay, I will test this part.

> 2. Try to hit Assert in LockAcquireExtended (a) by trying to
> re-acquire the page lock via the debugger,

I am not sure whether it is true or not, Because, if we are holding
the page lock and we try the same page lock then the lock will be
granted without reaching the code path. However, I agree that this is
not intended instead this is a side effect of allowing relation
extension lock while holding the same relation extension lock. So
basically, now the situation is that if the lock is directly granted
because we are holding the same lock then it will not go to the assert
code. IMHO, we don't need to add extra code to make it behave
differently. Please let me know what is your opinion on this.

(b) try to acquire the
> relation extension lock after page lock and it should be allowed
> (after acquiring page lock, we take relation extension lock in
> following code path:
> ginInsertCleanup->ginEntryInsert->ginFindLeafPage->ginPlaceToPage->GinNewBuffer).

ok

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com