Thread: Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
From
rajesh singarapu
Date:
Hi, In both TransactionGroupUpdateXidStatus and ProcArrayGroupClearXid global MyProc is used. for consistency, replaced with a function local variable. thanks Rajesh
Attachment
Re: Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
From
Bharath Rupireddy
Date:
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
Re: Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
From
Amit Kapila
Date:
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.
Re: Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
From
rajesh singarapu
Date:
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 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.
Re: Use proc instead of MyProc in ProcArrayGroupClearXid()/TransactionGroupUpdateXidStatus()
From
Bharath Rupireddy
Date:
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