Re: New pg_lsn type doesn't have hash/btree opclasses

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2014-05-09 22:14:25 +0900, Michael Paquier wrote:
>> [ patch ]

I've committed a revised version of Andres' patch. Mostly cosmetic
fixes, but the hash implementation was still wrong:

return DirectFunctionCall1(hashint8, PG_GETARG_LSN(0));

DirectFunctionCallN takes Datums, not de-Datumified values. This coding
would've crashed on 32-bit machines, where hashint8 would be expecting
to receive a Datum containing a pointer to int64.

We could have done it correctly like this:

return DirectFunctionCall1(hashint8, PG_GETARG_DATUM(0));

but I thought it was better, and microscopically more efficient, to
follow the existing precedent in timestamp_hash:

return hashint8(fcinfo);

since that avoids an extra FunctionCallInfoData setup.

>> On Fri, May 9, 2014 at 10:01 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> The patch looks good to me except the name of index operator class.
>>> I think that "pg_lsn_ops" is better than "pglsn_ops" because it's for "pg_lsn"
>>> data type.

I concur, and changed this.

> I honestly don't really see much point in the added tests. If at all I'd
> rather see my tests from
> http://archives.postgresql.org/message-id/20140506230722.GE24808%40awork2.anarazel.de
> addded. With some EXPLAIN (COSTS OFF) they'd test more.

I thought even that was kind of overkill; but a bigger problem is the
output was sensitive to hash values which are not portable across
different architectures. With a bit of experimentation I found that
a SELECT DISTINCT ... ORDER BY query would exercise both hashing and
sorting, so that's what got committed. (I'm not entirely sure though
whether the plan will be stable across architectures; we might have
to tweak the test case based on buildfarm feedback.)

regards, tom lane