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

On 2014-05-09 22:01:07 +0900, Fujii Masao wrote:
> On Wed, May 7, 2014 at 11:55 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)com> wrote:
> >
> > On Tue, May 6, 2014 at 8:25 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> > wrote:
> >>
> >> On Wed, May 7, 2014 at 8:22 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
> >> wrote:
> >> > Uh. They're different:
> >> >
> >> > Datum
> >> > timestamp_hash(PG_FUNCTION_ARGS)
> >> > {
> >> > /* We can use either hashint8 or hashfloat8 directly */
> >> > #ifdef HAVE_INT64_TIMESTAMP
> >> > return hashint8(fcinfo);
> >> > #else
> >> > return hashfloat8(fcinfo);
> >> > #endif
> >> > }
> >> > note it's passing fcinfo, not the datum as you do. Same with
> >> > time_hash.. In fact your version crashes when used because it's
> >> > dereferencing a int8 as a pointer inside hashfloat8.
> >> Thanks, didn't notice that fcinfo was used.
> >>
> >
> > Hi all,
> >
> > If helps, I added some regression tests to the lastest patch.
>
> +DATA(insert OID = 3260 ( 403 pglsn_ops PGNSP PGUID ));
> +DATA(insert OID = 3261 ( 405 pglsn_ops PGNSP PGUID ));
>
> The patch looks good to me except the name of index operator class.

FWIW, I've tested and looked through the patch as well.

> I think that "pg_lsn_ops" is better than "pglsn_ops" because it's for "pg_lsn"
> data type.

You're right, that's marginally prettier.

You plan to commit it?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services