Re: Re: [COMMITTERS] pgsql: Introduce group locking to prevent parallel processes from deadl

Robert Haas <robertmhaas@gmail.com> writes:
> Yeah, you're right.  Attached is a draft patch that tries to clean up
> that and a bunch of other things that you raised.

I reviewed this patch.  I don't have any particular comment on the changes
in deadlock.c; I haven't studied the code closely enough to know if those
are right.  I did think that the commentary elsewhere could be improved
some more, so attached is a delta patch on top of yours that shows what
I suggest.

I've not done anything here about removing lockGroupLeaderIdentifier,
although I will be happy to send a patch for that unless you know of
some reason I missed why it's necessary.

            regards, tom lane

diff --git a/src/backend/storage/lmgr/README b/src/backend/storage/lmgr/README
index 8eaa91c..5a62c8f 100644
*** a/src/backend/storage/lmgr/README
--- b/src/backend/storage/lmgr/README
*************** acquire an AccessShareLock while the oth
*** 603,609 ****
  This might seem dangerous and could be in some cases (more on that below), but
  if we didn't do this then parallel query would be extremely prone to
  self-deadlock.  For example, a parallel query against a relation on which the
! leader had already AccessExclusiveLock would hang, because the workers would
  try to lock the same relation and be blocked by the leader; yet the leader
  can't finish until it receives completion indications from all workers.  An
  undetected deadlock results.  This is far from the only scenario where such a
--- 603,609 ----
  This might seem dangerous and could be in some cases (more on that below), but
  if we didn't do this then parallel query would be extremely prone to
  self-deadlock.  For example, a parallel query against a relation on which the
! leader already had AccessExclusiveLock would hang, because the workers would
  try to lock the same relation and be blocked by the leader; yet the leader
  can't finish until it receives completion indications from all workers.  An
  undetected deadlock results.  This is far from the only scenario where such a
*************** quickly enough for this interlock to fai
*** 664,690 ****

  A PGPROC's lockGroupLeader is NULL for processes not involved in parallel
  query. When a process wants to cooperate with parallel workers, it becomes a
! lock group leader, which means setting this field back to point to its own
  PGPROC. When a parallel worker starts up, it points this field at the leader,
  with the above-mentioned interlock. The lockGroupMembers field is only used in
! the leader; it is a list of the workers. The lockGroupLink field is used to
! link the leader and all workers into the leader's list. All of these fields
! are protected by the lock manager locks; the lock manager lock that protects
! the lockGroupLeaderIdentifier, lockGroupLeader, and lockGroupMembers fields in
! a given PGPROC is chosen by taking pgprocno modulo the number of lock manager
! partitions. This unusual arrangement has a major advantage: the deadlock
! detector can count on the fact that no lockGroupLeader field can change while
! the deadlock detector is running, because it knows that it holds all the lock
! manager locks.  A PGPROC's lockGroupLink is protected by the lock manager
! partition lock for the group of which it is a part.  If it's not part of any
! group, this field is unused and can only be examined or modified by the
! process that owns the PGPROC.

  We institute a further coding rule that a process cannot join or leave a lock
  group while owning any PROCLOCK.  Therefore, given a lock manager lock
! sufficient to examine PROCLOCK *proclock, it also safe to examine
! proclock->tag.myProc->lockGroupLeader (but not the other fields mentioned in
! the previous paragraph).

  User Locks (Advisory Locks)
  ---------------------------
--- 664,689 ----

  A PGPROC's lockGroupLeader is NULL for processes not involved in parallel
  query. When a process wants to cooperate with parallel workers, it becomes a
! lock group leader, which means setting this field to point to its own
  PGPROC. When a parallel worker starts up, it points this field at the leader,
  with the above-mentioned interlock. The lockGroupMembers field is only used in
! the leader; it is a list of the member PGPROCs of the lock group (the leader
! and all workers). The lockGroupLink field is the list link for this list.
!
! All four of these fields are considered to be protected by a lock manager
! partition lock.  The partition lock that protects these fields within a given
! lock group is chosen by taking the leader's pgprocno modulo the number of lock
! manager partitions.  This unusual arrangement has a major advantage: the
! deadlock detector can count on the fact that no lockGroupLeader field can
! change while the deadlock detector is running, because it knows that it holds
! all the lock manager locks.  Also, holding this single lock allows safe
! manipulation of the lockGroupMembers list for the lock group.

  We institute a further coding rule that a process cannot join or leave a lock
  group while owning any PROCLOCK.  Therefore, given a lock manager lock
! sufficient to examine a PROCLOCK *proclock, it is also safe to examine
! proclock->tag.myProc->lockGroupLeader (but not the other lock-group-related
! fields of the PGPROC).

  User Locks (Advisory Locks)
  ---------------------------
diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c
index d81746d..21f92dd 100644
*** a/src/backend/storage/lmgr/deadlock.c
--- b/src/backend/storage/lmgr/deadlock.c
***************
*** 40,47 ****
   * is, it will be the leader rather than any other member of the lock group.
   * The group leaders act as representatives of the whole group even though
   * those particular processes need not be waiting at all.  There will be at
!  * least one member of the group on the wait queue for the given lock, maybe
!  * more.
   */
  typedef struct
  {
--- 40,47 ----
   * is, it will be the leader rather than any other member of the lock group.
   * The group leaders act as representatives of the whole group even though
   * those particular processes need not be waiting at all.  There will be at
!  * least one member of the waiter's lock group on the wait queue for the given
!  * lock, maybe more.
   */
  typedef struct
  {
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index cff8c08..deb88c7 100644
*** a/src/backend/storage/lmgr/proc.c
--- b/src/backend/storage/lmgr/proc.c
*************** ProcKill(int code, Datum arg)
*** 826,835 ****
              leader->lockGroupLeader = NULL;
              if (leader != MyProc)
              {
-                 procgloballist = leader->procgloballist;
-
                  /* Leader exited first; return its PGPROC. */
                  SpinLockAcquire(ProcStructLock);
                  leader->links.next = (SHM_QUEUE *) *procgloballist;
                  *procgloballist = leader;
                  SpinLockRelease(ProcStructLock);
--- 826,834 ----
              leader->lockGroupLeader = NULL;
              if (leader != MyProc)
              {
                  /* Leader exited first; return its PGPROC. */
                  SpinLockAcquire(ProcStructLock);
+                 procgloballist = leader->procgloballist;
                  leader->links.next = (SHM_QUEUE *) *procgloballist;
                  *procgloballist = leader;
                  SpinLockRelease(ProcStructLock);
*************** ProcSleep(LOCALLOCK *locallock, LockMeth
*** 1004,1010 ****
      int            i;

      /*
!      * If group locking is in use, locks held my members of my locking group
       * need to be included in myHeldLocks.
       */
      if (leader != NULL)
--- 1003,1009 ----
      int            i;

      /*
!      * If group locking is in use, locks held by members of my locking group
       * need to be included in myHeldLocks.
       */
      if (leader != NULL)
*************** BecomeLockGroupMember(PGPROC *leader, in
*** 1802,1810 ****
      /* PID must be valid. */
      Assert(pid != 0);

!     /* Try to join the group. */
      leader_lwlock = LockHashPartitionLockByProc(leader);
      LWLockAcquire(leader_lwlock, LW_EXCLUSIVE);
      if (leader->lockGroupLeaderIdentifier == pid)
      {
          ok = true;
--- 1801,1817 ----
      /* PID must be valid. */
      Assert(pid != 0);

!     /*
!      * Get lock protecting the group fields.  Note LockHashPartitionLockByProc
!      * accesses leader->pgprocno in a PGPROC that might be free.  This is safe
!      * because all PGPROCs' pgprocno fields are set during shared memory
!      * initialization and never change thereafter; so we will acquire the
!      * correct lock even if the leader PGPROC is in process of being recycled.
!      */
      leader_lwlock = LockHashPartitionLockByProc(leader);
      LWLockAcquire(leader_lwlock, LW_EXCLUSIVE);
+
+     /* Try to join the group */
      if (leader->lockGroupLeaderIdentifier == pid)
      {
          ok = true;
diff --git a/src/include/storage/lock.h b/src/include/storage/lock.h
index a3e19e1..2053bfe 100644
*** a/src/include/storage/lock.h
--- b/src/include/storage/lock.h
*************** typedef enum
*** 477,486 ****
   * by one of the lock hash partition locks.  Since the deadlock detector
   * acquires all such locks anyway, this makes it safe for it to access these
   * fields without doing anything extra.  To avoid contention as much as
!  * possible, we map different PGPROCs to different partition locks.
   */
! #define LockHashPartitionLockByProc(p) \
!     LockHashPartitionLock((p)->pgprocno)

  /*
   * function prototypes
--- 477,487 ----
   * by one of the lock hash partition locks.  Since the deadlock detector
   * acquires all such locks anyway, this makes it safe for it to access these
   * fields without doing anything extra.  To avoid contention as much as
!  * possible, we map different PGPROCs to different partition locks.  The lock
!  * used for a given lock group is determined by the group leader's pgprocno.
   */
! #define LockHashPartitionLockByProc(leader_pgproc) \
!     LockHashPartitionLock((leader_pgproc)->pgprocno)

  /*
   * function prototypes
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 81bddbb..a9405ce 100644
*** a/src/include/storage/proc.h
--- b/src/include/storage/proc.h
*************** struct PGPROC
*** 152,158 ****
       */
      TransactionId    procArrayGroupMemberXid;

!     /* Per-backend LWLock.  Protects fields below. */
      LWLock        backendLock;

      /* Lock manager data, recording fast-path locks taken by this backend. */
--- 152,158 ----
       */
      TransactionId    procArrayGroupMemberXid;

!     /* Per-backend LWLock.  Protects fields below (but not group fields). */
      LWLock        backendLock;

      /* Lock manager data, recording fast-path locks taken by this backend. */
*************** struct PGPROC
*** 163,173 ****
                                                   * lock */

      /*
!      * Support for lock groups.  Use LockHashPartitionLockByProc to get the
!      * LWLock protecting these fields.
       */
      int            lockGroupLeaderIdentifier;    /* MyProcPid, if I'm a leader */
!     PGPROC       *lockGroupLeader;    /* lock group leader, if I'm a follower */
      dlist_head    lockGroupMembers;    /* list of members, if I'm a leader */
      dlist_node  lockGroupLink;        /* my member link, if I'm a member */
  };
--- 163,173 ----
                                                   * lock */

      /*
!      * Support for lock groups.  Use LockHashPartitionLockByProc on the group
!      * leader to get the LWLock protecting these fields.
       */
      int            lockGroupLeaderIdentifier;    /* MyProcPid, if I'm a leader */
!     PGPROC       *lockGroupLeader;    /* lock group leader, if I'm a member */
      dlist_head    lockGroupMembers;    /* list of members, if I'm a leader */
      dlist_node  lockGroupLink;        /* my member link, if I'm a member */
  };