New pg_lsn type doesn't have hash/btree opclasses

Andres Freund <andres@2ndquadrant.com> writes:

Craig just mentioned in an internal chat that there's no btree or even
hash opclass for the new pg_lsn type. That restricts what you can do
with it quite severely.
Imo this should be fixed for 9.4 - after all it was possible unto now to
index a table with lsns returned by system functions or have queries
with grouping on them without casting.

Sorry, it is *way* too late for 9.4.

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

On 2014-05-06 09:37:54 -0400, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

Craig just mentioned in an internal chat that there's no btree or even
hash opclass for the new pg_lsn type. That restricts what you can do
with it quite severely.
Imo this should be fixed for 9.4 - after all it was possible unto now to
index a table with lsns returned by system functions or have queries
with grouping on them without casting.

Sorry, it is *way* too late for 9.4.

It's imo a regression/oversight introduced in the pg_lsn patch. Not a
new feature.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, May 6, 2014 at 4:14 PM, Andres Freund <andres@2ndquadrant.com> wrote:

Hi,

Craig just mentioned in an internal chat that there's no btree or even
hash opclass for the new pg_lsn type. That restricts what you can do
with it quite severely.
Imo this should be fixed for 9.4 - after all it was possible unto now to
index a table with lsns returned by system functions or have queries
with grouping on them without casting.

Makes sense, especially knowing operators needed for btree processing
are already defined. Patch attached solves that. Now to include it in
9.4 where development is over is another story... I wouldn't mind
adding it to the next commit fest either.
Regards,
--
Michael

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-05-06 09:37:54 -0400, Tom Lane wrote:

Sorry, it is *way* too late for 9.4.

It's imo a regression/oversight introduced in the pg_lsn patch. Not a
new feature.

You can argue that if you like, but it doesn't matter. It's too late for
a change as big as that for such an inessential feature. We are in the
stabilization game at this point, and adding features is not the thing to
be doing.

Especially not features for which no patch even exists. I don't care if
it could be done in a few hours by copy-and-paste, it would still be the
very definition of rushed coding. We're past the window for this kind of
change in 9.4.

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

On 05/06/2014 05:17 PM, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-05-06 09:37:54 -0400, Tom Lane wrote:

Sorry, it is *way* too late for 9.4.

It's imo a regression/oversight introduced in the pg_lsn patch. Not a
new feature.

You can argue that if you like, but it doesn't matter. It's too late for
a change as big as that for such an inessential feature. We are in the
stabilization game at this point, and adding features is not the thing to
be doing.

FWIW, I agree with Andres that this would be a reasonable thing to add.
Exactly the kind of oversight that we should be fixing at this stage in
the release cycle.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 05/06/2014 04:59 PM, Heikki Linnakangas wrote:

On 05/06/2014 05:17 PM, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-05-06 09:37:54 -0400, Tom Lane wrote:

Sorry, it is *way* too late for 9.4.

It's imo a regression/oversight introduced in the pg_lsn patch. Not a
new feature.

You can argue that if you like, but it doesn't matter. It's too late
for
a change as big as that for such an inessential feature. We are in the
stabilization game at this point, and adding features is not the
thing to
be doing.

FWIW, I agree with Andres that this would be a reasonable thing to
add. Exactly the kind of oversight that we should be fixing at this
stage in the release cycle.

I agree as well.

--
Vik

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, May 6, 2014 at 10:49 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

Makes sense, especially knowing operators needed for btree processing
are already defined. Patch attached solves that.

Please find attached an updated patch, I completely forgot that the
compare function needs to return {-1, 0, 1}.
--
Michael

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

On Wed, May 7, 2014 at 8:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.

Could you be clearer? By applying a filterdiff command or by using git
diff? The latter has been used for this 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.

Thanks, I recalled that this morning as well... And my 2nd patch uses
this flow instead:
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+       XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+       XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+       if (lsn1 < lsn2)
+               PG_RETURN_INT32(-1);
+       if (lsn1 > lsn2)
+               PG_RETURN_INT32(1);
+       PG_RETURN_INT32(0);
+}

+/* 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

In this case you may consider changing timestamp_hash@time.c and
time_hash@date.c as well :)
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-05-07 08:16:38 +0900, Michael Paquier wrote:

On Wed, May 7, 2014 at 8:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.

Could you be clearer? By applying a filterdiff command or by using git
diff? The latter has been used for this patch.

As I said, consider using git format-patch. Then the patch can be
applied using git am. Resulting in a local commit including your
commit message.

+/* 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.

Thanks, I recalled that this morning as well... And my 2nd patch uses
this flow instead:
+Datum
+pg_lsn_cmp(PG_FUNCTION_ARGS)
+{
+       XLogRecPtr lsn1 = PG_GETARG_LSN(0);
+       XLogRecPtr lsn2 = PG_GETARG_LSN(1);
+
+       if (lsn1 < lsn2)
+               PG_RETURN_INT32(-1);
+       if (lsn1 > lsn2)
+               PG_RETURN_INT32(1);
+       PG_RETURN_INT32(0);
+}

That's nearly what's in the patch I attached.

+/* 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

In this case you may consider changing timestamp_hash@time.c and
time_hash@date.c as well :)

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.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 7, 2014 at 8:22 AM, Andres Freund <andres@2ndquadrant.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.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, May 6, 2014 at 8:25 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Wed, May 7, 2014 at 8:22 AM, Andres Freund <andres@2ndquadrant.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.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

On 05/07/2014 07:16 AM, Michael Paquier wrote:

On Wed, May 7, 2014 at 8:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.

Could you be clearer? By applying a filterdiff command or by using git
diff? The latter has been used for this patch.

git format-patch -1

is usually what you want to emit a patch for the top commit. To produce
a patchset between the current branch and master:

git format-patch master

It doesn't use context diff, but I think people here have mostly stopped
caring about that now (?).

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 7, 2014 at 1:21 PM, Craig Ringer <craig@2ndquadrant.com> wrote:

On 05/07/2014 07:16 AM, Michael Paquier wrote:

On Wed, May 7, 2014 at 8:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-05-06 22:49:07 +0900, Michael Paquier wrote:
FWIW, the format you're using makes applying the patch including the
commit message relatively hard. Consider using git format-patch.

Could you be clearer? By applying a filterdiff command or by using git
diff? The latter has been used for this patch.

git format-patch -1

is usually what you want to emit a patch for the top commit. To produce
a patchset between the current branch and master:

git format-patch master

It doesn't use context diff, but I think people here have mostly stopped
caring about that now (?).

Thanks for the details, I didn't know this subcommand of git.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

On 2014-05-06 23:55:04 -0300, Fabrízio de Royes Mello wrote:

If helps, I added some regression tests to the lastest patch.

I think we should apply this patch now. It's much more sensible with the
opclasses present and we don't win anything by waiting for 9.5.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 7, 2014 at 11:55 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

On Tue, May 6, 2014 at 8:25 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Wed, May 7, 2014 at 8:22 AM, Andres Freund <andres@2ndquadrant.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.
I think that "pg_lsn_ops" is better than "pglsn_ops" because it's for "pg_lsn"
data type.

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@gmail.com> wrote:

On Tue, May 6, 2014 at 8:25 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Wed, May 7, 2014 at 8:22 AM, Andres Freund <andres@2ndquadrant.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, May 9, 2014 at 10:01 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

+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.
I think that "pg_lsn_ops" is better than "pglsn_ops" because it's for "pg_lsn"
data type.

Well, yes... Btw, before doing anything with this patch, I think that
the version of Fabrizio could be used as a base, but the regression
tests should be reshuffled a bit...
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-05-09 22:14:25 +0900, Michael Paquier wrote:

On Fri, May 9, 2014 at 10:01 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

+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.
I think that "pg_lsn_ops" is better than "pglsn_ops" because it's for "pg_lsn"
data type.

Well, yes... Btw, before doing anything with this patch, I think that
the version of Fabrizio could be used as a base, but the regression
tests should be reshuffled a bit...

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.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers