Re: Speed up Clog Access by increasing CLOG buffers
| От | Amit Kapila |
|---|---|
| Тема | Re: Speed up Clog Access by increasing CLOG buffers |
| Дата | |
| Msg-id | CAA4eK1JeGYfUoQi+tNmJrFyvr6G0Jmb=1gH2hNKqptcnNxdx_A@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: Speed up Clog Access by increasing CLOG buffers (Dilip Kumar <dilipbalaut@gmail.com>) |
| Список | pgsql-hackers |
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.
>
>
> 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);
>
>
>
>
> 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.
Thanks for the review.
В списке pgsql-hackers по дате отправления: