Hi, Matthias!
The revised patchset is attached.
On Wed, Jul 6, 2022 at 5:41 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> 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.
Because spit of logic between Tuplesortstate.copytup() function and
tuplesort_put*() functions is unclear. It doesn't look like we need
an abstraction here, while all the work could be done in
tuplesort_put*().
> 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.
getdatum1() was a bad name. Actually it restores original datum1
during rollback of abbreviations. I've replaced it with
removeabbrev(), which seems name to me and process many SortTuple's
during one call.
> 0004: Needs a commit message, but otherwise seems fine.
Commit message is added.
> 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.
TuplesortOps was renamed to TuplesortPublic. Comments and commit
messages are added.
There are some places, which potentially could cause a slowdown. I'm
going to make some experiments with that.
------
Regards,
Alexander Korotkov
