On Tue, Mar 22, 2016 at 12:33 PM, Dilip Kumar <
dilipbalaut@gmail.com> wrote:
>
>
> On Thu, Mar 17, 2016 at 11:39 AM, Amit Kapila <
amit.kapila16@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.