Re: SortSupport for UUID type

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: pg(at)heroku(dot)com
Cc: robertmhaas(at)gmail(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: SortSupport for UUID type
Date: 2015-11-06 08:35:34
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello, I tried to look on this as far as I can referring to

1. Patch application

This patch applies on the current master cleanly. And looks to
be work as expected.

2. uuid.c

pg_bswap.h is included under hash.h so it is not needed to be
included but I don't object if you include it explicitly.

3. uuid_sortsupport()

The comment for PrepareSortSupportFromIndexRel says that,

> * Caller must previously have zeroed the SortSupportData structure and then
> * filled in ssup_cxt, ssup_attno, ssup_collation, and ssup_nulls_first. This

And all callers comply with this order, and numeric_sortsupport
relies on this contract and does not initialize ssup->extra but
uuid_sortsupport does. I think these are better to be in uniform
style. I think removing ssup_extra = NULL and adding a comment
that ssup_extra has been initialized by the caller instead.

4. uuid_cmp_abbrev()

Although I don't know it is right or not, 3-way comparison
between two Datums surely works.

5. uuid_abbrev_abort()

I don't know the validity of every thresholds, but they work as

6. uuid_abbrev_convert()

> memcpy((char *) &res, authoritative->data, sizeof(Datum));

memcpy's prototype is "memcpy(void *dest..." so the cast to
(char *) is not necessary.

7. system catalogs

uuid_sortsupport looks to be properly registered so that
PrepareSortSupportFrom(OrderingOp|IndexRel)() can find and it.


At Thu, 5 Nov 2015 16:10:21 -0800, Peter Geoghegan <pg(at)heroku(dot)com> wrote in <CAM3SWZR7xv_9p4V2xpatk2xK7Jxmcz8B6BjAbKtAwhxBEmAK=A(at)mail(dot)gmail(dot)com>
> On Thu, Oct 8, 2015 at 5:27 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> > This is more or less lifted from numeric_abbrev_convert_var(). Perhaps
> > you should change it there too. The extra set of parenthesis are
> > removed in the attached patch. The patch also mechanically updates
> > things to be consistent with the text changes on the text thread [1]
> > -- I had to rebase.
> Attached is almost the same patch, but rebased. This was required
> because the name of our new macro was changed to
> DatumBigEndianToNative() at the last minute.

Kyotaro Horiguchi
NTT Open Source Software Center

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Artur Zakirov 2015-11-06 09:33:43 Re: [PROPOSAL] Improvements of Hunspell dictionaries support
Previous Message YUriy Zhuravlev 2015-11-06 08:12:10 Re: Some questions about the array.