Re: Custom tuplesorts for extensions

On Thu, 23 Jun 2022 at 14:12, Maxim Orlov <orlovmg@gmail.com> wrote:
>
> Hi!
>
> I've reviewed the patchset and noticed some minor issues:
> - extra semicolon in macro (lead to warnings)
> - comparison of var isWorker should be done in different way
>
> Here is an upgraded version of the patchset.
>
> Overall, I consider this patchset useful. Any opinions?

All of the patches are currently missing descriptive commit messages,
which I think is critical for getting this committed. As for per-patch
comments:

0001: This patch removes copytup, but it is not quite clear why -
please describe the reasoning in the commit message.

0002: getdatum1 needs comments on what it does. From the name, it
would return the datum1 from a sorttuple, but that's not what it does;
a better name would be putdatum1 or populatedatum1.

0003: in the various tuplesort_put*tuple[values] functions, the datum1
field is manually extracted. Shouldn't we use the getdatum1 functions
from 0002 instead? We could use either them directly to allow
inlining, or use the indirection through tuplesortstate.

0004: Needs a commit message, but otherwise seems fine.

0005:
> +struct TuplesortOps

This struct has no comment on what it is. Something like "Public
interface of tuplesort operators, containing data directly accessable
to users of tuplesort" should suffice, but feel free to update the
wording.

> +    void *arg;
> +};

This field could use a comment on what it is used for, and how to use it.

> +struct Tuplesortstate
> +{
> +    TuplesortOps ops;

This field needs a comment too.

0006: Needs a commit message, but otherwise seems fine.

Kind regards,

Matthias van de Meent