Re: Abbreviated keys for Numeric

From: Peter Geoghegan <pg(at)heroku(dot)com>
To: Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Gavin Flower <GavinFlower(at)archidevsys(dot)co(dot)nz>
Subject: Re: Abbreviated keys for Numeric
Date: 2015-03-25 13:31:57
Message-ID: CAM3SWZT99HR6QxbVEa2Amnk5QPj7-Ln91rM-fVWa037MujLvpg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Mar 24, 2015 at 12:03 AM, Andrew Gierth
<andrew(at)tao11(dot)riddles(dot)org(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thom Brown 2015-03-25 13:39:55 Re: Parallel Seq Scan
Previous Message David Steele 2015-03-25 13:24:44 Re: Auditing extension for PostgreSQL (Take 2)