Обсуждение: New pg_lsn type doesn't have hash/btree opclasses

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.

Greetings,

Andres Freund

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

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

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

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

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

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

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

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_TIMESTAMPreturn hashint8(fcinfo);
#elsereturn 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

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

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

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

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

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

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

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

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

On Fri, May 9, 2014 at 10:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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?

Yes unless many people object the commit.

Michael,
You're now modifying the patch?

Regards,

--
Fujii Masao

On Fri, May 9, 2014 at 10:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Yes unless many people object the commit.
>
> Michael,
> You're now modifying the patch?
Not within a couple of days.
-- 
Michael

Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, May 9, 2014 at 10:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Yes unless many people object the commit.
>> 
>> Michael,
>> You're now modifying the patch?
> Not within a couple of days.

I think it's really too late for this for 9.4.  At this point it's
less than 48 hours until beta1 wraps, and we do not have the bandwidth
to do anything but worry about stabilizing the features we've already
got.
        regards, tom lane

<div dir="ltr"><br />On Fri, May 9, 2014 at 10:18 AM, Andres Freund <<a
href="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>>wrote:<br />><br />> On 2014-05-09 22:14:25
+0900,Michael Paquier wrote:<br />> > On Fri, May 9, 2014 at 10:01 PM, Fujii Masao <<a
href="mailto:masao.fujii@gmail.com">masao.fujii@gmail.com</a>>wrote:<br /> > > > +DATA(insert OID = 3260 (
  403        pglsn_ops        PGNSP PGUID ));<br />> > > +DATA(insert OID = 3261 (    405        pglsn_ops    
  PGNSP PGUID ));<br />> > ><br />> > > The patch looks good to me except the name of index operator
class.<br/> > > > I think that "pg_lsn_ops" is better than "pglsn_ops" because it's for "pg_lsn"<br />>
>> data type.<br />> > Well, yes... Btw, before doing anything with this patch, I think that<br /> >
>the version of Fabrizio could be used as a base, but the regression<br />> > tests should be reshuffled a
bit...<br/>><br />> I honestly don't really see much point in the added tests. If at all I'd<br /> > rather
seemy tests from<br />> <a
href="http://archives.postgresql.org/message-id/20140506230722.GE24808%40awork2.anarazel.de">http://archives.postgresql.org/message-id/20140506230722.GE24808%40awork2.anarazel.de</a><br
/>> addded. With some EXPLAIN (COSTS OFF) they'd test more.<br />><br /><br />Ok. In a few hours I'll add your
suggestionand send it again.<br /><br />Regards,<br /><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching
PostgreSQL<br/> >> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br />>> Blog
sobreTI: <a href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br />>> Perfil
Linkedin:<a href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br /> >>
Twitter:<a href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div> 

Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
> On Fri, May 9, 2014 at 8:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think it's really too late for this for 9.4.  At this point it's
>> less than 48 hours until beta1 wraps, and we do not have the bandwidth
>> to do anything but worry about stabilizing the features we've already
>> got.

> But it's a very small change with many benefits, and Michael acted very
> proactive to make this happens.

[ shrug... ]  "proactive" would have been doing this a month ago.
If we're going to ship a release, we have to stop taking new features
at some point, and we are really past that point for 9.4.

And, to be blunt, this is not important enough to hold up the release
for, nor to take any stability risks for.  It should go into the next
commitfest cycle where it can get a non-rushed review.
        regards, tom lane

On Fri, May 9, 2014 at 10:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, May 9, 2014 at 10:03 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> You plan to commit it?
>
> Yes unless many people object the commit.
>
> Michael, you're now modifying the patch?
OK, I have been able to put my head into that earlier than I thought
as it would be better to get that done before the beta, finishing with
the attached. When hacking that, I noticed that some tests were
missing for some of the operators. I am including them in this patch
as well, with more tests for unique index creation, ordering, and
hash/btree index creation. The test suite looks cleaner with all that.
--
Michael

On Sat, May 10, 2014 at 9:51 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Fri, May 9, 2014 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
>> > On Fri, May 9, 2014 at 8:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >> I think it's really too late for this for 9.4.  At this point it's
>> >> less than 48 hours until beta1 wraps, and we do not have the bandwidth
>> >> to do anything but worry about stabilizing the features we've already
>> >> got.
>>
>> > But it's a very small change with many benefits, and Michael acted very
>> > proactive to make this happens.
>>
>> [ shrug... ]  "proactive" would have been doing this a month ago.
>> If we're going to ship a release, we have to stop taking new features
>> at some point, and we are really past that point for 9.4.
>>
>> And, to be blunt, this is not important enough to hold up the release
>> for, nor to take any stability risks for.  It should go into the next
>> commitfest cycle where it can get a non-rushed review.
>>
>
> I agree with you that is too late to add *new features*.
>
> But I agree with Andres when he said this is a regression introcuced in the
> pg_lsn patch.
>
> So we'll release a version that break a simple query like that:
>
> fabrizio=# SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1,
> 100) g(i), generate_series(1, 5);
> ERROR:  could not identify an equality operator for type pg_lsn
> LINE 1: SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1...
>                          ^

I agree that this is not new feature but just the fix of oversight of
the pg_lsn patch.
Without such opclass, we cannot execute even such simple query.

Regards,

--
Fujii Masao

On 2014-05-11 06:02:23 +0900, Fujii Masao wrote:
> On Sat, May 10, 2014 at 9:51 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> >
> > On Fri, May 9, 2014 at 9:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> [ shrug... ]  "proactive" would have been doing this a month ago.
> >> If we're going to ship a release, we have to stop taking new features
> >> at some point, and we are really past that point for 9.4.

We *couldn't* do it a month ago since we didn't know about it a month
ago. My delorean is getting a bit old.


> > I agree with you that is too late to add *new features*.
> >
> > But I agree with Andres when he said this is a regression introcuced in the
> > pg_lsn patch.
> >
> > So we'll release a version that break a simple query like that:
> >
> > fabrizio=# SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1,
> > 100) g(i), generate_series(1, 5);
> > ERROR:  could not identify an equality operator for type pg_lsn
> > LINE 1: SELECT DISTINCT (g.i||'/0')::pg_lsn f FROM generate_series(1...
> >                          ^
>
> I agree that this is not new feature but just the fix of oversight of
> the pg_lsn patch.
> Without such opclass, we cannot execute even such simple query.

I don't even understand why it's questionable whether this should be
fixed. It's an almost trivial patch for an oversight in a newly
introduced feature. We gain absolutely nothing by patching this in the
next cycle, except that things need to be tested twice.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> I don't even understand why it's questionable whether this should be
> fixed.

Sigh.  We have some debate isomorphic to this one every year, it seems
like.  The argument why it shouldn't be fixed now is: ITS. TOO. LATE.
Which part of that isn't clear to you?

Or, if you think that this feature is so important that we should slip
the beta schedule to get it in, we can take a vote on that.  But at
this point any slip means no beta till after PGCon.
        regards, tom lane

On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote:
> On Sat, May 10, 2014 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Andres Freund <andres@2ndquadrant.com> writes:
> > > I don't even understand why it's questionable whether this should be
> > > fixed.
> >
> > Sigh.  We have some debate isomorphic to this one every year, it seems
> > like.  The argument why it shouldn't be fixed now is: ITS. TOO. LATE.
> > Which part of that isn't clear to you?
> >
>
> Sorry but I don't understand why it's too late. The 9.4 branch not been
> created yet.

The problem is that once the beta is in progress (starting tomorrow),
the projects tries to avoid changes which require a dump and restore (or
pg_upgrade). Since the patch changes the catalog it'd require that.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-05-10 19:19:22 -0300, Fabr�zio de Royes Mello wrote:
>> Sorry but I don't understand why it's too late. The 9.4 branch not been
>> created yet.

> The problem is that once the beta is in progress (starting tomorrow),
> the projects tries to avoid changes which require a dump and restore (or
> pg_upgrade). Since the patch changes the catalog it'd require that.

Exactly.  If this isn't in before beta1, it's not going in, unless perhaps
we encounter some bug bad enough to force an initdb later in beta.
Which is certainly possible, and if it did happen there might be room
to reconsider.  But the same policy means there is zero margin for error
with a catalog-changing patch applied right before beta starts, which
is what we're debating here.

As a comparison point, in a nearby thread we're debating renaming
jsonb_hash_ops, which is a sufficiently mechanical change that I'd
not be too afraid to do it the day before beta.  But there are enough
ways to screw up a new operator class that I'm very afraid of putting
one in this weekend.
        regards, tom lane

On 2014-05-11 00:31:09 +0200, Magnus Hagander wrote:
> On Sun, May 11, 2014 at 12:27 AM, Andres Freund <andres@2ndquadrant.com>wrote:
>
> > On 2014-05-10 19:19:22 -0300, Fabrízio de Royes Mello wrote:
> > > On Sat, May 10, 2014 at 6:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > >
> > > > Andres Freund <andres@2ndquadrant.com> writes:
> > > > > I don't even understand why it's questionable whether this should be
> > > > > fixed.
> > > >
> > > > Sigh.  We have some debate isomorphic to this one every year, it seems
> > > > like.  The argument why it shouldn't be fixed now is: ITS. TOO. LATE.
> > > > Which part of that isn't clear to you?
> > > >
> > >
> > > Sorry but I don't understand why it's too late. The 9.4 branch not been
> > > created yet.
> >
> > The problem is that once the beta is in progress (starting tomorrow),
> > the projects tries to avoid changes which require a dump and restore (or
> > pg_upgrade). Since the patch changes the catalog it'd require that.
> >
> >
> It would be pg_upgrade'able though, wouldn't it?

Yes.

> Don't we have precedents
> for requiring pg_upgrade during beta? At least that's a smaller problem
> than requiring a complete dump/reload.

I think in reality there's been catversion updates after most of the
recent beta1 releases.

9.3 (one):
git log 817a89423f429a6a8b36847ee499f5b6be39c3be.. -- src/include/catalog/catversion.h
9.2 (one, but reverted):
git log f70fa835e08eee4cb2dc0f72d66cf633089c37e8..REL9_2_0 -- src/include/catalog/catversion.h
9.1 (two):
git log 993c5e59047dd568d4831f7ec5c6199acd21f17f..REL9_1_0 -- src/include/catalog/catversion.h
9.0 (one)
git log -p f9d9b2b34a094b94fda39231c16ab5f2e6bfbbe4..REL9_0_0 -- src/include/catalog/catversion.h

(makes me really wish betas were properly tagged with git as well...)

Greetings,

Andres Freund

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

On Sun, May 11, 2014 at 7:39 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> (makes me really wish betas were properly tagged with git as well...)
They are tags for betas, here is for example the update of CATVERSION for 9.3:
$ git log -p REL9_3_BETA1..REL9_3_0 src/include/catalog/catversion.h |
grep commit
commit dc3eb5638349e74a6628130a5101ce866455f4a3
-- 
Michael

Magnus Hagander <magnus@hagander.net> writes:
> On Sun, May 11, 2014 at 12:27 AM, Andres Freund <andres@2ndquadrant.com>wrote:
>> The problem is that once the beta is in progress (starting tomorrow),
>> the projects tries to avoid changes which require a dump and restore (or
>> pg_upgrade). Since the patch changes the catalog it'd require that.

> It would be pg_upgrade'able though, wouldn't it? Don't we have precedents
> for requiring pg_upgrade during beta? At least that's a smaller problem
> than requiring a complete dump/reload.

pg_upgrade makes the penalty for screwups smaller, but a post-beta1 initdb
is still the result of a screwup.  None of the historical examples you
mention were planned in advance of beta.
        regards, tom lane

On 2014-05-10 19:08:48 -0400, Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
> > On Sun, May 11, 2014 at 12:27 AM, Andres Freund <andres@2ndquadrant.com>wrote:
> >> The problem is that once the beta is in progress (starting tomorrow),
> >> the projects tries to avoid changes which require a dump and restore (or
> >> pg_upgrade). Since the patch changes the catalog it'd require that.
> 
> > It would be pg_upgrade'able though, wouldn't it? Don't we have precedents
> > for requiring pg_upgrade during beta? At least that's a smaller problem
> > than requiring a complete dump/reload.
> 
> pg_upgrade makes the penalty for screwups smaller, but a post-beta1 initdb
> is still the result of a screwup.  None of the historical examples you
> mention were planned in advance of beta.

Yea, I posted that just to answer Magnus' question.

I've argued that this omission should be fixed since tuesday. There's
been a tested and reviewed patch since
20140506230722.GE24808@awork2.anarazel.de. Given how many changes went
in since it certainly wouldn't have been a very destabilizing commit.

Anyway. I accept it's too late for beta1 now. Let's commit it if there's
another catversion bump.

Greetings,

Andres Freund

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

On Sat, May 10, 2014 at 9:45 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> Last night I sent a patch [1] to add more tests and change the operator
> name. Maybe we can merge the test cases... ;-)
Sure, I noticed that. But I think that they are more complicated than
necessary. I am as well doubtful about adding test cases with EXPLAIN
for a data type test suite.

The patch introduces two new things: pg_lsn_cmp and pg_lsn_hash. To
test the former a simple ORDER BY query is enough as cmp is used as an
ordering operator. And for the latter creating a hash index looks
enough as it tests using the hash function when inserting index
tuples. Not to mention as well that the tests are more readable.
Regards,
--
Michael

On Sun, May 11, 2014 at 8:17 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> Anyway. I accept it's too late for beta1 now. Let's commit it if there's
> another catversion bump.
+1. Let's rely on the experience of senior committers here.
-- 
Michael

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

On Thu, Jun 5, 2014 at 9:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.
Thanks!

> 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.)
Yeah this was a bit too much, and came up with some more light-weight
regression tests instead in the patch here:
http://www.postgresql.org/message-id/CAB7nPqSgZVrHRgsgUg63SCFY+AwH-=3JuDy7moq-_fo7Wi4++Q@mail.gmail.com
-- 
Michael