Andres Freund <and...@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.fu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers