pgsql: Refactor geometric functions and operators

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]/messages/by-id/2fce5d35-2f07-8d72-42ec-81ed97fbe640@2ndquadrant.com. If someone with access to an affected
machine could test if it actually does the trick, that would be nice.

[1]: /messages/by-id/2fce5d35-2f07-8d72-42ec-81ed97fbe640@2ndquadrant.com
/messages/by-id/2fce5d35-2f07-8d72-42ec-81ed97fbe640@2ndquadrant.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@coverity.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@2ndquadrant.com>
wrote:

Ning Yu <nyu@pivotal.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@pivotal.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.vondra@2ndquadrant.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