Re: Abbreviated keys for Numeric

On Tue, Mar 24, 2015 at 12:03 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
> So here's the latest (and, hopefully, last) version:
>
>  - adds diagnostic output from numeric_abbrev_abort using the trace_sort
>    GUC
>
>  - fixed Datum cs. uint32 issues in hash_uint32
>
>  - added a short comment about excess-k representation
>
>  - tweaked the indenting and comments a bit

You still pointlessly check memtupcount here:

+ if (memtupcount < 10000 || nss->input_count < 10000 || !nss->estimating)
+     return false;

I still don't like this comment:

+ nss = palloc(sizeof(NumericSortSupport));
+
+ /*
+ * palloc a buffer for handling unaligned packed values in addition to
+ * the support struct
+ */
+ nss->buf = palloc(VARATT_SHORT_MAX + VARHDRSZ + 1);

This cast to void is unnecessary:

+numeric_fast_cmp(Datum x, Datum y, SortSupport ssup)
+{
+ Numeric nx = DatumGetNumeric(x);
+ Numeric ny = DatumGetNumeric(y);
+ int result;
+
+ (void) ssup;

Please try and at least consider my feedback. I don't expect you to do
exactly what I ask, but I also don't expect you to blithely ignore it.

> I'm not particularly committed to any specific way of handling the
> DEC_DIGITS issue. (I moved away from the "transparently skip
> abbreviations" approach of the original because it seemed that reducing
> #ifdefism in the code was a desirable feature.)

Good.

> The INT64_MIN/MAX changes should be committed fairly soon. (I haven't
> posted a patch for TRACE_SORT)

I wouldn't assume that.

-- 
Peter Geoghegan