On Tue, Mar 22, 2016 at 12:33 PM, Dilip Kumar <dilipbal...@gmail.com> wrote: > > > On Thu, Mar 17, 2016 at 11:39 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > I have reviewed the patch.. here are some review comments, I will continue to review.. > > 1. > > + > + /* > + * Add the proc to list, if the clog page where we need to update the > > + */ > + if (nextidx != INVALID_PGPROCNO && > + ProcGlobal->allProcs[nextidx].clogGroupMemberPage != proc->clogGroupMemberPage) > + return false; > > Should we clear all these structure variable what we set above in case we are not adding our self to group, I can see it will not have any problem even if we don't clear them, > I think if we don't want to clear we can add some comment mentioning the same. >
I have updated the patch to just clear clogGroupMember as that is what is done when we wake the processes. > > 2. > > Here we are updating in our own proc, I think we don't need atomic operation here, we are not yet added to the list. > > + if (nextidx != INVALID_PGPROCNO && > + ProcGlobal->allProcs[nextidx].clogGroupMemberPage != proc->clogGroupMemberPage) > + return false; > + > + pg_atomic_write_u32(&proc->clogGroupNext, nextidx); > > We won't be able to assign nextidx to clogGroupNext is of type pg_atomic_uint32. Thanks for the review. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
