Re: [HACKERS] [PATCH] Improve geometric types

2017-11-09 Thread Emre Hasegeli
>> This is also effecting lseg ## box operator.
>
> Mmm.. It returns (1.5, 1.5) with the 0004 patch. It is surely a
> point on the second operand but I'm not sure it's right that the
> operator returns a specific point for two parallel segments.

I am changing it to return NULL, when they are parallel.

> I'd like to put comments on 0001 and 0004 only now:
>
>  - Adding [LR]DELIM_L looks good but they should be described in
>the comment just above.

I will mention it on the new version.

>  - I'm not sure the change of box_construct is good but currently
>all callers fits the new interface (giving two points, not
>four coordinates).

I tried to make things more consistent.  The other constructors takes points.

>  - close_lseg seems forgetting the case where the two given
>segments are crossing (and parallel).

I am re-implementing it covering those cases.

> - make_bound_box operates directly on the poly->boundbox. I'm
>   afraid that such coding hinders compiers from using registers.

I am changing it back.

>   This is a bit apart from this patch, it would be better if we
>   could eliminate internal calls using DirectFunctionCall.

We don't seem to be able to fix all issues without doing that.  I will
incorporate the change.

Thank you for your review.  I will address your other email before
posting new versions.


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-11-09 Thread Kyotaro HORIGUCHI
Hello,

> I'd like to put comments on 0001 and 0004 only now:
...

I don't have a comment on 0002.

About 0003:

>  @@ -4487,21 +4486,21 @@ circle_in(PG_FUNCTION_ARGS)
> ...
>   circle->radius = single_decode(s, , "circle", str);
> - if (circle->radius < 0)
> + if (float8_lt(circle->radius, 0.0))
>   ereport(ERROR,
>   (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),

flost8_lt and its family functions are provided to unify the
sorting order including NaN. NaN is not rejected by the usage of
float8_lt in the case but it is what the function is expected to
be used for. If we wanted to check if it is positive, it
unexpectedly throws an exception.  (I suppose that NaNs should be
silently ignored rather than stopping a query by throwng an
exception.)

Addition to that I don't think it proper that applying EPSILON(!)
there. It should be strictly compared regardless whether EPSION
is considered or not.


Similary, circle_overlap for example, float8_le is used to
compare the distance and the summed radius.

NaN causes a problem in another place.
  
> PG_RETURN_BOOL(FPle(point_dt(>center, >center),
> float8_pl(circle1->radius, circle2->radius)));

If the distance was NaN and the summed radius is not, the
function returns true. I think that a reasonable behavior is that
an object containing NaN cannot make any meaningful relationship
with other objects as floating number itself behave so. (Any
comparison other than != with NaN returns always false)

Just using another series of comparison functions that return
false for NaN-containing comparison is not a solution since the
meaning of the returned false differs by context, just same as
the first problem above. For exameple, the fictious functions
below,

| bool circle_overlaps()
|   ret = FPle(distance, radius_sum);

This gives correct results, but

| bool circle_not_overlaps()
|   ret = FPgt(distance, radius_sum);

This gives a wrong result for NaN-containing objects.

Perhaps it is enough to explicitly define behaviors for NaN
before comparison.

circle_overlap()
> distance = point_dt();
> radius_sum = float8_pl(...);
>
> /* NaN-containing objects doesn't overlap any other objects */ 
> if (isnan(distance) || isnan(radius_sum))
>  PG_RETURN_BOOL(false);
>
> /* NaN ordering of FPle() doesn't get into mischief here */
> return PG_RETURN_BOOL(FPle(distance, radius_sum));

(End Of the Comment to 0003)

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
 



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-11-07 Thread Kyotaro HORIGUCHI
Hello, thanks for the new patch.

0004 failed to be applied on the underneath patches.

At Sun, 5 Nov 2017 15:54:19 +0100, Emre Hasegeli  wrote in 

> > I am not sure how useful NaNs are in geometric types context, but we
> > allow them, so inconsistent hypot() would be a problem.  I will change
> > my patches to keep pg_hypot().
> 
> New versions of the patches are attached with 2 additional ones.  The
> new versions leave pg_hypot() in place.  One of the new patches
> improves the test coverage.  The line coverage of geo_ops.c increases
> from 55% to 81%.  The other one fixes -0 values to 0 on float
> operators.  I am not sure about performance implication of this, so
> kept it separate.  It may be a better idea to check this only on the
> platforms that has tendency to produce -0.
> 
> While working on the tests, I found some unreachable code and removed
> it.  I also found that lseg ## lseg operator returning wrong results.
> It is defined as "closest point to first segment on the second
> segment", but:
> 
> > # select '[(1,2),(3,4)]'::lseg ## '[(0,0),(6,6)]'::lseg;
> >  ?column?
> > --
> >  (1,2)
> > (1 row)
> 
> I appended the fix to the patches.  This is also effecting lseg ## box 
> operator.

Mmm.. It returns (1.5, 1.5) with the 0004 patch. It is surely a
point on the second operand but I'm not sure it's right that the
operator returns a specific point for two parallel segments.

> I also changed recently band-aided point ## lseg operator to return
> the point instead of NULL when it cannot find the correct result to
> avoid the operators depending on this one to crash.

I'd like to put comments on 0001 and 0004 only now:

 - Adding [LR]DELIM_L looks good but they should be described in
   the comment just above.

 - Renaming float8_slope to slope seems no problem.

 - I'm not sure the change of box_construct is good but currently
   all callers fits the new interface (giving two points, not
   four coordinates).

 - close_lseg seems forgetting the case where the two given
   segments are crossing (and parallel). For example,

   select '(-8,-8),(1,1)'::lseg ## '(-10,0),(2,0)'::lseg;

   is expected to return (0,0), which is the crossing point of
   the two segments, but it returns (1,0) (and returned (1,1)
   before the patch), which is the point on the l2 closest to the
   closer end of l1 to l2.

   As mentioned above, it is difficult to dicide what is the
   proper result for parallel segments. I suppose that the
   following operation should return an invalid value as a point.

   select '(-1,0),(1,0)'::lseg ## '(-1,1),(1,1)'::lseg;

   close_lseg does the following operations in the else case of
   'if (float8_...)'. If i'm not missing something, the result of
   the following operation is always the closer end of l2. In
   other words it seems a waste of cycles.
   
   | point = DirectFunctionCall2(close_ps,
   | PointPGetDatum(>p[closer_end2]),
   | LsegPGetDatum(l1));
   | return DirectFunctionCall2(close_ps, point, LsegPGetDatum(l2));


- make_bound_box operates directly on the poly->boundbox. I'm
  afraid that such coding hinders compiers from using registers.

  This is a bit apart from this patch, it would be better if we
  could eliminate internal calls using DirectFunctionCall.

reagrds,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-04 Thread Emre Hasegeli
> Now, it's also not clear that anything in PG really cares.  But if we
> do care, I think we should keep pg_hypot() ... and maybe clarify the
> comment a bit more.

I am not sure how useful NaNs are in geometric types context, but we
allow them, so inconsistent hypot() would be a problem.  I will change
my patches to keep pg_hypot().


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-03 Thread Tom Lane
Emre Hasegeli  writes:
>> Uh, I thought pg_hypot() was still needed on our oldest supported
>> platforms.  Or is hypot() already supported there?

> What is our oldest supported platform?

Our normal reference for such questions is SUS v2 (POSIX 1997):
http://pubs.opengroup.org/onlinepubs/007908799/

I looked into that, and noted that it does specify hypot(), but
it has different rules for edge conditions:

If the result would cause overflow, HUGE_VAL is returned and errno
may be set to [ERANGE].

If x or y is NaN, NaN is returned. and errno may be set to [EDOM].

If the correct result would cause underflow, 0 is returned and
errno may be set to [ERANGE].

whereas POSIX 2008 saith:

If the correct value would cause overflow, a range error shall
occur and hypot(), hypotf(), and hypotl() shall return the value
of the macro HUGE_VAL, HUGE_VALF, and HUGE_VALL, respectively.

[MX] If x or y is ±Inf, +Inf shall be returned (even if one of x
or y is NaN).

If x or y is NaN, and the other is not ±Inf, a NaN shall be
returned.

[MXX] If both arguments are subnormal and the correct result is
subnormal, a range error may occur and the correct result shall
be returned.

In short, the reason that the existing comment makes a point of the
Inf-and-NaN behavior is that the standard changed somewhere along the
line.  While I believe we could get away with assuming that hypot()
exists everywhere, it's much less clear that we could rely on the result
being Inf and not NaN in this case.

Now, it's also not clear that anything in PG really cares.  But if we
do care, I think we should keep pg_hypot() ... and maybe clarify the
comment a bit more.

regards, tom lane


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-03 Thread Emre Hasegeli
> Uh, I thought pg_hypot() was still needed on our oldest supported
> platforms.  Or is hypot() already supported there?  If not, I suppose we
> can keep the HYPOT() macro, and have it use hypot() on platforms that
> have it and pg_hypot elsewhere?  That particular fraction of 0001 seemed
> a bit dubious to me, as the largest possible source of platform
> dependency issues.

What is our oldest supported platform?

We can also just keep pg_hypot().  I don't think getting rid of it
justifies much work.


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-03 Thread Emre Hasegeli
> I think if we're going to do this it should be separated out as its
> own patch.  Also, I think someone should explain what the reasoning
> behind the change is.  Do we, for example, foresee that the built-in
> code might be faster or less likely to overflow?  Because we're
> clearly taking a risk -- most trivially, that the BF will break, or
> more seriously, that some machines will have versions of this function
> that don't actually behave quite the same.

I included removal of pg_hypot() on my patch simply because the
comment on the function header is suggesting it.  I though if we are
going to clean this module up, we better deal it first.  I understand
the risk.  The patches include more changes.  It may be a good idea to
have those together.

> That brings up a related point.  How good is our test case coverage
> for hypot(), especially in strange corner cases, like this one
> mentioned in pg_hypot()'s comment:
>
>  * This implementation conforms to IEEE Std 1003.1 and GLIBC, in that the
>  * case of hypot(inf,nan) results in INF, and not NAN.

I don't see any tests of geometric types with INF or NaN.  Currently,
there isn't consistent behaviour for them.  I don't think we can
easily add portable ones on the current state, but we should be able
to do so with my patches.  I will look into it.

> I'm potentially willing to commit a patch that just makes the
> pg_hypot() -> hypot() change and does nothing else, if there are not
> objections to that change, but I want to be sure that we'll know right
> away if that turns out to break.

I can split this one into another patch.


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-03 Thread Alvaro Herrera
Robert Haas wrote:

> I'm potentially willing to commit a patch that just makes the
> pg_hypot() -> hypot() change and does nothing else, if there are not
> objections to that change, but I want to be sure that we'll know right
> away if that turns out to break.

Uh, I thought pg_hypot() was still needed on our oldest supported
platforms.  Or is hypot() already supported there?  If not, I suppose we
can keep the HYPOT() macro, and have it use hypot() on platforms that
have it and pg_hypot elsewhere?  That particular fraction of 0001 seemed
a bit dubious to me, as the largest possible source of platform
dependency issues.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-03 Thread Kyotaro HORIGUCHI
At Mon, 2 Oct 2017 08:23:49 -0400, Robert Haas  wrote in 

> On Mon, Oct 2, 2017 at 4:23 AM, Kyotaro HORIGUCHI
>  wrote:
> > For other potential reviewers:
> >
> > I found the origin of the function here.
> >
> > https://www.postgresql.org/message-id/4a90bd76.7070...@netspace.net.au
> > https://www.postgresql.org/message-id/AANLkTim4cHELcGPf5w7Zd43_dQi_2RJ_b5_F_idSSbZI%40mail.gmail.com
> >
> > And the reason for pg_hypot is seen here.
> >
> > https://www.postgresql.org/message-id/407d949e0908222139t35ad3ad2q3e6b15646a27d...@mail.gmail.com
> >
> > I think the replacement is now acceptable according to the discussion.
> > ==
> 
> I think if we're going to do this it should be separated out as its
> own patch.

+1

> Also, I think someone should explain what the reasoning
> behind the change is.  Do we, for example, foresee that the built-in
> code might be faster or less likely to overflow?  Because we're
> clearly taking a risk -- most trivially, that the BF will break, or
> more seriously, that some machines will have versions of this function
> that don't actually behave quite the same.
> 
> That brings up a related point.  How good is our test case coverage
> for hypot(), especially in strange corner cases, like this one
> mentioned in pg_hypot()'s comment:
> 
>  * This implementation conforms to IEEE Std 1003.1 and GLIBC, in that the
>  * case of hypot(inf,nan) results in INF, and not NAN.

I'm not sure how precise we practically need them to be
identical.  FWIW as a rough confirmation on my box, I compared
hypot and pg_hypot for the following arbitrary choosed pairs of
parameters.


 {2.2e-308, 2.2e-308},
 {2.2e-308, 1.7e307},
 {1.7e307, 1.7e307},
 {1.7e308, 1.7e308},
 {2.2e-308, DBL_MAX},
 {1.7e308, DBL_MAX},
 {DBL_MIN, DBL_MAX},
 {DBL_MAX, DBL_MAX},
 {1.7e307, INFINITY},
 {2.2e-308, INFINITY},
 {0, INFINITY},
 {DBL_MIN, INFINITY},
 {INFINITY, INFINITY},
 {1, NAN},
 {INFINITY, NAN},
 {NAN, NAN},


Only the first pair gave slightly not-exactly-equal results but
it seems to do no harm. hypot set underflow flag.

 0: hypot=3.111269837220809e-308 (== 0.0 is 0, < DBL_MIN is 0)
   pg_hypot=3.11126983722081e-308 (== 0.0 is 0, < DBL_MIN is 0)
   equal=0,
   hypot(errno:0, inval:0, div0:0, of=0, uf=1),
   pg_hypot(errno:0, inval:0, div0:0, of=0, uf=0)

But not sure how the both functions behave on other platforms.

> I'm potentially willing to commit a patch that just makes the
> pg_hypot() -> hypot() change and does nothing else, if there are not
> objections to that change, but I want to be sure that we'll know right
> away if that turns out to break.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
#include 
#include 
#include 
#include 
#include 

double
pg_hypot(double x, double y)
{
double  yx;

/* Handle INF and NaN properly */
if (isinf(x) || isinf(y))
return (double) INFINITY;

if (isnan(x) || isnan(y))
return (double) NAN;

/* Else, drop any minus signs */
x = fabs(x);
y = fabs(y);

/* Swap x and y if needed to make x the larger one */
if (x < y)
{
double  temp = x;

x = y;
y = temp;
}

/*
 * If y is zero, the hypotenuse is x.  This test saves a few cycles in
 * such cases, but more importantly it also protects against
 * divide-by-zero errors, since now x >= y.
 */
if (y == 0.0)
return x;

/* Determine the hypotenuse */
yx = y / x;
return x * sqrt(1.0 + (yx * yx));
}

void
setfeflags(int *invalid, int *divzero, int *overflow, int *underflow)
{
int err = fetestexcept(FE_INVALID | FE_DIVBYZERO |
   FE_OVERFLOW | FE_UNDERFLOW);
*invalid = ((err & FE_INVALID) != 0);
*divzero = ((err & FE_DIVBYZERO) != 0);
*overflow = ((err & FE_OVERFLOW) != 0);
*underflow = ((err & FE_UNDERFLOW) != 0);
}

int
main(void)
{
double x;
double y;
double p[][2] =
{
{2.2e-308, 2.2e-308},
{2.2e-308, 1.7e307},
{1.7e307, 1.7e307},
{1.7e308, 1.7e308},
{2.2e-308, DBL_MAX},
{1.7e308, DBL_MAX},
{DBL_MIN, DBL_MAX},
{DBL_MAX, DBL_MAX},
{1.7e307, INFINITY},
{2.2e-308, INFINITY},
{0, INFINITY},
{DBL_MIN, INFINITY},
{INFINITY, INFINITY},
{1, NAN},
{INFINITY, NAN},
{NAN, NAN},
{0.0, 0.0}

Re: [HACKERS] [PATCH] Improve geometric types

2017-10-03 Thread Kyotaro HORIGUCHI
Thanks.

At Mon, 2 Oct 2017 11:46:15 +0200, Emre Hasegeli  wrote in 

Re: [HACKERS] [PATCH] Improve geometric types

2017-10-02 Thread Robert Haas
On Mon, Oct 2, 2017 at 4:23 AM, Kyotaro HORIGUCHI
 wrote:
> For other potential reviewers:
>
> I found the origin of the function here.
>
> https://www.postgresql.org/message-id/4a90bd76.7070...@netspace.net.au
> https://www.postgresql.org/message-id/AANLkTim4cHELcGPf5w7Zd43_dQi_2RJ_b5_F_idSSbZI%40mail.gmail.com
>
> And the reason for pg_hypot is seen here.
>
> https://www.postgresql.org/message-id/407d949e0908222139t35ad3ad2q3e6b15646a27d...@mail.gmail.com
>
> I think the replacement is now acceptable according to the discussion.
> ==

I think if we're going to do this it should be separated out as its
own patch.  Also, I think someone should explain what the reasoning
behind the change is.  Do we, for example, foresee that the built-in
code might be faster or less likely to overflow?  Because we're
clearly taking a risk -- most trivially, that the BF will break, or
more seriously, that some machines will have versions of this function
that don't actually behave quite the same.

That brings up a related point.  How good is our test case coverage
for hypot(), especially in strange corner cases, like this one
mentioned in pg_hypot()'s comment:

 * This implementation conforms to IEEE Std 1003.1 and GLIBC, in that the
 * case of hypot(inf,nan) results in INF, and not NAN.

I'm potentially willing to commit a patch that just makes the
pg_hypot() -> hypot() change and does nothing else, if there are not
objections to that change, but I want to be sure that we'll know right
away if that turns out to break.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-10-02 Thread Kyotaro HORIGUCHI
Hello. Thank you for the new version.

0001: applies cleanly. regress passed.
  this mainly refactoring geo_ops.c and replacing pg_hypot with hypot(3).
0002: applies cleanly. regress passed.
  this just replaces float-ops macros into inline functions.
0003: applies cleanly. regress passed.
  replaces double with float8 and bare arithmetic with defined functions.
0004: applies cleanly. regress passsed.
  fix broken line-related functions.
  I have some comments on this (later).

At Wed, 27 Sep 2017 17:44:52 +0200, Emre Hasegeli  wrote in 

Re: [HACKERS] [PATCH] Improve geometric types

2017-09-27 Thread Emre Hasegeli
> The patch replace pg_hypot with hypot in libc. The man page says
> as follows.
>
> man 3 hypot
>>   If the result overflows, a range error occurs, and the functions return
>>   HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively.
> ..
>>ERRORS
>>   See math_error(7) for information on how to determine whether an  error
>>   has occurred when calling these functions.
>>
>>   The following errors can occur:
>>
>>   Range error: result overflow
>>  errno  is  set  to ERANGE.  An overflow floating-point exception
>>  (FE_OVERFLOW) is raised.
>>
>>   Range error: result underflow
>>  An underflow floating-point exception (FE_UNDERFLOW) is raised.
>>
>>  These functions do not set errno for this case.
>
> So, the code seems to need some amendments following to this
> spec.

I included them on the latest version.


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-19 Thread Kyotaro HORIGUCHI
At Fri, 15 Sep 2017 11:25:30 -0400, Robert Haas  wrote 
in 
> On Fri, Sep 15, 2017 at 4:23 AM, Kyotaro HORIGUCHI
>  wrote:
> > /* don't merge the following same functions with different types
> >into single macros so that double evaluation won't happen */
> >
> > Is it still too verbose?
> 
> Personally, I don't think such a comment has much value, but I am not
> going to spend a lot of time arguing about it.  It's really up to the
> eventual committer to decide, and that probably won't be me in this
> case.  My knowledge of the geometric types isn't great, and I am a tad
> busy breaking^Wimproving things I do understand.

Ok, I agree with you.

# Though it is not a issue of geometric types :p

I'll withdrow the comment. Maybe someone notices of that when
reviewing such a patch.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-15 Thread Robert Haas
On Fri, Sep 15, 2017 at 4:23 AM, Kyotaro HORIGUCHI
 wrote:
> /* don't merge the following same functions with different types
>into single macros so that double evaluation won't happen */
>
> Is it still too verbose?

Personally, I don't think such a comment has much value, but I am not
going to spend a lot of time arguing about it.  It's really up to the
eventual committer to decide, and that probably won't be me in this
case.  My knowledge of the geometric types isn't great, and I am a tad
busy breaking^Wimproving things I do understand.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-15 Thread Kyotaro HORIGUCHI
Hello, just one point on 0001.

The patch replace pg_hypot with hypot in libc. The man page says
as follows.

man 3 hypot
>   If the result overflows, a range error occurs, and the functions return
>   HUGE_VAL, HUGE_VALF, or HUGE_VALL, respectively.
..
>ERRORS
>   See math_error(7) for information on how to determine whether an  error
>   has occurred when calling these functions.
>
>   The following errors can occur:
>
>   Range error: result overflow
>  errno  is  set  to ERANGE.  An overflow floating-point exception
>  (FE_OVERFLOW) is raised.
>
>   Range error: result underflow
>  An underflow floating-point exception (FE_UNDERFLOW) is raised.
>
>  These functions do not set errno for this case.

So, the code seems to need some amendments following to this
spec.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-15 Thread Kyotaro HORIGUCHI
At Fri, 15 Sep 2017 17:23:28 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20170915.172328.97446299.horiguchi.kyot...@lab.ntt.co.jp>
> At Thu, 14 Sep 2017 16:19:13 -0400, Robert Haas  wrote 
> in 
> > On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
> >  wrote:
> > > I recall a bit about the double-evaluation hazards. I think the
> > > functions needs a comment describing the reasons so that anyone
> > > kind won't try to merge them into a macro again.
> > 
> > I think we can count on PostgreSQL developers to understand the
> > advantages of an inline function over a macro.  Even if they don't,
> > the solution can't be to put a comment in every place where an inline
> > function is used explaining it.  That would be very repetitive.
> 
> Of course putting such a comment to all inline functions is
> silly. The point here is that many pairs of two functions with
> exactly the same shape but handle different types are defined
> side by side. Such situation seems tempting to merge them into
> single macros, as the previous author did there.
> 
> So a simple one like the following would be enough.
> 
> /* don't merge the following same functions with different types
>into single macros so that double evaluation won't happen */
> 
> Is it still too verbose?

That being said, I'm not stick on that if Robert or others think
it as needless.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-15 Thread Kyotaro HORIGUCHI
At Thu, 14 Sep 2017 16:19:13 -0400, Robert Haas  wrote 
in 
> On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
>  wrote:
> > I recall a bit about the double-evaluation hazards. I think the
> > functions needs a comment describing the reasons so that anyone
> > kind won't try to merge them into a macro again.
> 
> I think we can count on PostgreSQL developers to understand the
> advantages of an inline function over a macro.  Even if they don't,
> the solution can't be to put a comment in every place where an inline
> function is used explaining it.  That would be very repetitive.

Of course putting such a comment to all inline functions is
silly. The point here is that many pairs of two functions with
exactly the same shape but handle different types are defined
side by side. Such situation seems tempting to merge them into
single macros, as the previous author did there.

So a simple one like the following would be enough.

/* don't merge the following same functions with different types
   into single macros so that double evaluation won't happen */

Is it still too verbose?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-14 Thread Robert Haas
On Thu, Sep 14, 2017 at 3:33 AM, Kyotaro HORIGUCHI
 wrote:
> I recall a bit about the double-evaluation hazards. I think the
> functions needs a comment describing the reasons so that anyone
> kind won't try to merge them into a macro again.

I think we can count on PostgreSQL developers to understand the
advantages of an inline function over a macro.  Even if they don't,
the solution can't be to put a comment in every place where an inline
function is used explaining it.  That would be very repetitive.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-14 Thread Kyotaro HORIGUCHI
At Tue, 12 Sep 2017 19:30:44 +0200, Emre Hasegeli  wrote in 

Re: [HACKERS] [PATCH] Improve geometric types

2017-09-12 Thread Emre Hasegeli
> Hello, sorry to late for the party, but may I comment on this?

Thank you for picking this up again.

> The first patch reconstructs the operators in layers. These
> functions are called very frequently when used. Some function are
> already inlined in float.h but some static functions in float.h
> also can be and are better be inlined. Some of *_internal,
> point_construct, line_calculate_point and so on are the
> candidates.

They are static functions.  I though compiler can decide to inline
them.  Do you think adding "inline" to the function signatures are
necessary?

> You removed some DirectFunctionCall to the functions within the
> same file but other functions remain in the style,
> ex. poly_center or on_sl. The function called from the former
> seems large enough but the latter function calls a so small
> function that it could be inlined. Would you like to make some
> additional functions use C call (instead of DirectFunctionCall)
> and inlining them?

I tried to minimise my changes to make reviewing easier.  I can make
"_internal" functions for the remaining DirectFunctionCall()s, if you
find it necessary.

> This is not a fault of this patch, but some functions like on_pb
> seems missing comment to describe what it is. Would you like to
> add some?

I will add some on the next version.

> In the second patch, the additional include fmgrprotos.h in
> btree_gin.c seems needless.

It must be something I missed on rebase.  I will remove it.

> Some float[48] features were macros
> so that they share the same expressions between float4 and
> float8. They still seems sharing perfectly the same expressions
> in float.h. Is there any reason for converting them into typed
> inline functions?

Kevin Grittner suggested using inline functions instead of macros.
They are easier to use compared to macros, and avoid double-evaluation
hazards.

> In float.h, MAXDOUBLEWIDTH is redueced from 500 to 128, but the
> exponent of double is up to 308 so it doesn't seem sufficient. On
> the other hand we won't use non-scientific notation for extremely
> large numbers and it requires (perhaps) up to 26 bytes in the
> case. In the soruce code, most of them uses "%e" and one of them
> uses '%g". %e always takes the format of
> "-1.(17digits)e+308".. So it would be less than 26
> characters.
>
> =# set extra_float_digits to 3;
> =# select -1.221423424320453e308::float8;
>  ?column?
> ---
>  -1.22142342432045302e+308
>
> man printf: (linux)
>> Style e is used if the exponent from its conversion is less than
>> -4 or greater than or equal to the precision.
>
> So we should be safe to have a buffer with 26 byte length and 500
> bytes will apparently too large and even 128 will be too loose in
> most cases. So how about something like the following?
>
> #define MINDOUBLEWIDTH 32

Should it be same for float4 and float8?

> ...
> float4out@float.c:
>>int  ndig = FLT_DIG + extra_float_digits;
>>
>>if (ndig < 1)
>>   ndig = 1;
>>
>>len = snprintf(ascii, MINDOUBLEWIDTH + 1, "%+.*g", ndig, num);
>> if (len > MINDOUBLEWIDTH + 1)
>>{
>>ascii = (char *) repalloc(ascii, len);
>>if (snprintf(ascii, len, "%+.*e", ndig, num) > len)
>>   error(ERROR, "something wrong happens...");
>> }
>
> I don't think the if part can be used so there would be no
> performance degradation, I believe.

Wouldn't this change the output of the float datatypes?  That would be
a backwards incompatible change.

> I'd like to pause here.

I will submit new versions after your are done with your review.


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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-12 Thread Kyotaro HORIGUCHI
Hello, sorry to late for the party, but may I comment on this?

At Tue, 05 Sep 2017 13:18:12 +, Aleksander Alekseev 
 wrote in 
<20170905131812.18925.13551.p...@coridan.postgresql.org>
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
> 
> LGTM.
> 
> The new status of this patch is: Ready for Committer

The first patch reconstructs the operators in layers. These
functions are called very frequently when used. Some function are
already inlined in float.h but some static functions in float.h
also can be and are better be inlined. Some of *_internal,
point_construct, line_calculate_point and so on are the
candidates.

You removed some DirectFunctionCall to the functions within the
same file but other functions remain in the style,
ex. poly_center or on_sl. The function called from the former
seems large enough but the latter function calls a so small
function that it could be inlined. Would you like to make some
additional functions use C call (instead of DirectFunctionCall)
and inlining them?

This is not a fault of this patch, but some functions like on_pb
seems missing comment to describe what it is. Would you like to
add some?


In the second patch, the additional include fmgrprotos.h in
btree_gin.c seems needless. Some float[48] features were macros
so that they share the same expressions between float4 and
float8. They still seems sharing perfectly the same expressions
in float.h. Is there any reason for converting them into typed
inline functions?

In float.h, MAXDOUBLEWIDTH is redueced from 500 to 128, but the
exponent of double is up to 308 so it doesn't seem sufficient. On
the other hand we won't use non-scientific notation for extremely
large numbers and it requires (perhaps) up to 26 bytes in the
case. In the soruce code, most of them uses "%e" and one of them
uses '%g". %e always takes the format of
"-1.(17digits)e+308".. So it would be less than 26
characters. 

=# set extra_float_digits to 3;
=# select -1.221423424320453e308::float8;
 ?column?  
---
 -1.22142342432045302e+308

man printf: (linux)
> Style e is used if the exponent from its conversion is less than
> -4 or greater than or equal to the precision.

So we should be safe to have a buffer with 26 byte length and 500
bytes will apparently too large and even 128 will be too loose in
most cases. So how about something like the following?

#define MINDOUBLEWIDTH 32
...
float4out@float.c:
>int  ndig = FLT_DIG + extra_float_digits;
> 
>if (ndig < 1)
>   ndig = 1;
> 
>len = snprintf(ascii, MINDOUBLEWIDTH + 1, "%+.*g", ndig, num);
> if (len > MINDOUBLEWIDTH + 1)
>{
>ascii = (char *) repalloc(ascii, len);
>if (snprintf(ascii, len, "%+.*e", ndig, num) > len)
>   error(ERROR, "something wrong happens...");
> }

I don't think the if part can be used so there would be no
performance degradation, I believe.



I'd like to pause here.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-05 Thread Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

LGTM.

The new status of this patch is: Ready for Committer

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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-04 Thread Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

PostgreSQL fails with SIGSEGV during `make check-world`.

Backtrace: http://afiskon.ru/s/d4/f3dc17838a_sigsegv.txt
regression.diffs (not very useful): 
http://afiskon.ru/s/ac/ac5294656c_regression.diffs.txt
regression.out: http://afiskon.ru/s/70/39d872e2b8_regression.out.txt
How to reproduce: https://github.com/afiskon/pgscripts/blob/master/full-build.sh

The environment is Arch Linux x64, gcc 7.1.1

The new status of this patch is: Waiting on Author

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


Re: [HACKERS] [PATCH] Improve geometric types

2017-09-01 Thread Aleksander Alekseev
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi Emre,

I'm afraid these patches conflict with current master branch.

The new status of this patch is: Waiting on Author

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