Re: [BUGS] 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(at)dons(dot)net(dot)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(at)dons(dot)net(dot)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(dot)haribabu(at)gmail(dot)com>wrote:

> On Fri, Mar 21, 2014 at 5:17 PM, <darius(at)dons(dot)net(dot)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(at)postgresql(dot)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(at)omniti(dot)com> wrote:
> On Sun, Mar 23, 2014 at 10:42 PM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
> wrote:
>>
>> On Fri, Mar 21, 2014 at 5:17 PM, <darius(at)dons(dot)net(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

On Tue, Jun 3, 2014 at 10:48 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:

> On 2014-06-03 10:37:53 -0400, Tom Lane wrote:
> > Andres Freund <andres(at)2ndquadrant(dot)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
>

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

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

On Wed, Jun 4, 2014 at 5:46 AM, Keith Fiske <keith(at)omniti(dot)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 Tue, Jun 3, 2014 at 8:37 PM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:

> On Wed, Jun 4, 2014 at 5:46 AM, Keith Fiske <keith(at)omniti(dot)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

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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

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(dot)haribabu(at)gmail(dot)com>
wrote:

> On Thu, Jun 5, 2014 at 9:12 AM, Andres Freund <andres(at)2ndquadrant(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-bugs
>
>

At 2014-06-30 16:35:45 +0500, anaeem(dot)it(at)gmail(dot)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, Jun 30, 2014 at 4:45 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:

> At 2014-06-30 16:35:45 +0500, anaeem(dot)it(at)gmail(dot)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
>

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(dot)it(at)gmail(dot)com> wrote:

> On Mon, Jun 30, 2014 at 4:45 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
> wrote:
>
>> At 2014-06-30 16:35:45 +0500, anaeem(dot)it(at)gmail(dot)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
>>
>
>

On Mon, Jul 7, 2014 at 6:59 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)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

Hi Asif,

On Wed, Jul 9, 2014 at 6:51 PM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:
> On Mon, Jul 7, 2014 at 6:59 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)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

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(dot)haribabu(at)gmail(dot)com>
wrote:

> On Mon, Jul 7, 2014 at 6:59 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)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(dot)it(at)gmail(dot)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

Thank you Haribabu. Please see my comments inlined below i.e.

On Sun, Jul 27, 2014 at 11:42 AM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:

> On Thu, Jul 24, 2014 at 5:59 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)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(inet, inet)inetlarger of two inet typesmax('192.168.1.5', '192.168.1.4')
192.168.1.5min(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 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(dot)it(at)gmail(dot)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

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(at)postgresql(dot)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(dot)haribabu(at)gmail(dot)com>
wrote:

> On Mon, Aug 4, 2014 at 3:22 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)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
>

On Mon, Aug 18, 2014 at 8:12 PM, Asif Naeem <anaeem(dot)it(at)gmail(dot)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(at)postgresql(dot)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(dot)haribabu(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)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