On 2020-Nov-19, Michael Paquier wrote: > On Thu, Nov 19, 2020 at 12:13:44PM +0900, Michael Paquier wrote: > > That still looks useful for debugging, so DEBUG1 sounds fine to me. > > By the way, it strikes me that you could just do nothing as long as > (log_min_messages > DEBUG1), so you could encapsulate most of the > logic that plays with the lock tag using that.
Good idea, done. I also noticed that if we're going to accept a race (which BTW already exists) we may as well simplify the code about it. I think the attached is the final form of this.
>From cab95d8813cd79adb7b2a1b5501714c2dd87bec2 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 17 Nov 2020 21:09:22 -0300 Subject: [PATCH] Don't hold ProcArrayLock longer than needed in rare cases While cancelling an autovacuum worker, we hold ProcArrayLock while formatting a debugging log string. We can make this shorter by saving the data we need to produce the message and doing the formatting outside the locked region. This isn't terribly critical, as it only occurs pretty rarely: when a backend runs deadlock detection and it happens to be blocked by a autovacuum running autovacuum. Still, there's no need to cause a hiccup in ProcArrayLock processing, which can be very high-traffic in some cases. While at it, rework code so that we only print the string when it is really going to be used, as suggested by Michael Paquier. Discussion: https://postgr.es/m/20201118214127.GA3179@alvherre.pgsql Reviewed-by: Michael Paquier <mich...@paquier.xyz> --- src/backend/storage/lmgr/proc.c | 63 ++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index d1738c65f5..dbfdd7b3c7 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -1311,40 +1311,58 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) { PGPROC *autovac = GetBlockingAutoVacuumPgproc(); uint8 statusFlags; + uint8 lockmethod_copy; + LOCKTAG locktag_copy; - LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE); + /* + * Grab info we need, then release lock immediately. Note this + * coding means that there is a tiny chance that the process + * terminates its current transaction and starts a different one + * before we have a change to send the signal; the worst possible + * consequence is that a for-wraparound vacuum is cancelled. But + * that could happen in any case unless we were to do kill() with + * the lock held, which is much more undesirable. + */ + LWLockAcquire(ProcArrayLock, LW_SHARED); + statusFlags = ProcGlobal->statusFlags[autovac->pgxactoff]; + lockmethod_copy = lock->tag.locktag_lockmethodid; + locktag_copy = lock->tag; + LWLockRelease(ProcArrayLock); /* * Only do it if the worker is not working to protect against Xid * wraparound. */ - statusFlags = ProcGlobal->statusFlags[autovac->pgxactoff]; if ((statusFlags & PROC_IS_AUTOVACUUM) && !(statusFlags & PROC_VACUUM_FOR_WRAPAROUND)) { int pid = autovac->pid; - StringInfoData locktagbuf; - StringInfoData logbuf; /* errdetail for server log */ - initStringInfo(&locktagbuf); - initStringInfo(&logbuf); - DescribeLockTag(&locktagbuf, &lock->tag); - appendStringInfo(&logbuf, - _("Process %d waits for %s on %s."), - MyProcPid, - GetLockmodeName(lock->tag.locktag_lockmethodid, - lockmode), - locktagbuf.data); + /* report the case, if configured to do so */ + if (log_min_messages <= DEBUG1 || client_min_messages <= DEBUG1) + { + StringInfoData locktagbuf; + StringInfoData logbuf; /* errdetail for server log */ - /* release lock as quickly as possible */ - LWLockRelease(ProcArrayLock); + initStringInfo(&locktagbuf); + initStringInfo(&logbuf); + DescribeLockTag(&locktagbuf, &locktag_copy); + appendStringInfo(&logbuf, + _("Process %d waits for %s on %s."), + MyProcPid, + GetLockmodeName(lockmethod_copy, lockmode), + locktagbuf.data); + + ereport(DEBUG1, + (errmsg("sending cancel to blocking autovacuum PID %d", + pid), + errdetail_log("%s", logbuf.data))); + + pfree(logbuf.data); + pfree(locktagbuf.data); + } /* send the autovacuum worker Back to Old Kent Road */ - ereport(DEBUG1, - (errmsg("sending cancel to blocking autovacuum PID %d", - pid), - errdetail_log("%s", logbuf.data))); - if (kill(pid, SIGINT) < 0) { /* @@ -1362,12 +1380,7 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable) (errmsg("could not send signal to process %d: %m", pid))); } - - pfree(logbuf.data); - pfree(locktagbuf.data); } - else - LWLockRelease(ProcArrayLock); /* prevent signal from being sent again more than once */ allow_autovacuum_cancel = false; -- 2.20.1
