8000 Cosmetic improvements to group locking. · postgrespro/postgres@d91a4a6 · GitHub
[go: up one dir, main page]
Skip to content

Commit d91a4a6

Browse files
robertmhaasrobertmhaas
committed
Cosmetic improvements to group locking.
Reflow text in lock manager README so that it fits within 80 columns. Correct some mistakes. Expand the README to explain not only why group locking exists but also the data structures that support it. Improve comments related to group locking several files. Change the name of a macro argument for improved clarity. Most of these problems were reported by Tom Lane, but I found a few of them myself. Robert Haas and Tom Lane
1 parent dd077ef commit d91a4a6

File tree

5 files changed

+102
-53
lines changed

5 files changed

+102
-53
lines changed

‎src/backend/storage/lmgr/README

Lines changed: 67 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -591,34 +591,35 @@ Group Locking
591591

592592
As if all of that weren't already complicated enough, PostgreSQL now supports
593593
parallelism (see src/backend/access/transam/README.parallel), which means that
594-
we might need to resolve deadlocks that occur between gangs of related processes
595-
rather than individual processes. This doesn't change the basic deadlock
596-
detection algorithm very much, but it makes the bookkeeping more complicated.
594+
we might need to resolve deadlocks that occur between gangs of related
595+
processes rather than individual processes. This doesn't change the basic
596+
deadlock detection algorithm very much, but it makes the bookkeeping more
597+
complicated.
597598

598599
We choose to regard locks held by processes in the same parallel group as
599-
non-conflicting. This means that two processes in a parallel group can hold
600-
a self-exclusive lock on the same relation at the same time, or one process
601-
can acquire an AccessShareLock while the other already holds AccessExclusiveLock.
600+
non-conflicting. This means that two processes in a parallel group can hold a
601+
self-exclusive lock on the same relation at the same time, or one process can
602+
acquire an AccessShareLock while the other already holds AccessExclusiveLock.
602603
This might seem dangerous and could be in some cases (more on that below), but
603604
if we didn't do this then parallel query would be extremely prone to
604605
self-deadlock. For example, a parallel query against a relation on which the
605-
leader had already AccessExclusiveLock would hang, because the workers would
606-
try to lock the same relation and be blocked by the leader; yet the leader can't
607-
finish until it receives completion indications from all workers. An undetected
608-
deadlock results. This is far from the only scenario where such a problem
609-
happens. The same thing will occur if the leader holds only AccessShareLock,
610-
the worker seeks AccessShareLock, but between the time the leader attempts to
611-
acquire the lock and the time the worker attempts to acquire it, some other
612-
process queues up waiting for an AccessExclusiveLock. In this case, too, an
613-
indefinite hang results.
606+
leader already had AccessExclusiveLock would hang, because the workers would
607+
try to lock the same relation and be blocked by the leader; yet the leader
608+
can't finish until it receives completion indications from all workers. An
609+
undetected deadlock results. This is far from the only scenario where such a
610+
problem happens. The same thing will occur if the leader holds only
611+
AccessShareLock, the worker seeks AccessShareLock, but between the time the
612+
leader attempts to acquire the lock and the time the worker attempts to
613+
acquire it, some other process queues up waiting for an AccessExclusiveLock.
614+
In this case, too, an indefinite hang results.
614615

615616
It might seem that we could predict which locks the workers will attempt to
616617
acquire and ensure before going parallel that those locks would be acquired
617618
successfully. But this is very difficult to make work in a general way. For
618619
example, a parallel worker's portion of the query plan could involve an
619620
SQL-callable function which generates a query dynamically, and that query
620621
might happen to hit a table on which the leader happens to hold
621-
AccessExcusiveLock. By imposing enough restrictions on what workers can do,
622+
AccessExclusiveLock. By imposing enough restrictions on what workers can do,
622623
we could eventually create a situation where their behavior can be adequately
623624
restricted, but these restrictions would be fairly onerous, and even then, the
624625
system required to decide whether the workers will succeed at acquiring the
@@ -627,27 +628,56 @@ necessary locks would be complex and possibly buggy.
627628
So, instead, we take the approach of deciding that locks within a lock group
628629
do not conflict. This eliminates the possibility of an undetected deadlock,
629630
but also opens up some problem cases: if the leader and worker try to do some
630-
operation at the same time which would ordinarily be prevented by the heavyweight
631-
lock mechanism, undefined behavior might result. In practice, the dangers are
632-
modest. The leader and worker share the same transaction, snapshot, and combo
633-
CID hash, and neither can perform any DDL or, indeed, write any data at all.
634-
Thus, for either to read a table locked exclusively by the other is safe enough.
635-
Problems would occur if the leader initiated parallelism from a point in the
636-
code at which it had some backend-private state that made table access from
637-
another process unsafe, for example after calling SetReindexProcessing and
638-
before calling ResetReindexProcessing, catastrophe could ensue, because the
639-
worker won't have that state. Similarly, problems could occur with certain
640-
kinds of non-relation locks, such as relation extension locks. It's no safer
641-
for two related processes to extend the same relation at the time than for
642-
unrelated processes to do the same. However, since parallel mode is strictly
643-
read-only at present, neither this nor most of the similar cases can arise at
644-
present. To allow parallel writes, we'll either need to (1) further enhance
645-
the deadlock detector to handle those types of locks in a different way than
646-
other types; or (2) have parallel workers use some other mutual exclusion
647-
method for such cases; or (3) revise those cases so that they no longer use
648-
heavyweight locking in the first place (which is not a crazy idea, given that
649-
such lock acquisitions are not expected to deadlock and that heavyweight lock
650-
acquisition is fairly slow anyway).
631+
operation at the same time which would ordinarily be prevented by the
632+
heavyweight lock mechanism, undefined behavior might result. In practice, the
633+
dangers are modest. The leader and worker share the same transaction,
634+
snapshot, and combo CID hash, and neither can perform any DDL or, indeed,
635+
write any data at all. Thus, for either to read a table locked exclusively by
636+
the other is safe enough. Problems would occur if the leader initiated
637+
parallelism from a point in the code at which it had some backend-private
638+
state that made table access from another process unsafe, for example after
639+
calling SetReindexProcessing and before calling ResetReindexProcessing,
640+
catastrophe could ensue, because the worker won't have that state. Similarly,
641+
problems could occur with certain kinds of non-relation locks, such as
642+
relation extension locks. It's no safer for two related processes to extend
643+
the same relation at the time than for unrelated processes to do the same.
644+
However, since parallel mode is strictly read-only at present, neither this
645+
nor most of the similar cases can arise at present. To allow parallel writes,
646+
we'll either need to (1) further enhance the deadlock detector to handle those
647+
types of locks in a different way than other types; or (2) have parallel
648+
workers use some other mutual exclusion method for such cases; or (3) revise
649+
those cases so that they no longer use heavyweight locking in the first place
650+
(which is not a crazy idea, given that such lock acquisitions are not expected
651+
to deadlock and that heavyweight lock acquisition is fairly slow anyway).
652+
653+
Group locking adds four new members to each PGPROC: lockGroupLeaderIdentifier,
654+
lockGroupLeader, lockGroupMembers, and lockGroupLink. The first is simply a
655+
safety mechanism. A newly started parallel worker has to try to join the
656+
leader's lock group, but it has no guarantee that the group leader is still
657+
alive by the time it gets started. We try to ensure that the parallel leader
658+
dies after all workers in normal cases, but also that the system could survive
659+
relatively intact if that somehow fails to happen. This is one of the
660+
precautions against such a scenario: the leader relays its PGPROC and also its
661+
PID to the worker, and the worker fails to join the lock group unless the
662+
given PGPROC still has the same PID. We assume that PIDs are not recycled
663+
quickly enough for this interlock to fail.
664+
665+
A PGPROC's lockGroupLeader is NULL for processes not involved in parallel
666+
query. When a process wants to cooperate with parallel workers, it becomes a
667+
lock group leader, which means setting this field to point to its own
668+
PGPROC. When a parallel worker starts up, it points this field at the leader,
669+
with the above-mentioned interlock. The lockGroupMembers field is only used in
670+
the leader; it is a list of the member PGPROCs of the lock group (the leader
671+
and all workers). The lockGroupLink field is the list link for this list.
672+
673+
All four of these fields are considered to be protected by a lock manager
674+
partition lock. The partition lock that protects these fields within a given
675+
lock group is chosen by taking the leader's pgprocno modulo the number of lock
676+
manager partitions. This unusual arrangement has a major advantage: the
677+
deadlock detector can count on the fact that no lockGroupLeader field can
678+
change while the deadlock detector is running, because it knows that it holds
679+
all the lock manager locks. Also, holding this single lock allows safe
680+
manipulation of the lockGroupMembers list for the lock group.
651681

652682
User Locks (Advisory Locks)
653683
---------------------------

‎src/backend/storage/lmgr/deadlock.c

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,21 @@
3333
#include "utils/memutils.h"
3434

3535

36-
/* One edge in the waits-for graph */
36+
/*
37+
* One edge in the waits-for graph.
38+
*
39+
* waiter and blocker may or may not be members of a lock group, but if either
40+
* is, it will be the leader rather than any other member of the lock group.
41+
* The group leaders act as representatives of the whole group even though
42+
* those particular processes need not be waiting at all. There will be at
43+
* least one member of the waiter's lock group on the wait queue for the given
44+
* lock, maybe more.
45+
*/
3746
typedef struct
3847
{
39-
PGPROC *waiter; /* the waiting process */
40-
PGPROC *blocker; /* the process it is waiting for */
41-
LOCK *lock; /* the lock it is waiting for */
48+
PGPROC *waiter; /* the leader of the waiting lock group */
49+
PGPROC *blocker; /* the leader of the group it is waiting for */
50+
LOCK *lock; /* the lock being waited for */
4251
int pred; /* workspace for TopoSort */
4352
int link; /* workspace for TopoSort */
4453
} EDGE;
@@ -1006,8 +1015,8 @@ TopoSort(LOCK *lock,
10061015
return false;
10071016

10081017
/*
1009-
* Output everything in the lock group. There's no point in outputing
1010-
* an ordering where members of the same lock group are not
1018+
* Output everything in the lock group. There's no point in
1019+
* outputting an ordering where members of the same lock group are not
10111020
* consecutive on the wait queue: if some other waiter is between two
10121021
* requests that belong to the same group, then either it conflicts
10131022
* with both of them and is certainly not a solution; or it conflicts

‎src/backend/storage/lmgr/proc.c

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1004,7 +1004,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
10041004
int i;
10051005

10061006
/*
1007-
* If group locking is in use, locks held my members of my locking group
1007+
* If group locking is in use, locks held by members of my locking group
10081008
* need to be included in myHeldLocks.
10091009
*/
10101010
if (leader != NULL)
@@ -1050,7 +1050,8 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
10501050
{
10511051
/*
10521052
* If we're part of the same locking group as this waiter, its
1053-
* locks neither conflict with ours nor contribute to aheadRequsts.
1053+
* locks neither conflict with ours nor contribute to
1054+
* aheadRequests.
10541055
*/
10551056
if (leader != NULL && leader == proc->lockGroupLeader)
10561057
{
@@ -1797,9 +1798,17 @@ BecomeLockGroupMember(PGPROC *leader, int pid)
17971798
/* PID must be valid. */
17981799
Assert(pid != 0);
17991800

1800-
/* Try to join the group. */
1801+
/*
1802+
* Get lock protecting the group fields. Note LockHashPartitionLockByProc
1803+
* accesses leader->pgprocno in a PGPROC that might be free. This is safe
1804+
* because all PGPROCs' pgprocno fields are set during shared memory
1805+
* initialization and never change thereafter; so we will acquire the
1806+
* correct lock even if the leader PGPROC is in process of being recycled.
1807+
*/
18011808
leader_lwlock = LockHashPartitionLockByProc(MyProc);
18021809
LWLockAcquire(leader_lwlock, LW_EXCLUSIVE);
1810+
1811+
/* Try to join the group */
18031812
if (leader->lockGroupLeaderIdentifier == pid)
18041813
{
18051814
ok = true;

‎src/include/storage/lock.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -478,10 +478,11 @@ typedef enum
478478
* by one of the lock hash partition locks. Since the deadlock detector
479479
* acquires all such locks anyway, this makes it safe for it to access these
480480
* fields without doing anything extra. To avoid contention as much as
481-
* possible, we map different PGPROCs to different partition locks.
481+
* possible, we map different PGPROCs to different partition locks. The lock
482+
* used for a given lock group is determined by the group leader's pgprocno.
482483
*/
483-
#define LockHashPartitionLockByProc(p) \
484-
LockHashPartitionLock((p)->pgprocno)
484+
#define LockHashPartitionLockByProc(leader_pgproc) \
485+
LockHashPartitionLock((leader_pgproc)->pgprocno)
485486

486487
/*
487488
* function prototypes

‎src/include/storage/proc.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ struct PGPROC
152152
*/
153153
TransactionId procArrayGroupMemberXid;
154154

155-
/* Per-backend LWLock. Protects fields below. */
155+
/* Per-backend LWLock. Protects fields below (but not group fields). */
156156
LWLock backendLock;
157157

158158
/* Lock manager data, recording fast-path locks taken by this backend. */
@@ -163,11 +163,11 @@ struct PGPROC
163163
* lock */
164164

165165
/*
166-
* Support for lock groups. Use LockHashPartitionLockByProc to get the
167-
* LWLock protecting these fields.
166+
* Support for lock groups. Use LockHashPartitionLockByProc on the group
167+
* leader to get the LWLock protecting these fields.
168168
*/
169169
int lockGroupLeaderIdentifier; /* MyProcPid, if I'm a leader */
170-
PGPROC *lockGroupLeader; /* lock group leader, if I'm a follower */
170+
PGPROC *lockGroupLeader; /* lock group leader, if I'm a member */
171171
dlist_head lockGroupMembers; /* list of members, if I'm a leader */
172172
dlist_node lockGroupLink; /* my member link, if I'm a member */
173173
};

0 commit comments

Comments
 (0)
0