Re: minor fix in CancelVirtualTransaction
Hi, On 2019-01-11 18:35:13 -0300, Alvaro Herrera wrote: > On 2019-Jan-04, Peter Eisentraut wrote: > > > Your reasoning seems correct to me. > > > > Maybe add a code comment along the lines of "once we have found the > > right ... we don't need to check the remaining ...". > > > > Or, you can make this even more clear by comparing the backendId > > directly with the proc entry: > > I did both (the second idea was a non-obvious very nice cleanup -- > thanks). Patch attached. > > However, now I realize that this code is not covered at all, so I'll put > this patch to sleep until I write some test for it. Given that the CF entry has been waiting on this update I'll mark this as returned with feedback, rather than moving to the next CF. Greetings, Andres Freund
Re: minor fix in CancelVirtualTransaction
On 2019-Jan-04, Peter Eisentraut wrote:
> Your reasoning seems correct to me.
>
> Maybe add a code comment along the lines of "once we have found the
> right ... we don't need to check the remaining ...".
>
> Or, you can make this even more clear by comparing the backendId
> directly with the proc entry:
I did both (the second idea was a non-obvious very nice cleanup --
thanks). Patch attached.
However, now I realize that this code is not covered at all, so I'll put
this patch to sleep until I write some test for it.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index faa46742bf..984ab7258a 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2679,13 +2679,10 @@ CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
{
int pgprocno = arrayP->pgprocnos[index];
PGPROC *proc = [pgprocno];
- VirtualTransactionId procvxid;
- GET_VXID_FROM_PGPROC(procvxid, *proc);
-
- if (procvxid.backendId == vxid.backendId)
+ if (vxid.backendId == proc->backendId)
{
- if (procvxid.localTransactionId == vxid.localTransactionId)
+ if (vxid.localTransactionId == proc->lxid)
{
proc->recoveryConflictPending = true;
pid = proc->pid;
@@ -2695,9 +2692,14 @@ CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
* Kill the pid if it's still here. If not, that's what we
* wanted so ignore any errors.
*/
- (void) SendProcSignal(pid, sigmode, vxid.backendId);
+ (void) SendProcSignal(pid, sigmode, proc->backendId);
}
}
+
+ /*
+ * As soon as we find the right proc entry, we don't need to check
+ * the remaining ones.
+ */
break;
}
}
Re: minor fix in CancelVirtualTransaction
On 06/12/2018 21:37, Alvaro Herrera wrote:
> When scanning the list of virtual transactions looking for one
> particular vxid, we cancel the transaction *and* break out of the loop
> only if both backendId and localTransactionId matches us. The canceling
> part is okay, but limiting the break to that case is pointless: if we
> found the correct backendId, there cannot be any other entry with the
> same backendId. Rework the loop so that we break out of it if backendId
> matches even if the localTransactionId part doesn't.
Your reasoning seems correct to me.
Maybe add a code comment along the lines of "once we have found the
right ... we don't need to check the remaining ...".
Or, you can make this even more clear by comparing the backendId
directly with the proc entry:
if (vxid.backendId == proc->backendId)
{
if (vxid.localTransactionId == proc->lxid)
{
}
break;
}
Then the logic your are trying to achieve would be obvious.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: minor fix in CancelVirtualTransaction
Same patch, commit message added. Adding to commitfest.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 2cbe6fe5ac7617e424a63b820a6a2c83b712bab5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Thu, 6 Dec 2018 17:30:58 -0300
Subject: [PATCH] Optimize CancelVirtualTransaction a little bit
When scanning the list of virtual transactions looking for one
particular vxid, we cancel the transaction *and* break out of the loop
only if both backendId and localTransactionId matches us. The canceling
part is okay, but limiting the break to that case is pointless: if we
found the correct backendId, there cannot be any other entry with the
same backendId. Rework the loop so that we break out of it if backendId
matches even if the localTransactionId part doesn't.
Discussion: https://postgr.es/m/20180629170512.d6kof6l6uu3qqpd6@alvherre.pgsql
---
src/backend/storage/ipc/procarray.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index dc7e875680..5ed588d9be 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2683,18 +2683,20 @@ CancelVirtualTransaction(VirtualTransactionId vxid, ProcSignalReason sigmode)
GET_VXID_FROM_PGPROC(procvxid, *proc);
- if (procvxid.backendId == vxid.backendId &&
- procvxid.localTransactionId == vxid.localTransactionId)
+ if (procvxid.backendId == vxid.backendId)
{
- proc->recoveryConflictPending = true;
- pid = proc->pid;
- if (pid != 0)
+ if (procvxid.localTransactionId == vxid.localTransactionId)
{
-/*
- * Kill the pid if it's still here. If not, that's what we
- * wanted so ignore any errors.
- */
-(void) SendProcSignal(pid, sigmode, vxid.backendId);
+proc->recoveryConflictPending = true;
+pid = proc->pid;
+if (pid != 0)
+{
+ /*
+ * Kill the pid if it's still here. If not, that's what we
+ * wanted so ignore any errors.
+ */
+ (void) SendProcSignal(pid, sigmode, vxid.backendId);
+}
}
break;
}
--
2.11.0
minor fix in CancelVirtualTransaction
It looks to me like we're missing a trick in CancelVirtualTransaction --
there's a loop to compare all VXIDs, and we break as soon as find a
perfect match in (backendid, localTransactionId). However, once we've
found the backendId that matches, it's not possible for there to be
another entry with the same backendId, so we might as well just quit the
loop, even if we don't send a signal.
No, I don't know if this shows in profiles. I just noticed while
reading code.
Attached patch (git diff --ignore-space-change; needs reindent)
illustrates. This was added by commit efc16ea52067 (Dec 2009) and seems
unchanged since then.
--
Álvaro Herrera Developer, https://www.PostgreSQL.org/
commit 9082e3f85bdc076c9187e6120d5fb94e53c4403a[m
Author: Alvaro Herrera
AuthorDate: Fri Jun 29 12:59:28 2018 -0400
CommitDate: Fri Jun 29 12:59:28 2018 -0400
fixup CancelVirtualTransaction
diff --git a/src/backend/storage/ipc/procarray.c
b/src/backend/storage/ipc/procarray.c
index 7f293d989b..2f776cf5c3 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2658,8 +2658,9 @@ CancelVirtualTransaction(VirtualTransactionId vxid,
ProcSignalReason sigmode)
GET_VXID_FROM_PGPROC(procvxid, *proc);
- if (procvxid.backendId == vxid.backendId &&
- procvxid.localTransactionId == vxid.localTransactionId)
+ if (procvxid.backendId == vxid.backendId)
+ {
+ if (procvxid.localTransactionId ==
vxid.localTransactionId)
{
proc->recoveryConflictPending = true;
pid = proc->pid;
@@ -2671,6 +2672,7 @@ CancelVirtualTransaction(VirtualTransactionId vxid,
ProcSignalReason sigmode)
*/
(void) SendProcSignal(pid, sigmode,
vxid.backendId);
}
+ }
break;
}
}
