Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
Hi,
In both TransactionGroupUpdateXidStatus and ProcArrayGroupClearXid
global MyProc is used. for consistency, replaced with a function local variable.
thanks
Rajesh
Attachments:
v1-0001-Use-proc-instead-of-MyProc-in-ProcArrayGroupClear.patchapplication/octet-stream; name=v1-0001-Use-proc-instead-of-MyProc-in-ProcArrayGroupClear.patchDownload
From 0b61998d33ccdb77b1e3b8851f2e1a23da571284 Mon Sep 17 00:00:00 2001
From: ssingarr <ssingarr@gmail.com>
Date: Mon, 7 Nov 2022 09:36:41 +0000
Subject: [PATCH v1] Use proc instead of MyProc in
ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
In both TransactionGroupUpdateXidStatus() and
ProcArrayGroupClearXid() global MyProc is used. For consistency,
replace it with a function local variable.
---
src/backend/access/transam/clog.c | 2 +-
src/backend/storage/ipc/procarray.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 77d9894dab..edc06d817c 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -553,7 +553,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
wakeproc->clogGroupMember = false;
- if (wakeproc != MyProc)
+ if (wakeproc != proc)
PGSemaphoreUnlock(wakeproc->sem);
}
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 207c4b27fd..f1fe625e4a 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -881,7 +881,7 @@ ProcArrayGroupClearXid(PGPROC *proc, TransactionId latestXid)
nextproc->procArrayGroupMember = false;
- if (nextproc != MyProc)
+ if (nextproc != proc)
PGSemaphoreUnlock(nextproc->sem);
}
}
--
2.34.1
On Mon, Nov 7, 2022 at 3:17 PM rajesh singarapu <rajesh.rs0541@gmail.com> wrote:
Hi,
In both TransactionGroupUpdateXidStatus and ProcArrayGroupClearXid
global MyProc is used. for consistency, replaced with a function local variable.
if (nextproc != MyProc)
PGSemaphoreUnlock(nextproc->sem);
The intention of this wake up code in the two functions is to skip the
leader process from waking itself up. Only the leader gets to execute
this code and all the followers don't hit this code at all as they
return from the first loop in those functions.
All the callers of ProcArrayGroupClearXid() get MyProc as their proc
and pass it down. And using the passed down function parameter proc
makes the function look consistent.
And, in TransactionGroupUpdateXidStatus() proc is initialized with
MyProc and using it instead of MyProc in the wake up loop also makes
the code consistent.
While it does no harm with the existing way using MyProc, +1 for
replacing it with the local variable proc in both the functions for
consistency.
Another thing I noticed is an extra assertion in
ProcArrayGroupClearXid() Assert(TransactionIdIsValid(proc->xid));, the
caller already has the same assertion, I think we can also remove it.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Nov 7, 2022 at 3:17 PM rajesh singarapu <rajesh.rs0541@gmail.com> wrote:
In both TransactionGroupUpdateXidStatus and ProcArrayGroupClearXid
global MyProc is used. for consistency, replaced with a function local variable.
In ProcArrayGroupClearXid(), currently, we always pass MyProc as proc,
so the change suggested by you will work but I think if in the future
someone calls it with a different proc, then the change suggested by
you won't work. The change in TransactionGroupUpdateXidStatus() looks
good but If we don't want to change ProcArrayGroupClearXid() then I am
not sure if there is much value in making the change in
TransactionGroupUpdateXidStatus().
--
With Regards,
Amit Kapila.
Thanks Bharat and Amit for the review and explaining rationale.
for the TransactionGroupUpdateXidStatus() change, let me see if I can
piggy back this change on something more valuable.
thanks
Rajesh
Show quoted text
On Tue, Nov 8, 2022 at 11:58 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Nov 7, 2022 at 3:17 PM rajesh singarapu <rajesh.rs0541@gmail.com> wrote:
In both TransactionGroupUpdateXidStatus and ProcArrayGroupClearXid
global MyProc is used. for consistency, replaced with a function local variable.In ProcArrayGroupClearXid(), currently, we always pass MyProc as proc,
so the change suggested by you will work but I think if in the future
someone calls it with a different proc, then the change suggested by
you won't work. The change in TransactionGroupUpdateXidStatus() looks
good but If we don't want to change ProcArrayGroupClearXid() then I am
not sure if there is much value in making the change in
TransactionGroupUpdateXidStatus().--
With Regards,
Amit Kapila.
On Tue, Nov 8, 2022 at 11:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Nov 7, 2022 at 3:17 PM rajesh singarapu <rajesh.rs0541@gmail.com> wrote:
In both TransactionGroupUpdateXidStatus and ProcArrayGroupClearXid
global MyProc is used. for consistency, replaced with a function local variable.In ProcArrayGroupClearXid(), currently, we always pass MyProc as proc,
so the change suggested by you will work but I think if in the future
someone calls it with a different proc, then the change suggested by
you won't work.
Well, yes. Do you have any thoughts around such future usages of
ProcArrayGroupClearXid()?
The change in TransactionGroupUpdateXidStatus() looks
good but If we don't want to change ProcArrayGroupClearXid() then I am
not sure if there is much value in making the change in
TransactionGroupUpdateXidStatus().
AFICS, there are many places in the code that use proc == MyProc (20
instances) or proc != MyProc (6 instances) sorts of things. I think
defining a macro, something like below, is better for readability.
However, I'm concerned that we might have to use it in 26 places.
#define IsPGPROCMine(proc) (proc != NULL && proc == MyProc)
or just
#define IsPGPROCMine(proc) (proc == MyProc)
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com