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.
Changes look fine to me.
> 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 feel it looks cleaner this way as well. But, If we plan to move it
to common function/macro then we should use some common name such that
it can be used in FindLockCycleRecurseMember as well as in
LockCheckConflicts.
> I have also used an extension to test this patch. This is the same
> extension that I have used to test the group locking patch. It will
> allow backends to form a group as we do for parallel workers. The
> extension is attached to this email.
>
> Test without patch:
> Session-1
> Create table t1(c1 int, c2 char(500));
> Select become_lock_group_leader();
>
> Insert into t1 values(generate_series(1,100),'aaa'); -- stop this
> after acquiring relation extension lock via GDB.
>
> Session-2
> Select become_lock_group_member();
> Insert into t1 values(generate_series(101,200),'aaa');
> - Debug LockAcquire and found that it doesn't generate conflict for
> Relation Extension lock.
>
> The above experiment has shown that without patch group members can
> acquire relation extension lock if the group leader has that lock.
> After patch the second session waits for the first session to release
> the relation extension lock. I know this is not a perfect way to test,
> but it is better than nothing. I think we need to do some more
> testing either using this extension or some other way for extension
> and page locks.
I have also tested the same and verified it.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com