Re: [HACKERS] [PATCH] Improve geometric types
>> 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
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, &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(&circle1->center, &circle2->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
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(&l2->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
> 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
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
> 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
> 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
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
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}
};
int i;
for (i = 0 ; p[i][0] != 0.0 || p[i][1] != 0.0 ; i++)
{
int errno1, errno
Re: [HACKERS] [PATCH] Improve geometric types
Thanks. At Mon, 2 Oct 2017 11:46:15 +0200, Emre Hasegeli wrote in > > And this should be the last comment of mine on the patch set. > > Thank you. The new versions of the patches are attached addressing > your comments. > > > By the way, I found that MAXDOUBLEWIDTH has two inconsistent > > definitions in formatting.c(500) and float.c(128). The definition > > in new float.h is according to float.c and it seems better to be > > untouched and it would be another issue. > > The last version of the patch don't move these declarations to the header > file. Yeah, it is not about the patch itself. > > # The commit message of 0001 says that "using C11 hypot()" but it > > # is sine C99. I suppose we shold follow C99 at the time so I > > # suggest rewrite it in the next version if any. > > Changed. > > > close_pl got a bug fix not only refactoring. I think it is > > recommended to separate bugs and improvements, but I'm fine with > > the current patch. > > I split the refactoring to the first patch. > > > You added sanity check "A==0 && B==0" (in Ax + By + C) in > > line_recv. I'm not sure the necessity of the check since it has > > been checked in line_in but anyway the comparisons seem to be > > typo(or thinko) of FPzero. > > Tom Lane suggested [1] this one. I now made it use FPzero(). I see. It's fine with me. I suppose that Tom didn't intened the suggestion to be teken literary so using FPzero() seems better (at least for now). > > dist_pl is changed to take the smaller distance of both ends of > > the segment. It seems absorbing error, so it might be better > > taking the mean of the two distances. If you have a firm reason > > for the change, it is better to be written there, or it might be > > better left alone. > > I don't really, so I left that part out. Mmm, sorry. It's my mistake. > [1] https://www.postgresql.org/message-id/11053.1466362319%40sss.pgh.pa.us I'll look the new version further. 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
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
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 > The new versions of the patches are attached addressing your comments. > > > C++ surely make just static functions inlined but I'm not sure C > > compiler does that. > > Thank you for your explanation. I marked the mentioned functions "inline". Thanks. > > 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 > > I left this part out for now. We can improve it separately. Agreed. I found that psprintf does that with initial length of 128. By the way, I found that MAXDOUBLEWIDTH has two inconsistent definitions in formatting.c(500) and float.c(128). The definition in new float.h is according to float.c and it seems better to be untouched and it would be another issue. At Wed, 27 Sep 2017 17:45:04 +0200, Emre Hasegeli wrote in > > The patch replace pg_hypot with hypot in libc. The man page says > > as follows. ... > I included them on the latest version. # The commit message of 0001 says that "using C11 hypot()" but it # is sine C99. I suppose we shold follow C99 at the time so I # suggest rewrite it in the next version if any. It seems bettern than expected. I confirmed that pg_hypot(DBL_MAX, DBL_MAX) returned a value that is equivalent to HUGE_VAL * HUGE_VAL on glibc, but I'm not sure the behavior on other platforms is the same. == 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. == And this should be the last comment of mine on the patch set. In 0004, line_distance and line_interpt_internal seem correct. Seemingly horizontal/vertical checks are redundant but it is because unclearly defined EPSLON bahavior. lseg_perp seems correct. close_pl got a bug fix not only refactoring. I think it is recommended to separate bugs and improvements, but I'm fine with the current patch. You added sanity check "A==0 && B==0" (in Ax + By + C) in line_recv. I'm not sure the necessity of the check since it has been checked in line_in but anyway the comparisons seem to be typo(or thinko) of FPzero. dist_pl is changed to take the smaller distance of both ends of the segment. It seems absorbing error, so it might be better taking the mean of the two distances. If you have a firm reason for the change, it is better to be written there, or it might be better left alone. 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
> 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
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
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
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
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
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
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
At Tue, 12 Sep 2017 19:30:44 +0200, Emre Hasegeli wrote in
> > 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?
C++ surely make just static functions inlined but I'm not sure C
compiler does that. "static" is a scope modifier and "inline" is
not (what kind of modifier is it?). In regard to gcc,
https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Inline.html#Inline
> By declaring a function inline, you can direct GCC to make
> calls to that function faster. You can also direct GCC
> to try to integrate all “simple enough” functions into their
> callers with the option -finline-functions.
I didn't find that postgresql uses the options so I think we
shouldn't expect that they are inlined automatically.
> > 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.
Ok, it would be another patch even if we need that.
> > 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.
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.
> > 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?
Strictly they are different each other but float8 needs 26 bytes
(additional 1 byte is the sign for expnent, which I forgot.) and
float4 needs 15 bytes so it could be reduced to 32 and 16
respectively.
| select -1.11e-38::float4;
| -1.1113e-38
| select -1.11e-4::float4;
| -0.0002
| select -1.11e-5::float4;
| -1.1112e-05
On the other hand float4 is anyway converted into double in
float4out and I'm not sure that the extension doesn't adds
spurious digits. Therefore I think it would be reasonable that
"%g" formatter requires up to 27 bytes (26 + terminating zero) so
it should use MINDOUBLEWIDTH regardless of the precision of the
value given to the formatter.
Addition to that if we are deliberately using %g in float4out, we
can use flot8out_internal instead of most of the stuff of
float4out.
| Datum
| float4out(PG_FUNCTION_ARGS)
| {
| float8 num = PG_GETARG_FLOAT4(0);
|
| PG_RETURN_CSTRING(float8out_internal(num));
| }
> > ...
> > float4out@float.c:
> >>int nd
Re: [HACKERS] [PATCH] Improve geometric types
> 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
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
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
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
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
