Re: SortSupport for UUID type

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


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_sortsupportrelies on this contract and does not initialize
ssup->extrabutuuid_sortsupport does. I think these are better to be in uniformstyle. I think removing ssup_extra = NULL
andadding a commentthat 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 comparisonbetween two Datums surely works.

5. uuid_abbrev_abort()
I don't know the validity of every thresholds, but they work asexpected.

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 thatPrepareSortSupportFrom(OrderingOp|IndexRel)() can find and it.


regards,



At Thu, 5 Nov 2015 16:10:21 -0800, Peter Geoghegan <pg@heroku.com> wrote in
<CAM3SWZR7xv_9p4V2xpatk2xK7Jxmcz8B6BjAbKtAwhxBEmAK=A@mail.gmail.com>
> On Thu, Oct 8, 2015 at 5:27 PM, Peter Geoghegan <pg@heroku.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