On 2020/12/16 23:28, Drouvot, Bertrand wrote:
Hi,
On 12/16/20 2:36 PM, Victor Yegorov wrote:
*CAUTION*: This email originated from outside of the organization. Do not click
links or open attachments unless you can confirm the sender and know the
content is safe.
ср, 16 дек. 2020 г. в 13:49, Fujii Masao <masao.fu...@oss.nttdata.com
<mailto:masao.fu...@oss.nttdata.com>>:
After doing this procedure, you can see the startup process and backend
wait for the table lock each other, i.e., deadlock. But this deadlock
remains
even after deadlock_timeout passes.
This seems a bug to me.
+1
> * Deadlocks involving the Startup process and an ordinary backend process
> * will be detected by the deadlock detector within the ordinary backend.
The cause of this issue seems that ResolveRecoveryConflictWithLock() that
the startup process calls when recovery conflict on lock happens doesn't
take care of deadlock case at all. You can see this fact by reading the
above
source code comment for ResolveRecoveryConflictWithLock().
To fix this issue, I think that we should enable STANDBY_DEADLOCK_TIMEOUT
timer in ResolveRecoveryConflictWithLock() so that the startup process can
send PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK signal to the backend.
Then if PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK signal arrives,
the backend should check whether the deadlock actually happens or not.
Attached is the POC patch implimenting this.
good catch!
I don't see any obvious reasons why the STANDBY_DEADLOCK_TIMEOUT shouldn't be
set in ResolveRecoveryConflictWithLock() too (it is already set in
ResolveRecoveryConflictWithBufferPin()).
So + 1 to consider this as a bug and for the way the patch proposes to fix it.
Thanks Victor and Bertrand for agreeing!
Attached is the updated version of the patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/src/backend/storage/ipc/procarray.c
b/src/backend/storage/ipc/procarray.c
index ee912b9d5e..7c1a3f8b3f 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3324,6 +3324,13 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid
dbOid)
*/
pid_t
CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
+{
+ return SignalVirtualTransaction(vxid, sigmode, true);
+}
+
+pid_t
+SignalVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode,
+ bool conflictPending)
{
ProcArrayStruct *arrayP = procArray;
int index;
@@ -3342,7 +3349,7 @@ CancelVirtualTransaction(VirtualTransactionId vxid,
ProcSignalReason sigmode)
if (procvxid.backendId == vxid.backendId &&
procvxid.localTransactionId == vxid.localTransactionId)
{
- proc->recoveryConflictPending = true;
+ proc->recoveryConflictPending = conflictPending;
pid = proc->pid;
if (pid != 0)
{
diff --git a/src/backend/storage/ipc/standby.c
b/src/backend/storage/ipc/standby.c
index 4ea3cf1f5c..4b7762644e 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -42,10 +42,15 @@ int max_standby_streaming_delay = 30 * 1000;
static HTAB *RecoveryLockLists;
+/* Is a deadlock check pending? */
+static volatile sig_atomic_t got_standby_deadlock_timeout;
+
static void ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId
*waitlist,
ProcSignalReason reason,
uint32 wait_event_info,
bool report_waiting);
+static void SendRecoveryConflictWithLock(ProcSignalReason reason,
+
LOCKTAG locktag);
static void SendRecoveryConflictWithBufferPin(ProcSignalReason reason);
static XLogRecPtr LogCurrentRunningXacts(RunningTransactions CurrRunningXacts);
static void LogAccessExclusiveLocks(int nlocks, xl_standby_lock *locks);
@@ -395,8 +400,10 @@ ResolveRecoveryConflictWithDatabase(Oid dbid)
* lock. As we are already queued to be granted the lock, no new lock
* requests conflicting with ours will be granted in the meantime.
*
- * Deadlocks involving the Startup process and an ordinary backend process
- * will be detected by the deadlock detector within the ordinary backend.
+ * We also must check for deadlocks involving the Startup process and
+ * hot-standby backend processes. If deadlock_timeout is reached in
+ * this function, the backends are requested to check themselves for
+ * deadlocks.
*/
void
ResolveRecoveryConflictWithLock(LOCKTAG locktag)
@@ -407,7 +414,7 @@ ResolveRecoveryConflictWithLock(LOCKTAG locktag)
ltime = GetStandbyLimitTime();
- if (GetCurrentTimestamp() >= ltime)
+ if (GetCurrentTimestamp() >= ltime && ltime != 0)
{
/*
* We're already behind, so clear a path as quickly as possible.
@@ -429,19 +436,47 @@ ResolveRecoveryConflictWithLock(LOCKTAG locktag)
else
{
/*
- * Wait (or wait again) until ltime
+ * Wait (or wait again) until ltime, and check for deadlocks as
well
+ * if we will be waiting longer than deadlock_timeout
*/
- EnableTimeoutParams timeouts[1];
+ EnableTimeoutParams timeouts[2];
+ int cnt = 0;
- timeouts[0].id = STANDBY_LOCK_TIMEOUT;
- timeouts[0].type = TMPARAM_AT;
- timeouts[0].fin_time = ltime;
- enable_timeouts(timeouts, 1);
+ if (ltime != 0)
+ {
+ timeouts[cnt].id = STANDBY_LOCK_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AT;
+ timeouts[cnt].fin_time = ltime;
+ cnt++;
+ }
+
+ timeouts[cnt].id = STANDBY_DEADLOCK_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AFTER;
+ timeouts[cnt].delay_ms = DeadlockTimeout;
+ cnt++;
+
+ enable_timeouts(timeouts, cnt);
}
/* Wait to be signaled by the release of the Relation Lock */
+ got_standby_deadlock_timeout = false;
ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type);
+ if (got_standby_deadlock_timeout)
+ {
+ /*
+ * Send out a request for hot-standby backends to check
themselves for
+ * deadlocks.
+ */
+ SendRecoveryConflictWithLock(
+
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
+
locktag);
+ got_standby_deadlock_timeout = false;
+
+ /* Wait again to be signaled by the release of the Relation
Lock */
+ ProcWaitForSignal(PG_WAIT_LOCK | locktag.locktag_type);
+ }
+
/*
* Clear any timeout requests established above. We assume here that
the
* Startup process doesn't have any other outstanding timeouts than
those
@@ -520,8 +555,23 @@ ResolveRecoveryConflictWithBufferPin(void)
}
/* Wait to be signaled by UnpinBuffer() */
+ got_standby_deadlock_timeout = false;
ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
+ if (got_standby_deadlock_timeout)
+ {
+ /*
+ * Send out a request for hot-standby backends to check
themselves for
+ * deadlocks.
+ */
+ SendRecoveryConflictWithBufferPin(
+
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
+ got_standby_deadlock_timeout = false;
+
+ /* Wait again to be signaled by UnpinBuffer() */
+ ProcWaitForSignal(PG_WAIT_BUFFER_PIN);
+ }
+
/*
* Clear any timeout requests established above. We assume here that
the
* Startup process doesn't have any other timeouts than what this
function
@@ -531,6 +581,30 @@ ResolveRecoveryConflictWithBufferPin(void)
disable_all_timeouts(false);
}
+static void
+SendRecoveryConflictWithLock(ProcSignalReason reason, LOCKTAG locktag)
+{
+ VirtualTransactionId *backends;
+
+ Assert(reason == PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
+
+ backends = GetLockConflicts(&locktag, AccessExclusiveLock, NULL);
+
+ /* Quick exit if there's no work to be done */
+ if (!VirtualTransactionIdIsValid(*backends))
+ return;
+
+ /*
+ * We send signal to all the backends holding the conflicting locks, to
+ * ask them to check for deadlocks.
+ */
+ while (VirtualTransactionIdIsValid(*backends))
+ {
+ SignalVirtualTransaction(*backends, reason, false);
+ backends++;
+ }
+}
+
static void
SendRecoveryConflictWithBufferPin(ProcSignalReason reason)
{
@@ -588,13 +662,17 @@ CheckRecoveryConflictDeadlock(void)
/*
* StandbyDeadLockHandler() will be called if STANDBY_DEADLOCK_TIMEOUT
- * occurs before STANDBY_TIMEOUT. Send out a request for hot-standby
- * backends to check themselves for deadlocks.
+ * occurs before STANDBY_TIMEOUT.
*/
void
StandbyDeadLockHandler(void)
{
-
SendRecoveryConflictWithBufferPin(PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK);
+ int save_errno = errno;
+
+ got_standby_deadlock_timeout = true;
+
+ SetLatch(MyLatch);
+ errno = save_errno;
}
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3679799e50..385a749e46 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2919,11 +2919,21 @@ RecoveryConflictInterrupt(ProcSignalReason reason)
case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
/*
- * If we aren't blocking the Startup process
there is nothing
- * more to do.
+ * If PROCSIG_RECOVERY_CONFLICT_BUFFERPIN is
requested but we
+ * aren't blocking the Startup process there is
nothing more
+ * to do.
+ *
+ * If
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK is requested
+ * and we're waiting for locks but don't hold
buffer pins
+ * blocking the Startup process, we set the
flag so that
+ * ProcSleep() will check for deadlocks.
*/
if (!HoldingBufferPinThatDelaysRecovery())
+ {
+ if (reason ==
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK)
+ CheckDeadLockAlert();
return;
+ }
MyProc->recoveryConflictPending = true;
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index ea8a876ca4..4d272c7287 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -72,6 +72,8 @@ extern VirtualTransactionId
*GetCurrentVirtualXIDs(TransactionId limitXmin,
int *nvxids);
extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId
limitXmin, Oid dbOid);
extern pid_t CancelVirtualTransaction(VirtualTransactionId vxid,
ProcSignalReason sigmode);
+extern pid_t SignalVirtualTransaction(VirtualTransactionId vxid,
ProcSignalReason sigmode,
+ bool
conflictPending);
extern bool MinimumActiveBackends(int min);
extern int CountDBBackends(Oid databaseid);
