pgsql: Refactor geometric functions and operators

Refactor geometric functions and operators

The primary goal of this patch is to eliminate duplicate code and share
code between different geometric data types more often, to prepare the
ground for additional patches. Until now the code reuse was limited,
probably because the simpler types (line and point) were implemented
after the more complex ones.

The changes are quite extensive and can be summarised as:

* Eliminate SQL-level function calls.
* Re-use more functions to implement others.
* Unify internal function names and signatures.
* Remove private functions from geo_decls.h.
* Replace should-not-happen checks with assertions.
* Add comments describe for various functions.
* Remove some unreachable code.
* Define delimiter symbols of line datatype like the other ones.
* Remove the GEODEBUG macro and printf() calls.
* Unify code style of a few oddly formatted lines.

While the goal was to cause minimal user-visible changes, it was not
possible to keep the original behavior in all cases - for example when
handling NaN values, or when reusing code makes the functions return
consistent results.

Author: Emre Hasegeli
Reviewed-by: Kyotaro Horiguchi, me

Discussion: https://www.postgresql.org/message-id/CAE2gYzxF7-5djV6-cEvqQu-fNsnt%3DEqbOURx7ZDg%2BVv6ZMTWbg%40mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/a7dc63d904a6044d299aebdf59ad3199b6a9e99d

Modified Files
--------------
src/backend/utils/adt/geo_ops.c | 1882 +++++++++++++++++-------------------
src/backend/utils/adt/geo_spgist.c | 3 +-
src/include/utils/geo_decls.h | 4 -
src/test/regress/regress.c | 7 +-
4 files changed, 870 insertions(+), 1026 deletions(-)

On Sun, Jul 29, 2018 at 12:43:29AM +0000, Tomas Vondra wrote:
> Refactor geometric functions and operators
>
> The primary goal of this patch is to eliminate duplicate code and share
> code between different geometric data types more often, to prepare the
> ground for additional patches. Until now the code reuse was limited,
> probably because the simpler types (line and point) were implemented
> after the more complex ones.

It looks that this commit has upset a couple of OSX animals, locus and
prairiedog:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust&dt=2018-07-29%2013%3A13%3A35

Some AIX animals are also upset for the same reason with the handling
of a negative sign with 0.
--
Michael

On 07/29/2018 04:59 PM, Michael Paquier wrote:
> On Sun, Jul 29, 2018 at 12:43:29AM +0000, Tomas Vondra wrote:
>> Refactor geometric functions and operators
>>
>> The primary goal of this patch is to eliminate duplicate code and share
>> code between different geometric data types more often, to prepare the
>> ground for additional patches. Until now the code reuse was limited,
>> probably because the simpler types (line and point) were implemented
>> after the more complex ones.
>
> It looks that this commit has upset a couple of OSX animals, locus and
> prairiedog:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust&dt=2018-07-29%2013%3A13%3A35
>
> Some AIX animals are also upset for the same reason with the handling
> of a negative sign with 0.

Yeah, the commit broke this by inadvertedly undoing 43fe90f66a0 :-(

I've posted a fix to hackers [1]. If someone with access to an affected
machine could test if it actually does the trick, that would be nice.

[1]
https://www.postgresql.org/message-id/2fce5d35-2f07-8d72-42ec-81ed97fbe640%402ndquadrant.com

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello guys. Coverity complained about this patch as below. What, if
anything, should be done about it? One solution is to mark it as a
false-positive in Coverity, of course.

On 2018-Jul-29, scan-admin(at)coverity(dot)com wrote:

> ** CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
>
>
> ________________________________________________________________________________________________________
> *** CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/adt/geo_ops.c: 2647 in close_lseg()
> 2641 LSEG *l1 = PG_GETARG_LSEG_P(0);
> 2642 LSEG *l2 = PG_GETARG_LSEG_P(1);
> 2643 Point *result;
> 2644
> 2645 result = (Point *) palloc(sizeof(Point));
> 2646
> >>> CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> >>> The positions of arguments in the call to "lseg_closept_lseg" do not match the ordering of the parameters:
> * "l2" is passed to "l1"
> * "l1" is passed to "l2"
> 2647 lseg_closept_lseg(result, l2, l1);
> 2648
> 2649 PG_RETURN_POINT_P(result);
> 2650 }
> 2651
> 2652

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

To make coverity happy we might be able to suppress this false alarm by
adding a line like below:

```
/* coverity[SWAPPED_ARGUMENTS] */
lseg_closept_lseg(result, l2, l1);
```

From my point of view it's better to also put some comments for humans to
understand why we are passing l1 and l2 in reverse order.

On Wed, Aug 1, 2018 at 1:21 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Hello guys. Coverity complained about this patch as below. What, if
> anything, should be done about it? One solution is to mark it as a
> false-positive in Coverity, of course.
>
> On 2018-Jul-29, scan-admin(at)coverity(dot)com wrote:
>
> > ** CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> >
> >
> > ____________________________________________________________
> ____________________________________________
> > *** CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> > /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/adt/geo_ops.c:
> 2647 in close_lseg()
> > 2641 LSEG *l1 = PG_GETARG_LSEG_P(0);
> > 2642 LSEG *l2 = PG_GETARG_LSEG_P(1);
> > 2643 Point *result;
> > 2644
> > 2645 result = (Point *) palloc(sizeof(Point));
> > 2646
> > >>> CID 1438146: API usage errors (SWAPPED_ARGUMENTS)
> > >>> The positions of arguments in the call to "lseg_closept_lseg" do
> not match the ordering of the parameters:
> > * "l2" is passed to "l1"
> > * "l1" is passed to "l2"
> > 2647 lseg_closept_lseg(result, l2, l1);
> > 2648
> > 2649 PG_RETURN_POINT_P(result);
> > 2650 }
> > 2651
> > 2652
>
> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

Ning Yu <nyu(at)pivotal(dot)io> writes:
> From my point of view it's better to also put some comments for humans to
> understand why we are passing l1 and l2 in reverse order.

The header comment for lseg_closept_lseg() is pretty far from adequate
as well. After studying it for awhile I think I've puzzled out the
indeterminacies, but I shouldn't have had to. I think it probably
should have read

* Closest point on line segment l2 to line segment l1
*
* This returns the minimum distance from l1 to the closest point on l2.
* If result is not NULL, the closest point on l2 is stored at *result.

Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
that description. But the mere fact that there is any question about
that means that the function is poorly documented and perhaps poorly
named as well. For that matter, is there a good reason why l1/l2
have those roles and not the reverse?

While Coverity is surely operating from a shaky heuristic here, it's
got a point. The fact that it makes a difference which argument is
given first means that you've got to be really careful about which
is which, and this API spec is giving no help in that regard at all.

regards, tom lane

On 08/01/2018 04:22 AM, Tom Lane wrote:
> Ning Yu <nyu(at)pivotal(dot)io> writes:
>> From my point of view it's better to also put some comments for humans to
>> understand why we are passing l1 and l2 in reverse order.
>
> The header comment for lseg_closept_lseg() is pretty far from adequate
> as well. After studying it for awhile I think I've puzzled out the
> indeterminacies, but I shouldn't have had to. I think it probably
> should have read
>
> * Closest point on line segment l2 to line segment l1
> *
> * This returns the minimum distance from l1 to the closest point on l2.
> * If result is not NULL, the closest point on l2 is stored at *result.
>
> Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
> that description. But the mere fact that there is any question about
> that means that the function is poorly documented and perhaps poorly
> named as well. For that matter, is there a good reason why l1/l2
> have those roles and not the reverse?
>
> While Coverity is surely operating from a shaky heuristic here, it's
> got a point. The fact that it makes a difference which argument is
> given first means that you've got to be really careful about which
> is which, and this API spec is giving no help in that regard at all.
>

Yeah, I agree. The comments don't make it very clear what is the API
semantics. Will fix.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
> that description. But the mere fact that there is any question about
> that means that the function is poorly documented and perhaps poorly
> named as well. For that matter, is there a good reason why l1/l2
> have those roles and not the reverse?

Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1,
yyy *l2) functions in a way that they find the find the point on "l1".

On 08/01/2018 11:55 AM, Emre Hasegeli wrote:
>> Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
>> that description. But the mere fact that there is any question about
>> that means that the function is poorly documented and perhaps poorly
>> named as well. For that matter, is there a good reason why l1/l2
>> have those roles and not the reverse?
>
> Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1,
> yyy *l2) functions in a way that they find the find the point on "l1".
>

IMHO the main issue here is that the rule is not obvious / documented
anywhere. I think the best way to do that is by making it clear in a
comment for each such such function.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
> On 08/01/2018 11:55 AM, Emre Hasegeli wrote:
>> Consistency. I organized all xxx_closept_yyy(Point *result, xxx *l1,
>> yyy *l2) functions in a way that they find the find the point on "l1".

> IMHO the main issue here is that the rule is not obvious / documented
> anywhere. I think the best way to do that is by making it clear in a
> comment for each such such function.

I think there are three different things that need to be addressed:

* Underspecified comments.

* The function names and argument names are badly chosen IMO, because even
granted a convention such as the above, it's not very obvious what roles
"l1" and "l2" play. I'm not exactly sure what would be better, but if you
used names like "ofseg" and "otherseg" you'd at least be trying. I'd go
with an asymmetrical function name too, to make miswriting of calls less
likely.

* And lastly, are we sure there aren't actual *bugs* here? I'd initially
supposed that lseg_closept_lseg acted as Emre says above, but reading the
code makes me think it's the other way around. Its first two potential
assignments to *result are definitely assigning points on l2 not l1.

regards, tom lane

> I think there are three different things that need to be addressed:
>
> * Underspecified comments.

I agree. My patch added comments, the next one with actual fixes adds
more. I just didn't want to invest even more time on them while the
code is its current shape.

> * The function names and argument names are badly chosen IMO, because even
> granted a convention such as the above, it's not very obvious what roles
> "l1" and "l2" play. I'm not exactly sure what would be better, but if you
> used names like "ofseg" and "otherseg" you'd at least be trying. I'd go
> with an asymmetrical function name too, to make miswriting of calls less
> likely.

Good idea. I haven't though about that, but now such names makes more
sense to me. I will prepare another patch to improve the naming and
comments to be applied on top of my current patches.

> * And lastly, are we sure there aren't actual *bugs* here? I'd initially
> supposed that lseg_closept_lseg acted as Emre says above, but reading the
> code makes me think it's the other way around. Its first two potential
> assignments to *result are definitely assigning points on l2 not l1.

Yes, it is wrong beyond understanding. The committed patch intended
to keep answers as they were. The next one actually fixes those.

On 08/01/2018 04:07 PM, Emre Hasegeli wrote:
>> I think there are three different things that need to be addressed:
>>
>> * Underspecified comments.
>
> I agree. My patch added comments, the next one with actual fixes adds
> more. I just didn't want to invest even more time on them while the
> code is its current shape.
>
>> * The function names and argument names are badly chosen IMO, because even
>> granted a convention such as the above, it's not very obvious what roles
>> "l1" and "l2" play. I'm not exactly sure what would be better, but if you
>> used names like "ofseg" and "otherseg" you'd at least be trying. I'd go
>> with an asymmetrical function name too, to make miswriting of calls less
>> likely.
>
> Good idea. I haven't though about that, but now such names makes more
> sense to me. I will prepare another patch to improve the naming and
> comments to be applied on top of my current patches.
>
>> * And lastly, are we sure there aren't actual *bugs* here? I'd initially
>> supposed that lseg_closept_lseg acted as Emre says above, but reading the
>> code makes me think it's the other way around. Its first two potential
>> assignments to *result are definitely assigning points on l2 not l1.
>
> Yes, it is wrong beyond understanding. The committed patch intended
> to keep answers as they were. The next one actually fixes those.
>

OK, so I think we should not do anything about the issues reported by
coverity until we commit all the patches. Which may resolve some of
those (pre-existing) issues, for example.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> Good idea. I haven't though about that, but now such names makes more
> sense to me. I will prepare another patch to improve the naming and
> comments to be applied on top of my current patches.

The patch is attached to improve the comments and variable names. I
explained the functions with the same signature on the file header. I
can duplicate those on the function headers if you find that better.

On 2018-Nov-06, Emre Hasegeli wrote:

> The patch is attached to improve the comments and variable names. I
> explained the functions with the same signature on the file header. I
> can duplicate those on the function headers if you find that better.

Surely the comment in line 3839 deserves an update :-)

This seems good material. I would put the detailed conventions comment
separately from the head of the file, like this (where I also changed
"Type1 *type1" into "Type1 *obj1", and a few "has" to "have")

Thanks

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

> Surely the comment in line 3839 deserves an update :-)

Done.

> This seems good material. I would put the detailed conventions comment
> separately from the head of the file, like this (where I also changed
> "Type1 *type1" into "Type1 *obj1", and a few "has" to "have")

Looks better to me. I found one more "has" and changed it.

On 2018-Nov-06, Emre Hasegeli wrote:

> > Surely the comment in line 3839 deserves an update :-)
>
> Done.
>
> > This seems good material. I would put the detailed conventions comment
> > separately from the head of the file, like this (where I also changed
> > "Type1 *type1" into "Type1 *obj1", and a few "has" to "have")
>
> Looks better to me. I found one more "has" and changed it.

Pushed, with some further minor changes. I decided not to remove the
redundant comments that your patch was removing, as I felt that it's
better to keep the API contract together with the function definition.

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