Обсуждение: [PATCH] Simple code cleanup in tuplesort.c.
Hi hackers,
The bounded heap sorting status flag is set twice in sort_bounded_heap() and tuplesort_performsort(). This patch helps remove one of them.
Best Regards,
Xing
Вложения
On Wed, Jul 27, 2022 at 5:10 PM Xing Guo <higuoxing@gmail.com> wrote:
The bounded heap sorting status flag is set twice in sort_bounded_heap() and tuplesort_performsort(). This patch helps remove one of them.
+1. Looks good to me.
Thanks
Richard
Thanks
Richard
On Wed, Jul 27, 2022 at 5:10 PM Xing Guo <higuoxing@gmail.com> wrote:
The bounded heap sorting status flag is set twice in sort_bounded_heap() and tuplesort_performsort(). This patch helps remove one of them.
Revisiting this patch I think maybe it's better to remove the setting of
Tuplesort status from tuplesort_performsort() for the TSS_BOUNDED case.
Thus we keep the heap manipulation routines, make_bounded_heap and
sort_bounded_heap, consistent in that they update their status
accordingly inside the function.
Also, would you please add it to the CF to not lose track of it?
Thanks
Richard
Tuplesort status from tuplesort_performsort() for the TSS_BOUNDED case.
Thus we keep the heap manipulation routines, make_bounded_heap and
sort_bounded_heap, consistent in that they update their status
accordingly inside the function.
Also, would you please add it to the CF to not lose track of it?
Thanks
Richard
Hi Richard, Sorry for not responding for a long time, I missed the previous email by accident :-) I attached a newer patch based on your suggestions and created an entry in cf manager. https://commitfest.postgresql.org/40/3924/ Best Regards, Xing Guo On 9/16/22, Richard Guo <guofenglinux@gmail.com> wrote: > On Wed, Jul 27, 2022 at 5:10 PM Xing Guo <higuoxing@gmail.com> wrote: > >> The bounded heap sorting status flag is set twice in sort_bounded_heap() >> and tuplesort_performsort(). This patch helps remove one of them. >> > > Revisiting this patch I think maybe it's better to remove the setting of > Tuplesort status from tuplesort_performsort() for the TSS_BOUNDED case. > Thus we keep the heap manipulation routines, make_bounded_heap and > sort_bounded_heap, consistent in that they update their status > accordingly inside the function. > > Also, would you please add it to the CF to not lose track of it? > > Thanks > Richard > -- Best Regards, Xing
Вложения
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation: not tested Removing "state->status = TSS_SORTEDINMEM" should be fine as it is already set in sort_bounded_heap(state) few lines before. Cary Huang ---------------- HighGo Software Canada www.highgo.ca
On Fri, Sep 16, 2022 at 1:43 PM Richard Guo <guofenglinux@gmail.com> wrote:
>
>
> On Wed, Jul 27, 2022 at 5:10 PM Xing Guo <higuoxing@gmail.com> wrote:
>>
>> The bounded heap sorting status flag is set twice in sort_bounded_heap() and tuplesort_performsort(). This patch helps remove one of them.
>
>
> Revisiting this patch I think maybe it's better to remove the setting of
> Tuplesort status from tuplesort_performsort() for the TSS_BOUNDED case.
> Thus we keep the heap manipulation routines, make_bounded_heap and
> sort_bounded_heap, consistent in that they update their status
> accordingly inside the function.
The label TSS_BUILDRUNS has a similar style and also the following comment, so I will push this patch with a similar comment added unless someone wants to make a case for doing otherwise.
* Note that mergeruns sets the correct state->status.
--
John Naylor
EDB: http://www.enterprisedb.com
On Thu, Jan 5, 2023 at 8:18 AM John Naylor <john.naylor@enterprisedb.com> wrote:
>
> The label TSS_BUILDRUNS has a similar style and also the following comment, so I will push this patch with a similar comment added unless someone wants to make a case for doing otherwise.
>
> * Note that mergeruns sets the correct state->status.
This has been pushed, thanks. Note that both patches in this thread have the same name. Adding a version number to the name is a good way to distinguish them.
--
John Naylor
EDB: http://www.enterprisedb.com
On 1/9/23, John Naylor <john.naylor@enterprisedb.com> wrote: > On Thu, Jan 5, 2023 at 8:18 AM John Naylor <john.naylor@enterprisedb.com> > wrote: >> >> The label TSS_BUILDRUNS has a similar style and also the following > comment, so I will push this patch with a similar comment added unless > someone wants to make a case for doing otherwise. >> >> * Note that mergeruns sets the correct state->status. > > This has been pushed, thanks. Note that both patches in this thread have > the same name. Adding a version number to the name is a good way to > distinguish them. Thank you John. This is my first patch, I'll keep it in mind that adding a version number next time I sending the patch. > -- > John Naylor > EDB: http://www.enterprisedb.com > -- Best Regards, Xing
On Mon, Jan 9, 2023 at 7:29 PM Xing Guo <higuoxing@gmail.com> wrote:
>
> Thank you John. This is my first patch, I'll keep it in mind that
> adding a version number next time I sending the patch.
Welcome to the community! You may also consider reviewing a patch from the current commitfest, since we can always use additional help there.