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

Hi,

On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
> Makes sense, especially knowing operators needed for btree processing
> are already defined. Patch attached solves that.

Thanks for doing that quickly.

FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.

> +/* handler for btree index operator */
> +Datum
> +pg_lsn_cmp(PG_FUNCTION_ARGS)
> +{
> +    XLogRecPtr lsn1 = PG_GETARG_LSN(0);
> +    XLogRecPtr lsn2 = PG_GETARG_LSN(1);
> +
> +    PG_RETURN_INT32(lsn1 == lsn2);
> +}

This doesn't look correct. A cmp routine needs to return -1, 0, 1 when a
< b, a = b, a > b respectively. You'll only return 0 and 1 here.

> +/* hash index support */
> +Datum
> +pg_lsn_hash(PG_FUNCTION_ARGS)
> +{
> +    XLogRecPtr lsn = PG_GETARG_LSN(0);
> +
> +    return hashint8(lsn);
> +}

That can't be right either. There's at least two things wrong here:
a) hashint8 takes PG_FUNCTION_ARGS, not a Datum
b) you're not including the necessary header defining hashint8.

I've used

SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1, 100) g(i), generate_series(1, 5);
SELECT (g.i||'/0')::pg_lsn f FROM generate_series(1, 100) g(i) ORDER BY f;
SELECT (g.i||'/0')::pg_lsn, count(*) FROM generate_series(1, 100) g(i), generate_series(1, 5) GROUP BY 1 ORDER BY 1;
SELECT * FROM
    (SELECT (g.i||'/0')::pg_lsn lsn FROM generate_series(1, 10) g(i) ORDER BY g.i) a
    JOIN (SELECT (g.i||'/0')::pg_lsn lsn FROM generate_series(1, 10) g(i) ORDER BY g.i DESC) b
        ON (a.lsn = b.lsn );

To check that a) hashing works, b) sorting works, c) mergesorts works
after fixing the above issues. New version of the patch attached

I completely agree with Heikki's point that this kind of oversight is
the actual reason for a beta period.

Greetings,

Andres Freund

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