[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
Attachments:
remove-redundant-logic.patchtext/x-patch; charset=US-ASCII; name=remove-redundant-logic.patchDownload
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index d90a1aebdf..84dc0d07f9 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2709,7 +2709,6 @@ sort_bounded_heap(Tuplesortstate *state)
*/
reversedirection(state);
- state->status = TSS_SORTEDINMEM;
state->boundUsed = true;
}
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
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
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
Attachments:
remove-redundant-logic.patchtext/x-patch; charset=US-ASCII; name=remove-redundant-logic.patchDownload
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 416f02ba3c..ef85e58aeb 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1445,7 +1445,6 @@ tuplesort_performsort(Tuplesortstate *state)
state->eof_reached = false;
state->markpos_offset = 0;
state->markpos_eof = false;
- state->status = TSS_SORTEDINMEM;
break;
case TSS_BUILDRUNS:
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.
--
John Naylor
EDB: http://www.enterprisedb.com