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. + proc->clogGroupMember = true; + proc->clogGroupMemberXid = xid; + proc->clogGroupMemberXidStatus = status; + proc->clogGroupMemberPage = pageno; + proc->clogGroupMemberLsn = lsn; 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); -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
