Andres Freund <andres@2ndquadrant.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.fujii@gmail.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
