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

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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Andres Freund
Date:
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



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

From
Tom Lane
Date:
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



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

From
Andres Freund
Date:
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



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

From
Tom Lane
Date:
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



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

From
Andres Freund
Date:
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



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

From
Keith Fiske
Date:
<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> 

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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Keith Fiske
Date:



On Tue, Jun 3, 2014 at 8:37 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
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


Applying patch against latest HEAD (654e8e444749f053c3bf3fd543d10deb6aa6dd09) with no issues

$ patch -p1 -i ../inet_agg_v3.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
patching file src/test/regress/expected/inet.out
patching file src/test/regress/sql/inet.sql

Ran same min/max test as before and worked without issues.

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

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

From
Andres Freund
Date:
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

Attachment

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

From
Tom Lane
Date:
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



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

From
Andres Freund
Date:
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



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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Asif Naeem
Date:
Hi Haribabu,

I am not able to apply latest patch on REL9_4_STABLE or master branch i.e.

pc1dotnetpk:postgresql asif$ git apply ~/core/min_max_support_for_inet_datatypes/inet_agg_v4.patch
fatal: unrecognized input

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
--------------------------
File to patch: 

Is there any other utility required to apply the patch, Can you please guide ?. Thanks.

Regards,
Muhammad Asif Naeem


On Thu, Jun 5, 2014 at 6:28 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
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


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


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

From
Abhijit Menon-Sen
Date:
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



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

From
Asif Naeem
Date:
On Mon, Jun 30, 2014 at 4:45 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
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.

Thank you Abhijit, It worked.
 

-- Abhijit

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

From
Asif Naeem
Date:
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.

Regards,
Muhammad Asif Naeem


On Mon, Jul 7, 2014 at 1:56 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
On Mon, Jun 30, 2014 at 4:45 PM, Abhijit Menon-Sen <ams@2ndquadrant.com> wrote:
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.

Thank you Abhijit, It worked.
 

-- Abhijit


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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Amit Kapila
Date:
Hi Asif,

On Wed, Jul 9, 2014 at 6:51 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> 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.

Hari has provided an updated patch as per your comments, if
you think patch is fine, could you please move it to Ready For Committer?

Incase your review is still pending, then it is okay.  I have asked
as from your mail it seems to me that the new patch addresses all
your concerns.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

From
Asif Naeem
Date:
Hi Haribabu,

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 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

Other than this patch looks good to me. Thanks.

Regards,
Muhammad Asif Naeem

On Wed, Jul 9, 2014 at 6:21 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
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

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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Asif Naeem
Date:
Thank you Haribabu. Please see my comments inlined below i.e.

On Sun, Jul 27, 2014 at 11:42 AM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
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.

Sorry for not being clear, above mentioned test is related to following doc (sgml) changes that seems not working as described i.e.

Table 9-35. cidr and inet Functions

FunctionReturn TypeDescriptionExampleResult





max(inetinet)inetlarger of two inet typesmax('192.168.1.5', '192.168.1.4')192.168.1.5
min(inetinet)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 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

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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Asif Naeem
Date:
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.

Regards,
Muhammad Asif Naeem



On Tue, Aug 12, 2014 at 3:12 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
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 described i.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

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

From
Haribabu Kommi
Date:
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

Attachment

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

From
Tom Lane
Date:
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



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

From
Haribabu Kommi
Date:
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