On 2020-01-16 13:56, Robert Haas wrote:
On Thu, Jan 16, 2020 at 12:50 AM Michael Paquier <mich...@paquier.xyz> wrote:
Thanks, that looks fine. I am still not sure whether the second patch
adding an enum via ProcWaitStatus improves the code readability
though, so my take would be to discard it for now. Perhaps others
think differently, I don't know.
IMHO, custom enums for each particular case would be a big improvement
over supposedly-generic STATUS codes. It makes it clearer which values
are possible in each code path, and it comes out nicer in the
debugger, too.
Given this feedback, I would like to re-propose the original patch,
attached again here.
After this, the use of the remaining STATUS_* symbols will be contained
to the frontend and backend libpq code, so it'll be more coherent.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 477515ea4b112e860587640f255cbfcdfee1755c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Sun, 29 Dec 2019 09:09:20 +0100
Subject: [PATCH v2 1/4] Remove STATUS_WAITING
Add a separate enum for use in the locking APIs, which were the only
user.
Discussion:
https://www.postgresql.org/message-id/flat/a6f91ead-0ce4-2a34-062b-7ab9813ea308%402ndquadrant.com
---
src/backend/access/transam/twophase.c | 2 +-
src/backend/storage/lmgr/lock.c | 8 ++---
src/backend/storage/lmgr/proc.c | 46 +++++++++++++--------------
src/include/c.h | 1 -
src/include/storage/proc.h | 13 ++++++--
5 files changed, 38 insertions(+), 32 deletions(-)
diff --git a/src/backend/access/transam/twophase.c
b/src/backend/access/transam/twophase.c
index e1904877fa..54fb6cc047 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -460,7 +460,7 @@ MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId
xid, const char *gid,
MemSet(proc, 0, sizeof(PGPROC));
proc->pgprocno = gxact->pgprocno;
SHMQueueElemInit(&(proc->links));
- proc->waitStatus = STATUS_OK;
+ proc->waitStatus = PROC_WAIT_STATUS_OK;
/* We set up the gxact's VXID as InvalidBackendId/XID */
proc->lxid = (LocalTransactionId) xid;
pgxact->xid = xid;
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 7fecb38162..95989ce79b 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -1856,7 +1856,7 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
*/
PG_TRY();
{
- if (ProcSleep(locallock, lockMethodTable) != STATUS_OK)
+ if (ProcSleep(locallock, lockMethodTable) !=
PROC_WAIT_STATUS_OK)
{
/*
* We failed as a result of a deadlock, see
CheckDeadLock(). Quit
@@ -1907,7 +1907,7 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
/*
* Remove a proc from the wait-queue it is on (caller must know it is on one).
* This is only used when the proc has failed to get the lock, so we set its
- * waitStatus to STATUS_ERROR.
+ * waitStatus to PROC_WAIT_STATUS_ERROR.
*
* Appropriate partition lock must be held by caller. Also, caller is
* responsible for signaling the proc if needed.
@@ -1923,7 +1923,7 @@ RemoveFromWaitQueue(PGPROC *proc, uint32 hashcode)
LOCKMETHODID lockmethodid = LOCK_LOCKMETHOD(*waitLock);
/* Make sure proc is waiting */
- Assert(proc->waitStatus == STATUS_WAITING);
+ Assert(proc->waitStatus == PROC_WAIT_STATUS_WAITING);
Assert(proc->links.next != NULL);
Assert(waitLock);
Assert(waitLock->waitProcs.size > 0);
@@ -1946,7 +1946,7 @@ RemoveFromWaitQueue(PGPROC *proc, uint32 hashcode)
/* Clean up the proc's own state, and pass it the ok/fail signal */
proc->waitLock = NULL;
proc->waitProcLock = NULL;
- proc->waitStatus = STATUS_ERROR;
+ proc->waitStatus = PROC_WAIT_STATUS_ERROR;
/*
* Delete the proclock immediately if it represents no already-held
locks.
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index f5eef6fa4e..e57fcd2538 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -383,7 +383,7 @@ InitProcess(void)
* initialized by InitProcGlobal.
*/
SHMQueueElemInit(&(MyProc->links));
- MyProc->waitStatus = STATUS_OK;
+ MyProc->waitStatus = PROC_WAIT_STATUS_OK;
MyProc->lxid = InvalidLocalTransactionId;
MyProc->fpVXIDLock = false;
MyProc->fpLocalTransactionId = InvalidLocalTransactionId;
@@ -567,7 +567,7 @@ InitAuxiliaryProcess(void)
* initialized by InitProcGlobal.
*/
SHMQueueElemInit(&(MyProc->links));
- MyProc->waitStatus = STATUS_OK;
+ MyProc->waitStatus = PROC_WAIT_STATUS_OK;
MyProc->lxid = InvalidLocalTransactionId;
MyProc->fpVXIDLock = false;
MyProc->fpLocalTransactionId = InvalidLocalTransactionId;
@@ -755,7 +755,7 @@ LockErrorCleanup(void)
* did grant us the lock, we'd better remember it in our local
lock
* table.
*/
- if (MyProc->waitStatus == STATUS_OK)
+ if (MyProc->waitStatus == PROC_WAIT_STATUS_OK)
GrantAwaitedLock();
}
@@ -1051,14 +1051,14 @@ ProcQueueInit(PROC_QUEUE *queue)
* The lock table's partition lock must be held at entry, and will be held
* at exit.
*
- * Result: STATUS_OK if we acquired the lock, STATUS_ERROR if not (deadlock).
+ * Result: PROC_WAIT_STATUS_OK if we acquired the lock, PROC_WAIT_STATUS_ERROR
if not (deadlock).
*
* ASSUME: that no one will fiddle with the queue until after
* we release the partition lock.
*
* NOTES: The process queue is now a priority queue for locking.
*/
-int
+ProcWaitStatus
ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
{
LOCKMODE lockmode = locallock->tag.mode;
@@ -1070,7 +1070,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
LOCKMASK myHeldLocks = MyProc->heldLocks;
bool early_deadlock = false;
bool allow_autovacuum_cancel = true;
- int myWaitStatus;
+ ProcWaitStatus myWaitStatus;
PGPROC *proc;
PGPROC *leader = MyProc->lockGroupLeader;
int i;
@@ -1161,7 +1161,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
/* Skip the wait and just grant myself
the lock. */
GrantLock(lock, proclock, lockmode);
GrantAwaitedLock();
- return STATUS_OK;
+ return PROC_WAIT_STATUS_OK;
}
/* Break out of loop to put myself before him */
break;
@@ -1195,7 +1195,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
MyProc->waitProcLock = proclock;
MyProc->waitLockMode = lockmode;
- MyProc->waitStatus = STATUS_WAITING;
+ MyProc->waitStatus = PROC_WAIT_STATUS_WAITING;
/*
* If we detected deadlock, give up without waiting. This must agree
with
@@ -1204,7 +1204,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
if (early_deadlock)
{
RemoveFromWaitQueue(MyProc, hashcode);
- return STATUS_ERROR;
+ return PROC_WAIT_STATUS_ERROR;
}
/* mark that we are waiting for a lock */
@@ -1236,7 +1236,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
/*
* Set timer so we can wake up after awhile and check for a deadlock.
If a
* deadlock is detected, the handler sets MyProc->waitStatus =
- * STATUS_ERROR, allowing us to know that we must report failure rather
+ * PROC_WAIT_STATUS_ERROR, allowing us to know that we must report
failure rather
* than success.
*
* By delaying the check until we've waited for a bit, we can avoid
@@ -1302,11 +1302,11 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
}
/*
- * waitStatus could change from STATUS_WAITING to something else
+ * waitStatus could change from PROC_WAIT_STATUS_WAITING to
something else
* asynchronously. Read it just once per loop to prevent
surprising
* behavior (such as missing log messages).
*/
- myWaitStatus = *((volatile int *) &MyProc->waitStatus);
+ myWaitStatus = *((volatile ProcWaitStatus *)
&MyProc->waitStatus);
/*
* If we are not deadlocked, but are waiting on an
autovacuum-induced
@@ -1487,24 +1487,24 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
lockHoldersNum, lock_holders_sbuf.data, lock_waiters_sbuf.data))));
}
- if (myWaitStatus == STATUS_WAITING)
+ if (myWaitStatus == PROC_WAIT_STATUS_WAITING)
ereport(LOG,
(errmsg("process %d still
waiting for %s on %s after %ld.%03d ms",
MyProcPid,
modename, buf.data, msecs, usecs),
(errdetail_log_plural("Process
holding the lock: %s. Wait queue: %s.",
"Processes holding the lock: %s. Wait queue: %s.",
lockHoldersNum, lock_holders_sbuf.data, lock_waiters_sbuf.data))));
- else if (myWaitStatus == STATUS_OK)
+ else if (myWaitStatus == PROC_WAIT_STATUS_OK)
ereport(LOG,
(errmsg("process %d acquired %s
on %s after %ld.%03d ms",
MyProcPid,
modename, buf.data, msecs, usecs)));
else
{
- Assert(myWaitStatus == STATUS_ERROR);
+ Assert(myWaitStatus == PROC_WAIT_STATUS_ERROR);
/*
* Currently, the deadlock checker always kicks
its own
- * process, which means that we'll only see
STATUS_ERROR when
+ * process, which means that we'll only see
PROC_WAIT_STATUS_ERROR when
* deadlock_state == DS_HARD_DEADLOCK, and
there's no need to
* print redundant messages. But for
completeness and
* future-proofing, print a message if it looks
like someone
@@ -1529,7 +1529,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
pfree(lock_holders_sbuf.data);
pfree(lock_waiters_sbuf.data);
}
- } while (myWaitStatus == STATUS_WAITING);
+ } while (myWaitStatus == PROC_WAIT_STATUS_WAITING);
/*
* Disable the timers, if they are still running. As in
LockErrorCleanup,
@@ -1568,7 +1568,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
/*
* If we got the lock, be sure to remember it in the locallock table.
*/
- if (MyProc->waitStatus == STATUS_OK)
+ if (MyProc->waitStatus == PROC_WAIT_STATUS_OK)
GrantAwaitedLock();
/*
@@ -1590,10 +1590,10 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable)
* XXX: presently, this code is only used for the "success" case, and only
* works correctly for that case. To clean up in failure case, would need
* to twiddle the lock's request counts too --- see RemoveFromWaitQueue.
- * Hence, in practice the waitStatus parameter must be STATUS_OK.
+ * Hence, in practice the waitStatus parameter must be PROC_WAIT_STATUS_OK.
*/
PGPROC *
-ProcWakeup(PGPROC *proc, int waitStatus)
+ProcWakeup(PGPROC *proc, ProcWaitStatus waitStatus)
{
PGPROC *retProc;
@@ -1601,7 +1601,7 @@ ProcWakeup(PGPROC *proc, int waitStatus)
if (proc->links.prev == NULL ||
proc->links.next == NULL)
return NULL;
- Assert(proc->waitStatus == STATUS_WAITING);
+ Assert(proc->waitStatus == PROC_WAIT_STATUS_WAITING);
/* Save next process before we zap the list link */
retProc = (PGPROC *) proc->links.next;
@@ -1657,7 +1657,7 @@ ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock)
{
/* OK to waken */
GrantLock(lock, proc->waitProcLock, lockmode);
- proc = ProcWakeup(proc, STATUS_OK);
+ proc = ProcWakeup(proc, PROC_WAIT_STATUS_OK);
/*
* ProcWakeup removes proc from the lock's waiting
process queue
@@ -1737,7 +1737,7 @@ CheckDeadLock(void)
* preserve the flexibility to kill some other transaction than
the
* one detecting the deadlock.)
*
- * RemoveFromWaitQueue sets MyProc->waitStatus to STATUS_ERROR,
so
+ * RemoveFromWaitQueue sets MyProc->waitStatus to
PROC_WAIT_STATUS_ERROR, so
* ProcSleep will report an error after we return from the
signal
* handler.
*/
diff --git a/src/include/c.h b/src/include/c.h
index d72b23afe4..a904b49a37 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1133,7 +1133,6 @@ typedef union PGAlignedXLogBlock
#define STATUS_OK (0)
#define STATUS_ERROR (-1)
#define STATUS_EOF (-2)
-#define STATUS_WAITING (2)
/*
* gettext support
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 1ee9000b2b..b20e2ad4f6 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -76,6 +76,13 @@ struct XidCache
*/
#define INVALID_PGPROCNO PG_INT32_MAX
+typedef enum
+{
+ PROC_WAIT_STATUS_OK,
+ PROC_WAIT_STATUS_WAITING,
+ PROC_WAIT_STATUS_ERROR,
+} ProcWaitStatus;
+
/*
* Each backend has a PGPROC struct in shared memory. There is also a list of
* currently-unused PGPROC structs that will be reallocated to new backends.
@@ -99,7 +106,7 @@ struct PGPROC
PGPROC **procgloballist; /* procglobal list that owns this PGPROC */
PGSemaphore sem; /* ONE semaphore to sleep on */
- int waitStatus; /* STATUS_WAITING,
STATUS_OK or STATUS_ERROR */
+ ProcWaitStatus waitStatus;
Latch procLatch; /* generic latch for process */
@@ -315,8 +322,8 @@ extern bool HaveNFreeProcs(int n);
extern void ProcReleaseLocks(bool isCommit);
extern void ProcQueueInit(PROC_QUEUE *queue);
-extern int ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable);
-extern PGPROC *ProcWakeup(PGPROC *proc, int waitStatus);
+extern ProcWaitStatus ProcSleep(LOCALLOCK *locallock, LockMethod
lockMethodTable);
+extern PGPROC *ProcWakeup(PGPROC *proc, ProcWaitStatus waitStatus);
extern void ProcLockWakeup(LockMethod lockMethodTable, LOCK *lock);
extern void CheckDeadLockAlert(void);
extern bool IsWaitingForLock(void);
--
2.27.0
