Обсуждение: BUG #9652: inet types don't support min/max

The following bug has been logged on the website:

Bug reference:      9652
Logged by:          Daniel O'Connor
Email address:      darius@dons.net.au
PostgreSQL version: 9.2.7
Operating system:   OSX Mavericks
Description:

reclog=> select * from foo;
   bar
---------
 1.2.3.4
(1 row)

reclog=> select min(bar) from foo;
ERROR:  function min(inet) does not exist
LINE 1: select min(bar) from foo;
               ^
HINT:  No function matches the given name and argument types. You might need
to add explicit type casts.

This is surprising to me since the inet type is ordered, hence min/max are
possible.

You also can't cast an inet to an integer or bigint although I guess that
isn't too surprising since v6 addrs are larger than bigints.

On Fri, Mar 21, 2014 at 5:17 PM,  <darius@dons.net.au> wrote:
> The following bug has been logged on the website:
> reclog=> select * from foo;
>    bar
> ---------
>  1.2.3.4
> (1 row)
>
> reclog=> select min(bar) from foo;
> ERROR:  function min(inet) does not exist
> LINE 1: select min(bar) from foo;
>                ^
> HINT:  No function matches the given name and argument types. You might need
> to add explicit type casts.
>
> This is surprising to me since the inet type is ordered, hence min/max are
> possible.

In the code, some comparison logic for the inet datatypes already present.
With those I thought it is easy to support the min and max aggregates also.
So I modified the code to support the aggregate functions of min and
max by using
the already existing inet comparison logic. Is there anything I am missing?

postgres=# create table tbl(f inet);
CREATE TABLE
postgres=# insert into tbl values('1.2.3.5');
INSERT 0 1
postgres=# insert into tbl values('1.2.3.4');
INSERT 0 1
postgres=# select min(f) from tbl;
   min
---------
 1.2.3.4
(1 row)

postgres=# select max(f) from tbl;
   max
---------
 1.2.3.5
(1 row)

Patch is attached.
This is the first time I am touching this area so please let me know
your suggestions.

Regards,
Hari Babu
Fujitsu Australia

On Sun, Mar 23, 2014 at 10:42 PM, Haribabu Kommi
<kommi.haribabu@gmail.com>wrote:

> On Fri, Mar 21, 2014 at 5:17 PM,  <darius@dons.net.au> wrote:
> > The following bug has been logged on the website:
> > reclog=> select * from foo;
> >    bar
> > ---------
> >  1.2.3.4
> > (1 row)
> >
> > reclog=> select min(bar) from foo;
> > ERROR:  function min(inet) does not exist
> > LINE 1: select min(bar) from foo;
> >                ^
> > HINT:  No function matches the given name and argument types. You might
> need
> > to add explicit type casts.
> >
> > This is surprising to me since the inet type is ordered, hence min/max
> are
> > possible.
>
> In the code, some comparison logic for the inet datatypes already present.
> With those I thought it is easy to support the min and max aggregates also.
> So I modified the code to support the aggregate functions of min and
> max by using
> the already existing inet comparison logic. Is there anything I am missing?
>
> postgres=# create table tbl(f inet);
> CREATE TABLE
> postgres=# insert into tbl values('1.2.3.5');
> INSERT 0 1
> postgres=# insert into tbl values('1.2.3.4');
> INSERT 0 1
> postgres=# select min(f) from tbl;
>    min
> ---------
>  1.2.3.4
> (1 row)
>
> postgres=# select max(f) from tbl;
>    max
> ---------
>  1.2.3.5
> (1 row)
>
> Patch is attached.
> This is the first time I am touching this area so please let me know
> your suggestions.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
>
> --
> Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>
>
This is my first time reviewing a patch, so apologies if I've missed
something in the process.

I tried applying your patch to the current git HEAD as of 2014-05-27 and
the portion against pg_aggregate.h fails

postgres $ patch -Np1 -i ../inet_agg.patch
patching file src/backend/utils/adt/network.c
Hunk #1 succeeded at 471 (offset -49 lines).
patching file src/include/catalog/pg_aggregate.h
Hunk #1 FAILED at 140.
Hunk #2 FAILED at 162.
2 out of 2 hunks FAILED -- saving rejects to file
src/include/catalog/pg_aggregate.h.rej
patching file src/include/catalog/pg_proc.h
Hunk #1 succeeded at 2120 (offset 8 lines).
Hunk #2 succeeded at 3163 (offset 47 lines).
Hunk #3 succeeded at 3204 (offset 47 lines).
patching file src/include/utils/builtins.h
Hunk #1 succeeded at 907 (offset 10 lines).

Looks like starting around April 12th some additional changes were made to
the DATA lines in this file that have to be accounted for.

https://github.com/postgres/postgres/commits/master/src/include/catalog/pg_aggregate.h

Don't have the knowledge of how to fix this for the new format, but thought
I'd at least try to apply the patch and see if it works.

--
Keith Fiske
Database Administrator
OmniTI Computer Consulting, Inc.
http://www.keithf4.com

On Thu, May 29, 2014 at 3:28 AM, Keith Fiske <keith@omniti.com> wrote:
> On Sun, Mar 23, 2014 at 10:42 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>>
>> On Fri, Mar 21, 2014 at 5:17 PM,  <darius@dons.net.au> wrote:
>> > The following bug has been logged on the website:
>> > reclog=> select * from foo;
>> >    bar
>> > ---------
>> >  1.2.3.4
>> > (1 row)
>> >
>> > reclog=> select min(bar) from foo;
>> > ERROR:  function min(inet) does not exist
>> > LINE 1: select min(bar) from foo;
>> >                ^
>> > HINT:  No function matches the given name and argument types. You might
>> > need
>> > to add explicit type casts.
>> >
>> > This is surprising to me since the inet type is ordered, hence min/max
>> > are
>> > possible.
>>
>> In the code, some comparison logic for the inet datatypes already present.
>> With those I thought it is easy to support the min and max aggregates
>> also.
>> So I modified the code to support the aggregate functions of min and
>> max by using
>> the already existing inet comparison logic. Is there anything I am
>> missing?
>>
>> postgres=# create table tbl(f inet);
>> CREATE TABLE
>> postgres=# insert into tbl values('1.2.3.5');
>> INSERT 0 1
>> postgres=# insert into tbl values('1.2.3.4');
>> INSERT 0 1
>> postgres=# select min(f) from tbl;
>>    min
>> ---------
>>  1.2.3.4
>> (1 row)
>>
>> postgres=# select max(f) from tbl;
>>    max
>> ---------
>>  1.2.3.5
>> (1 row)
>>
>> Patch is attached.
>
> This is my first time reviewing a patch, so apologies if I've missed
> something in the process.
>
> I tried applying your patch to the current git HEAD as of 2014-05-27 and the
> portion against pg_aggregate.h fails
>
> postgres $ patch -Np1 -i ../inet_agg.patch
> patching file src/backend/utils/adt/network.c
> Hunk #1 succeeded at 471 (offset -49 lines).
> patching file src/include/catalog/pg_aggregate.h
> Hunk #1 FAILED at 140.
> Hunk #2 FAILED at 162.
> 2 out of 2 hunks FAILED -- saving rejects to file
> src/include/catalog/pg_aggregate.h.rej
> patching file src/include/catalog/pg_proc.h
> Hunk #1 succeeded at 2120 (offset 8 lines).
> Hunk #2 succeeded at 3163 (offset 47 lines).
> Hunk #3 succeeded at 3204 (offset 47 lines).
> patching file src/include/utils/builtins.h
> Hunk #1 succeeded at 907 (offset 10 lines).
>
> Looks like starting around April 12th some additional changes were made to
> the DATA lines in this file that have to be accounted for.
>
> https://github.com/postgres/postgres/commits/master/src/include/catalog/pg_aggregate.h
>
> Don't have the knowledge of how to fix this for the new format, but thought
> I'd at least try to apply the patch and see if it works.

Thanks for verifying the patch. Please find the re-based patch
attached in the mail.

Regards,
Hari Babu
Fujitsu Australia

Hi,

On 2014-06-03 12:43:28 +1000, Haribabu Kommi wrote:
> *** a/src/test/regress/expected/create_function_3.out
> --- b/src/test/regress/expected/create_function_3.out
> ***************
> *** 153,389 **** RESET SESSION AUTHORIZATION;
>   SELECT proname, prorettype::regtype, proargtypes::regtype[]
>          FROM pg_proc JOIN pg_namespace ON pronamespace = pg_namespace.oid
>          WHERE nspname = 'pg_catalog' AND proleakproof ORDER BY proname;
> !     proname     | prorettype |                             proargtypes                             
> ! ----------------+------------+---------------------------------------------------------------------
> !  abstimeeq      | boolean    | [0:1]={abstime,abstime}
...
> !  xideq          | boolean    | [0:1]={xid,xid}
> ! (228 rows)
>   
>   --
>   -- CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT
> --- 153,391 ----
>   SELECT proname, prorettype::regtype, proargtypes::regtype[]
>          FROM pg_proc JOIN pg_namespace ON pronamespace = pg_namespace.oid
>          WHERE nspname = 'pg_catalog' AND proleakproof ORDER BY proname;
> !      proname     | prorettype |                             proargtypes                             
> ! -----------------+------------+---------------------------------------------------------------------
> !  abstimeeq       | boolean    | [0:1]={abstime,abstime}
...
> !  xideq           | boolean    | [0:1]={xid,xid}
> ! (230 rows)

I didn't reall look at the patch, but it very much looks to me like that
query result could use the \a\t treatment that rules.sql and
sanity_check.sql got. It's hard to see the actual difference
before/after the patch.
I'll patch that now, to reduce the likelihood of changes there causing
conflicts for more people.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> I didn't reall look at the patch, but it very much looks to me like that
> query result could use the \a\t treatment that rules.sql and
> sanity_check.sql got. It's hard to see the actual difference
> before/after the patch.
> I'll patch that now, to reduce the likelihood of changes there causing
> conflicts for more people.

Personally, I would wonder why the regression tests contain such a query
in the first place.  It seems like nothing but a major maintenance PITA.
        regards, tom lane

On 2014-06-03 10:24:46 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > I didn't reall look at the patch, but it very much looks to me like that
> > query result could use the \a\t treatment that rules.sql and
> > sanity_check.sql got. It's hard to see the actual difference
> > before/after the patch.
> > I'll patch that now, to reduce the likelihood of changes there causing
> > conflicts for more people.
> 
> Personally, I would wonder why the regression tests contain such a query
> in the first place.  It seems like nothing but a major maintenance PITA.

I haven't added it, but it seems appropriate in that specific case. The
number of leakproof functions should be fairly small and every addition
should be carefully reviewed... I am e.g. not sure that it's a good idea
to declare network_smaller/greater as leakproof - but it's hard to catch
that on the basic of pg_proc.h alone.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-06-03 10:24:46 -0400, Tom Lane wrote:
>> Personally, I would wonder why the regression tests contain such a query
>> in the first place.  It seems like nothing but a major maintenance PITA.

> I haven't added it, but it seems appropriate in that specific case. The
> number of leakproof functions should be fairly small and every addition
> should be carefully reviewed... I am e.g. not sure that it's a good idea
> to declare network_smaller/greater as leakproof - but it's hard to catch
> that on the basic of pg_proc.h alone.

Meh.  I agree that new leakproof functions should be carefully reviewed,
but I have precisely zero faith that this regression test will contribute
to that.  It hasn't even got a comment saying why changes here should
receive any scrutiny; moreover, it's not in a file where changes would be
likely to excite suspicion.  (Probably it should be in opr_sanity, if
we're going to have such a thing at all.)
        regards, tom lane

On 2014-06-03 10:37:53 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-06-03 10:24:46 -0400, Tom Lane wrote:
> >> Personally, I would wonder why the regression tests contain such a query
> >> in the first place.  It seems like nothing but a major maintenance PITA.
> 
> > I haven't added it, but it seems appropriate in that specific case. The
> > number of leakproof functions should be fairly small and every addition
> > should be carefully reviewed... I am e.g. not sure that it's a good idea
> > to declare network_smaller/greater as leakproof - but it's hard to catch
> > that on the basic of pg_proc.h alone.
> 
> Meh.  I agree that new leakproof functions should be carefully reviewed,
> but I have precisely zero faith that this regression test will contribute
> to that.

Well, I personally wouldn't have noticed that the OP's patch marked the
new functions as leakproof without that test. At least not while looking
at the patch. pg_proc.h is just too hard to read.

> It hasn't even got a comment saying why changes here should
> receive any scrutiny; moreover, it's not in a file where changes would be
> likely to excite suspicion.  (Probably it should be in opr_sanity, if
> we're going to have such a thing at all.)

I don't object to moving it there.

Greetings,

Andres Freund

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

<div dir="ltr"><br /><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 3, 2014 at 10:48 AM, Andres Freund
<spandir="ltr"><<a href="mailto:andres@2ndquadrant.com" target="_blank">andres@2ndquadrant.com</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex"><divclass="">On 2014-06-03 10:37:53 -0400, Tom Lane wrote:<br /> > Andres Freund
<<ahref="mailto:andres@2ndquadrant.com">andres@2ndquadrant.com</a>> writes:<br /> > > On 2014-06-03
10:24:46-0400, Tom Lane wrote:<br /> > >> Personally, I would wonder why the regression tests contain such a
query<br/> > >> in the first place.  It seems like nothing but a major maintenance PITA.<br /> ><br /> >
>I haven't added it, but it seems appropriate in that specific case. The<br /> > > number of leakproof
functionsshould be fairly small and every addition<br /> > > should be carefully reviewed... I am e.g. not sure
thatit's a good idea<br /> > > to declare network_smaller/greater as leakproof - but it's hard to catch<br />
>> that on the basic of pg_proc.h alone.<br /> ><br /> > Meh.  I agree that new leakproof functions should
becarefully reviewed,<br /> > but I have precisely zero faith that this regression test will contribute<br /> >
tothat.<br /><br /></div>Well, I personally wouldn't have noticed that the OP's patch marked the<br /> new functions as
leakproofwithout that test. At least not while looking<br /> at the patch. pg_proc.h is just too hard to read.<br
/><divclass=""><br /> > It hasn't even got a comment saying why changes here should<br /> > receive any scrutiny;
moreover,it's not in a file where changes would be<br /> > likely to excite suspicion.  (Probably it should be in
opr_sanity,if<br /> > we're going to have such a thing at all.)<br /><br /></div>I don't object to moving it
there.<br/><div class=""><div class="h5"><br /> Greetings,<br /><br /> Andres Freund<br /><br /> --<br />  Andres
Freund                    <a href="http://www.2ndQuadrant.com/" target="_blank">http://www.2ndQuadrant.com/</a><br />
 PostgreSQLDevelopment, 24x7 Support, Training & Services<br /></div></div></blockquote></div><br /><br
/></div><divclass="gmail_extra">Andres's changes on June 3rd to <br /><a
href="https://github.com/postgres/postgres/commits/master/src/test/regress/expected/create_function_3.out">https://github.com/postgres/postgres/commits/master/src/test/regress/expected/create_function_3.out</a><br
/></div><divclass="gmail_extra">are causing patch v2 to fail for that regression test file. <br /><br />postgres $
patch-p1 -i ../inet_agg_v2.patch <br />patching file src/backend/utils/adt/network.c<br />patching file
src/include/catalog/pg_aggregate.h<br/> patching file src/include/catalog/pg_proc.h<br />patching file
src/include/utils/builtins.h<br/>patching file src/test/regress/expected/create_function_3.out<br />Hunk #1 FAILED at
153.<br/>1 out of 1 hunk FAILED -- saving rejects to file src/test/regress/expected/create_function_3.out.rej<br />
patchingfile src/test/regress/expected/inet.out<br />patching file src/test/regress/sql/inet.sql<br /><br />Otherwise
itapplies without any issues to the latest HEAD. I built and started a new instance, and I was able to test at least
thebasic min/max functionality<br /><br />keith=# create table test_inet (id serial, ipaddress inet);<br />CREATE
TABLE<br/>Time: 25.546 ms<br />keith=# insert into test_inet (ipaddress) values ('192.168.1.1');<br />INSERT 0 1<br
/>Time:3.143 ms<br />keith=# insert into test_inet (ipaddress) values ('192.168.1.2');<br /> INSERT 0 1<br />Time:
2.932ms<br />keith=# insert into test_inet (ipaddress) values ('127.0.0.1');<br />INSERT 0 1<br />Time: 1.786 ms<br
/>keith=#select min(ipaddress) from test_inet;<br />    min    <br />-----------<br /> 127.0.0.1<br /> (1 row)<br /><br
/>Time:3.371 ms<br />keith=# select max(ipaddress) from test_inet;<br />     max     <br />-------------<br
/> 192.168.1.2<br/>(1 row)<br /><br />Time: 1.104 ms<br /><br /></div><div class="gmail_extra">--<br />Keith Fiske<br
/>Database Administrator<br />OmniTI Computer Consulting, Inc.<br /><a href="http://www.keithf4.com"
target="_blank">http://www.keithf4.com</a><br/></div></div> 

On Wed, Jun 4, 2014 at 5:46 AM, Keith Fiske <keith@omniti.com> wrote:
>
> Andres's changes on June 3rd to
> https://github.com/postgres/postgres/commits/master/src/test/regress/expected/create_function_3.out
> are causing patch v2 to fail for that regression test file.
>
> postgres $ patch -p1 -i ../inet_agg_v2.patch
> patching file src/backend/utils/adt/network.c
> patching file src/include/catalog/pg_aggregate.h
> patching file src/include/catalog/pg_proc.h
> patching file src/include/utils/builtins.h
> patching file src/test/regress/expected/create_function_3.out
> Hunk #1 FAILED at 153.
> 1 out of 1 hunk FAILED -- saving rejects to file
> src/test/regress/expected/create_function_3.out.rej
> patching file src/test/regress/expected/inet.out
> patching file src/test/regress/sql/inet.sql
>
> Otherwise it applies without any issues to the latest HEAD. I built and
> started a new instance, and I was able to test at least the basic min/max
> functionality
>
> keith=# create table test_inet (id serial, ipaddress inet);
> CREATE TABLE
> Time: 25.546 ms
> keith=# insert into test_inet (ipaddress) values ('192.168.1.1');
> INSERT 0 1
> Time: 3.143 ms
> keith=# insert into test_inet (ipaddress) values ('192.168.1.2');
> INSERT 0 1
> Time: 2.932 ms
> keith=# insert into test_inet (ipaddress) values ('127.0.0.1');
> INSERT 0 1
> Time: 1.786 ms
> keith=# select min(ipaddress) from test_inet;
>     min
> -----------
>  127.0.0.1
> (1 row)
>
> Time: 3.371 ms
> keith=# select max(ipaddress) from test_inet;
>      max
> -------------
>  192.168.1.2
> (1 row)
>
> Time: 1.104 ms

Thanks for the test. Patch is re-based to the latest head.

Regards,
Hari Babu
Fujitsu Australia

On 2014-06-03 10:37:53 -0400, Tom Lane wrote:
> It hasn't even got a comment saying why changes here should
> receive any scrutiny; moreover, it's not in a file where changes would be
> likely to excite suspicion.  (Probably it should be in opr_sanity, if
> we're going to have such a thing at all.)

I've written up the attached patch that moves the test to opr_sanity and
adds a littlebit of commentary. Will apply unless somebody protests in
the next 24h or so.

Greetings,

Andres Freund

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

Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-06-03 10:37:53 -0400, Tom Lane wrote:
>> It hasn't even got a comment saying why changes here should
>> receive any scrutiny; moreover, it's not in a file where changes would be
>> likely to excite suspicion.  (Probably it should be in opr_sanity, if
>> we're going to have such a thing at all.)

> I've written up the attached patch that moves the test to opr_sanity and
> adds a littlebit of commentary. Will apply unless somebody protests in
> the next 24h or so.

+1, but as long as we're touching this, could we make the output be

SELECT oid::regprocedure, prorettype::regtype FROM pg_proc ...

Same information, but more readable IMO.  (I'm not really sure why
we need to show prorettype here at all, btw.)
        regards, tom lane

Hi,

On 2014-06-04 10:37:48 +1000, Haribabu Kommi wrote:
> Thanks for the test. Patch is re-based to the latest head.

Did you look at the source of the conflict? Did you intentionally mark
the functions as leakproof and reviewed that that truly is the case? Or
was that caused by copy & paste?

Greetings,

Andres Freund

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

On Thu, Jun 5, 2014 at 9:12 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> Hi,
>
> On 2014-06-04 10:37:48 +1000, Haribabu Kommi wrote:
>> Thanks for the test. Patch is re-based to the latest head.
>
> Did you look at the source of the conflict? Did you intentionally mark
> the functions as leakproof and reviewed that that truly is the case? Or
> was that caused by copy & paste?

Yes it is copy & paste mistake. I didn't know much about that parameter.
Thanks for the information. I changed it.

Regards,
Hari Babu
Fujitsu Australia