Re: Re: Abbreviated keys for Datum tuplesort

On Fri, Mar 13, 2015 at 7:51 PM, Andrew Gierth
<andrew(at)tao11(dot)riddles(dot)org(dot)uk> wrote:
> Since ApplySortComparator returns int, and "compare" is used to store
> the return value of comparetup_datum which is also declared int, this
> seems inappropriate even as a "stylistic tweak".

Consistency matters. That change, and the other changes to the
structure of comparetup_datum() makes it match the other comparetup_*
cases more closely. I don't think it's a big deal, and I'm surprised
you're calling it out specifically.

> Also, your changes to the block comment for SortTuple now hide the fact
> that datum1 is potentially the abbreviated value in tuple as well as
> single-Datum cases. Here are the versions for comparison (mine is
> first):

I don't think that I've failed to convey that, even just based on the
diff you included. My remarks apply to the new case, datum sorting,
where there is a new convention that must consider whether the type is
pass-by-value or pass-by-reference. Surely if it's not clear that
abbreviation is used for the heap tuple case (I think that's what you
mean), then that's a problem with the extant code in the master branch
that has nothing to do with this.

The whole point of this patch is that virtually every reasonable case
for abbreviation is now supported. That uniformity clarifies things.
So of course sortSupport (and abbreviation) is supported by every case
other than the hash case, where it clearly cannot apply. We just
needed to add a note on what the datum sort case does with
pass-by-value/past-by-reference-ness with respect to abbreviation,
alongside the existing discussion of that, since that becomes a
special case.

--
Peter Geoghegan