Hi, Pavel!
Thank you for your review and corrections.
On Fri, Jul 22, 2022 at 6:57 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> I've looked through the updated patch. Overall it looks good enough.
>
> Some minor things:
>
> - PARALLEL_SORT macro is based on coordinate struct instead of state struct. In some calls(i.e. from
_bt_spools_heapscan) coordinate could appear to be NULL, which can be a segfault on items dereference inside the macro.
>
> - state->worker and coordinate->isWorker a little bit differ in semantics i.e.:
> ..............................................worker............... leader
> state -> worker........................ >=0.....................-1
> coordinate ->isWorker............. 1..........................0
>
> - in tuplesort_begin_index_btree I suppose it should be base->nKeys instead of state->nKeys
Perfect, thank you!
> - Cfbot reports gcc warnings due to mixed code and declarations. So I used this to beautify code in
tuplesortvariants.ca little. (This is added as a separate patch 0007)
It appears that warnings were caused by the extra semicolon in
TuplesortstateGetPublic() macro. I've removed that semicolon, and I
don't think we need a beautification patch. Also, please note that
there is no point to add indentation, which doesn't survive pgindent.
> All these things are corrected/done in a new version 3 of a patchset (PFA). For me, the patchset seems like a
long-neededthing to support PostgreSQL extensibility. Overall corrections in v3 are minor, so I'd like to mark the
patchas RfC if there are no objections.
Thank you. I've also revised the comments in the top of tuplesort.c
and tuplesortvariants.c. The revised patchset is attached.
Also, my OrioleDB colleagues Ilya Kobets and Tatsiana Yaumenenka run
tests to check if the patchset causes a performance regression. The
script and results are present in the "tuplesort_patch_test.zip"
archive. The final comparison is given in the result/final_table.txt.
In short, they repeat each test 10 times and there is no difference
exceeding the random variation.
------
Regards,
Alexander Korotkov