Thread: Re: [BUGS] BUG #9652: inet types don't support min/max

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

At 2014-06-30 16:35:45 +0500, anaeem.it@gmail.com wrote:
>
> pc1dotnetpk:postgresql asif$ patch -p0 <
> > ~/core/min_max_support_for_inet_datatypes/inet_agg_v4.patch
> > can't find file to patch at input line 3
> > Perhaps you used the wrong -p or --strip option?
> > The text leading up to this was:
> > --------------------------
> > |*** a/src/backend/utils/adt/network.c
> > |--- b/src/backend/utils/adt/network.c
> > --------------------------

You need to use patch -p1, to strip off the "a"/"b" prefix.

-- Abhijit

On Mon, Jul 7, 2014 at 6:59 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Hi Haribabu,
>
> Thank you for sharing the patch. I have spent some time to review the
> changes. Overall patch looks good to me, make check and manual testing seems
> run fine with it. There seems no related doc/sgml changes ?. Patch added
> network_smaller() and network_greater() functions but in PG source code,
> general practice seems to be to use “smaller" and “larger” as related
> function name postfix e.g. timestamp_smaller()/timestamp_larger(),
> interval_smaller/interval_larger(), cashsmaller()/cashlarger() etc. Thanks.

Thanks for reviewing the patch.

I corrected the function names as smaller and larger.
and also added documentation changes.

Updated patch attached in the mail.

Regards,
Hari Babu
Fujitsu Australia

On Thu, Jul 24, 2014 at 5:59 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Sorry for being late. Thank you for sharing updated patch, sgml changes
> seems not working i.e.
>
>> postgres=# select max('192.168.1.5', '192.168.1.4');
>> ERROR:  function max(unknown, unknown) does not exist
>> LINE 1: select max('192.168.1.5', '192.168.1.4');
>>
>>                ^
>> HINT:  No function matches the given name and argument types. You might
>> need to add explicit type casts.
>> postgres=# select min('192.168.1.5', '192.168.1.4');
>> ERROR:  function min(unknown, unknown) does not exist
>> LINE 1: select min('192.168.1.5', '192.168.1.4');
>>
>>                ^
>> HINT:  No function matches the given name and argument types. You might
>> need to add explicit type casts.

I didn't get your point. I tested the patch, the sgml changes are working fine.

> I would suggest the following or similar changes e.g.
>
>> doc/src/sgml/func.sgml
>>         </indexterm>
>>         <function>max(<replaceable
>> class="parameter">expression</replaceable>)</function>
>>        </entry>
>> -      <entry>any array, numeric, string, or date/time type</entry>
>> +      <entry>any inet, array, numeric, string, or date/time type</entry>
>>        <entry>same as argument type</entry>
>>        <entry>
>>         maximum value of <replaceable
>> @@ -12204,7 +12228,7 @@ NULL baz</literallayout>(3 rows)</entry>
>>         </indexterm>
>>         <function>min(<replaceable
>> class="parameter">expression</replaceable>)</function>
>>        </entry>
>> -      <entry>any array, numeric, string, or date/time type</entry>
>> +      <entry>any inet, array, numeric, string, or date/time type</entry>
>>        <entry>same as argument type</entry>
>>        <entry>
>>         minimum value of <replaceable

Thanks for reviewing the patch.
I missed the above change. Corrected the same in the attached patch.

Regards,
Hari Babu
Fujitsu Australia

On Mon, Aug 4, 2014 at 3:22 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Sorry for not being clear, above mentioned test is related to following doc (sgml) changes that seems not working as
describedi.e. 
>
> Table 9-35. cidr and inet Functions
>
> FunctionReturn TypeDescriptionExampleResult
>
> max(inet, inet) inetlarger of two inet typesmax('192.168.1.5', '192.168.1.4')192.168.1.5
> min(inet, inet) inetsmaller of two inet typesmin('192.168.1.5', '192.168.1.4')192.168.1.4
>
> Can you please elaborate these sgml change ?

I thought of adding them for newly added "network" functions but
mistakenly I kept the names as max and min.
Anyway with your suggestion in the earlier mail, these changes are not required.

I removed these changes in the attached patch.
Thanks for reviewing the patch.

Regards,
Hari Babu
Fujitsu Australia

On Mon, Aug 18, 2014 at 8:12 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thank you for sharing updated patch. With latest 9.5 source code, patch
> build is failing with following error message i.e.
>
>> /Applications/Xcode.app/Contents/Developer/usr/bin/make -C catalog
>> schemapg.h
>> cd ../../../src/include/catalog && '/opt/local/bin/perl' ./duplicate_oids
>> 3255
>> make[3]: *** [postgres.bki] Error 1
>> make[2]: *** [submake-schemapg] Error 2
>> make[1]: *** [all-backend-recurse] Error 2
>> make: *** [all-src-recurse] Error 2
>
>
> New function is being added by following commit i.e.
>
>> commit b34e37bfefbed1bf9396dde18f308d8b96fd176c
>> Author: Robert Haas <rhaas@postgresql.org>
>> Date:   Thu Aug 14 12:09:52 2014 -0400
>>     Add sortsupport routines for text.
>>     This provides a small but worthwhile speedup when sorting text, at
>> least
>>     in cases to which the sortsupport machinery applies.
>>     Robert Haas and Peter Geoghegan
>
>
> It seems that you need to use another oid. Other than this patch looks good
> to me, please share updated patch and feel free to assign it to committer.
> Thanks.

Thanks for your review. Please find the rebased patch to latest HEAD.

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> Thanks for your review. Please find the rebased patch to latest HEAD.

Committed with minor (mostly cosmetic) alterations.
        regards, tom lane

On Fri, Aug 29, 2014 at 12:39 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> Thanks for your review. Please find the rebased patch to latest HEAD.
>
> Committed with minor (mostly cosmetic) alterations.

Thanks.

Regards,
Hari Babu
Fujitsu Australia