Re: Bogus documentation for bogus geometric operators

[Tom Lane]

Pavel Borisov  writes:
> I've made another check of the final state and suppose it is ready to be 
> pushed.

Sounds good, pushed.

regards, tom lane

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

I've made another check of the final state and suppose it is ready to be pushed.

Pavel Borisov

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

>
> The wording seems no problem to me. I  looked into a patch and changes
>> also seem sensible but I can not apply this patch because of really many
>> rejects. Which commit should I use to apply it onto?
>
> Sorry, the rejects were due to my git configuration. I will apply and make
the final checks soon.


-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

>
> >> undocumented.  Maybe instead of removing, change the text to be
> >> "Deprecated, use the equivalent XXX operator instead."  Or we could
> >> add a footnote similar to what was there for a previous renaming:
>
> > The problem that this new <<| is equivalent to <^ only for points (To
> > recap: the source of a problem is the same name of <^  operator for
> points
> > and boxes with different meaning for these types).
>
> I don't think it's that hard to be clear; see proposed wording below.
>
> The other loose end is that I don't think we can take away the opclass
> entries for the old spellings, unless we're willing to visibly break
> people's queries by removing those operator names altogether.  That
> doesn't seem like it'll fly when we haven't even deprecated the old
> names yet.  So for now, we have to support both names in the opclasses.
> I extended the patch to do that.
>
> This version seems committable to me --- any thoughts?
>
The wording seems no problem to me. I  looked into a patch and changes also
seem sensible but I can not apply this patch because of really many
rejects. Which commit should I use to apply it onto?

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Bogus documentation for bogus geometric operators

[Tom Lane]

Pavel Borisov  writes:
>> undocumented.  Maybe instead of removing, change the text to be
>> "Deprecated, use the equivalent XXX operator instead."  Or we could
>> add a footnote similar to what was there for a previous renaming:

> The problem that this new <<| is equivalent to <^ only for points (To
> recap: the source of a problem is the same name of <^  operator for points
> and boxes with different meaning for these types).

I don't think it's that hard to be clear; see proposed wording below.

The other loose end is that I don't think we can take away the opclass
entries for the old spellings, unless we're willing to visibly break
people's queries by removing those operator names altogether.  That
doesn't seem like it'll fly when we haven't even deprecated the old
names yet.  So for now, we have to support both names in the opclasses.
I extended the patch to do that.

This version seems committable to me --- any thoughts?

regards, tom lane

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 7d06b979eb..507bc1a668 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10609,7 +10609,7 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple


 Is first object strictly below second?
-Available for box, polygon,
+Available for point, box, polygon,
 circle.


@@ -10625,7 +10625,7 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple


 Is first object strictly above second?
-Available for box, polygon,
+Available for point, box, polygon,
 circle.


@@ -10680,21 +10680,6 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple

   
 
-  
-   
-point ^ point
-boolean
-   
-   
-Is first object strictly below second?
-(This operator is misnamed; it should be |.)
-   
-   
-point '(1,0)' ^ point '(1,1)'
-t
-   
-  
-
   

 box ^ box
@@ -10709,21 +10694,6 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple

   
 
-  
-   
-point ^ point
-boolean
-   
-   
-Is first object strictly above second?
-(This operator is misnamed; it should be |.)
-   
-   
-point '(1,1)' ^ point '(1,0)'
-t
-   
-  
-
   

 geometric_type ?# geometric_type
@@ -10877,6 +10847,18 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
 

 
+   
+
+ Before PostgreSQL 14, the point
+ is strictly below/above comparison operators point
+ | point and point
+ | point were respectively
+ called ^ and ^.  These
+ names are still available, but are deprecated and will eventually be
+ removed.
+
+   
+

 Geometric Functions
 
diff --git a/doc/src/sgml/gist.sgml b/doc/src/sgml/gist.sgml
index 1bf5f09659..d1b6cc9a01 100644
--- a/doc/src/sgml/gist.sgml
+++ b/doc/src/sgml/gist.sgml
@@ -118,12 +118,12 @@
 
  
   point_ops
-  ^ (point,point)
+  | (point,point)
   - (point,point)
  
   (point,point)
   (point,point)
- ^ (point,point)
+ | (point,point)
  ~= (point,point)
  @ (point,box)
  @ (point,polygon)
diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index 68d09951d9..ea88ae45e5 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -76,7 +76,7 @@
  
   box_ops
(box,box)
-  - (box,point)  
+  - (box,point)
  
   (box,box)
   (box,box)
@@ -92,12 +92,12 @@
 
  
   kd_point_ops
-  ^ (point,point)
+  | (point,point)
   - (point,point)
  
   (point,point)
   (point,point)
- ^ (point,point)
+ | (point,point)
  ~= (point,point)
  @ (point,box)
 
@@ -132,16 +132,16 @@
  | (polygon,polygon)
  | (polygon,polygon)
  | (polygon,polygon)
- | (polygon,polygon)  
+ | (polygon,polygon)
 
  
   quad_point_ops
-  ^ (point,point)
+  | (point,point)
   - (point,point)
  
   (point,point)
   (point,point)
- ^ (point,point)
+ | (point,point)
  ~= (point,point)
  @ (point,box)
 
@@ -159,7 +159,7 @@
   (anyrange,anyrange)
   (anyrange,anyrange)
  -|- (anyrange,anyrange)
- 
+
  
   text_ops
   = (text,text)
diff --git a/src/backend/access/gist/gistproc.c b/src/backend/access/gist/gistproc.c
index b03c4b55a0..784807c636 100644
--- a/src/backend/access/gist/gistproc.c
+++ b/src/backend/access/gist/gistproc.c
@@ -1341,8 +1341,18 @@ gist_point_consistent(PG_FUNCTION_ARGS)
 	StrategyNumber strategy = (StrategyNumber) PG_GETARG_UINT16(2);
 	bool	   *recheck = (bool *) PG_GETARG_POINTER(4);
 	bool		result;
-	

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

>
> 1. The patch removes <^ and >^ from func.sgml, which is fine, but

shouldn't there be an addition for the new operators?  (I think
>
I fully agree and added "point" as a possible input type for <<| and |>> in
manual. PFA v5


> undocumented.  Maybe instead of removing, change the text to be
> "Deprecated, use the equivalent XXX operator instead."  Or we could
> add a footnote similar to what was there for a previous renaming:
>
The problem that this new <<| is equivalent to <^ only for points (To
recap: the source of a problem is the same name of <^  operator for points
and boxes with different meaning for these types).

   point
   box
<<| |>>  strictly above/below  (new)
strictly above/below
<^ >^ strictly above/below (deprecated, but available)
 above/below

So it seems to me that trying to mention the subtle difference of
deprecated operator to same-named one for different data type inevitably
make things much worse for reader. On this reason I'd vote for complete
nuking <^ for point type (but this is not the only way so I haven't done
this in v5).

What do you think?

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 


v5-0001-Deprecate-and-replace-and-operators-for-points.patch
Description: Binary data

Re: Bogus documentation for bogus geometric operators

[Tom Lane]

Pavel Borisov  writes:
> [ v4-0001-Deprecate-and-replace-and-operators-for-points.patch ]

I made a cursory pass over this, and two things stood out to me:

1. The patch removes <^ and >^ from func.sgml, which is fine, but
shouldn't there be an addition for the new operators?  (I think
probably this need only be an addition of "point" as a possible
input type for <<| and |>>.)  Actually, as long we're not completely
removing these operators, I'm not sure it's okay to make them completely
undocumented.  Maybe instead of removing, change the text to be
"Deprecated, use the equivalent XXX operator instead."  Or we could
add a footnote similar to what was there for a previous renaming:

Note

Before PostgreSQL 8.2, the containment operators @> and <@ were
respectively called ~ and @. These names are still available, but
are deprecated and will eventually be removed.

2. I'm a bit queasy about removing these operators from the opclasses.
I'm not sure anyone will thank us for "the operator is still there, so
your query is still accepted, but it runs 1000X slower than before".
It seems like more plausible answers are either "nuke the operators
entirely, force people to change their queries now" or else "support
both old and new names in the opclasses for awhile to come".  In
previous operator renamings we've generally followed the second path,
so that's what I'm inclined to think should happen here.

regards, tom lane

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

>
> > I have only one thing to note: as this patch doesn't disable <^ and >^
> operator for boxes the existing state of documentation seem consistent to
> me:
> >
> > select '((0,0),(1,1))'::box <<| '((0,1),(1,2))'::box;
> > --
> >  f
> >
> > select '((0,0),(1,1))'::box <^ '((0,1),(1,2))'::box;
> > --
> >  t
> >
> > So I've only reverted the changes in the documentation on geometric
> functions in your patch.
>
> You are right.  We need to keep the documentation for box operators,
> but remove the lines for the point operators.
>

Indeed you are right. PFA v4 with documentation removed for <^ and >^ for
point
Thanks!

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 


v4-0001-Deprecate-and-replace-and-operators-for-points.patch
Description: Binary data

Re: Bogus documentation for bogus geometric operators

[Emre Hasegeli]

> I've rebased and tested your proposed patch. It seems fine and sensible to me.

Thanks

> I have only one thing to note: as this patch doesn't disable <^ and >^ 
> operator for boxes the existing state of documentation seem consistent to me:
>
> select '((0,0),(1,1))'::box <<| '((0,1),(1,2))'::box;
> --
>  f
>
> select '((0,0),(1,1))'::box <^ '((0,1),(1,2))'::box;
> --
>  t
>
> So I've only reverted the changes in the documentation on geometric functions 
> in your patch.

You are right.  We need to keep the documentation for box operators,
but remove the lines for the point operators.

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

Emre,

I've rebased and tested your proposed patch. It seems fine and sensible to
me.
I have only one thing to note: as this patch doesn't disable <^ and >^
operator for boxes the existing state of documentation seem consistent to
me:

select '((0,0),(1,1))'::box <<| '((0,1),(1,2))'::box;
--
 f

select '((0,0),(1,1))'::box <^ '((0,1),(1,2))'::box;
--
 t

So I've only reverted the changes in the documentation on geometric
functions in your patch.
PFA v3 of your patch. I'd mark it ready to commit if you agree.

Thank you!

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 


v3-0001-Deprecate-and-replace-and-operators-for-points.patch
Description: Binary data

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

 Emre, could you please again rebase your patch on top of
2f70fdb0644c32c4154236c2b5c241bec92eac5e
?
 It is not applied anymore.

>

-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Bogus documentation for bogus geometric operators

[Pavel Borisov]

> The subject is about the documentation, but the post reveals
> inconsistencies of the operators.  Tom Lane fixed the documentation on
> the back branches.  The new patch is to fix the operators on the
> master only.
>

Nice catch, thanks!
I agree that different operators should not have the same name and I'm
planning to review the patch soon. What are your ideas on the possibility
to backpatch it also? It seems a little bit weird that the operator can
change its name between versions of PG.
-- 
Best regards,
Pavel Borisov

Postgres Professional: http://postgrespro.com 

Re: Bogus documentation for bogus geometric operators

[Emre Hasegeli]

> Emre, the CF bot complains that this does not apply anymore, so please
> provide a rebase.  By the way, I am a bit confused to see a patch
> that adds new operators on a thread whose subject is about
> documentation.

Rebased version is attached.

The subject is about the documentation, but the post reveals
inconsistencies of the operators.  Tom Lane fixed the documentation on
the back branches.  The new patch is to fix the operators on the
master only.


0001-Add-and-operators-for-points-v02.patch
Description: Binary data

Re: Bogus documentation for bogus geometric operators

[Michael Paquier]

On Fri, Aug 21, 2020 at 12:00:45PM +0100, Emre Hasegeli wrote:
> I prepared a patch to add <<| and |>> operators for points to
> deprecate the previous ones.

Emre, the CF bot complains that this does not apply anymore, so please
provide a rebase.  By the way, I am a bit confused to see a patch
that adds new operators on a thread whose subject is about
documentation.
--
Michael


signature.asc
Description: PGP signature

Re: Bogus documentation for bogus geometric operators

[Emre Hasegeli]

> While revising the docs for the geometric operators, I came across
> these entries:
>
> <^  Is below (allows touching)? circle '((0,0),1)' <^ circle 
> '((0,5),1)'
> >^  Is above (allows touching)? circle '((0,5),1)' >^ circle 
> >'((0,0),1)'
>
> These have got more than a few problems:
>
> 1. There are no such operators for circles, so the examples are pure
> fantasy.
>
> 2. What these operators do exist for is points (point_below, point_above
> respectively) and boxes (box_below_eq, box_above_eq).  However, the
> stated behavior is not what the point functions actually do:
>
> point_below(PG_FUNCTION_ARGS)
> ...
> PG_RETURN_BOOL(FPlt(pt1->y, pt2->y));
>
> point_above(PG_FUNCTION_ARGS)
> ...
> PG_RETURN_BOOL(FPgt(pt1->y, pt2->y));
>
> So point_below would be more accurately described as "is strictly below",
> so its operator name really ought to be <<|.  And point_above is "is
> strictly above", so its operator name ought to be |>>.

I prepared a patch to add <<| and |>> operators for points to
deprecate the previous ones.

> 3. The box functions do seem to be correctly documented:
>
> box_below_eq(PG_FUNCTION_ARGS)
> ...
> PG_RETURN_BOOL(FPle(box1->high.y, box2->low.y));
>
> box_above_eq(PG_FUNCTION_ARGS)
> ...
> PG_RETURN_BOOL(FPge(box1->low.y, box2->high.y));
>
> But there are comments in the source code to the effect of
>
>  * box_below_eq and box_above_eq are obsolete versions that (probably
>  * erroneously) accept the equal-boundaries case.  Since these are not
>  * in sync with the box_left and box_right code, they are deprecated and
>  * not supported in the PG 8.1 rtree operator class extension.
>
> I'm not sure how seriously to take this deprecation comment, but it
> is true that box_below (<<|) and box_above (|>>) have analogs for
> other data types while these functions don't.

I think we should take this comment seriously and deprecate those
operators, so the patch removes them from the documentation.

> 4. Just for extra fun, these point operators are listed in some
> GIST and SP-GIST opclasses; though the box ones are not, as per
> that code comment.

It also updates the operator classes to support the new operators
instead of former ones.  I don't think there are many users of them to
notice the change.

I am adding this to the next commitfest.


0001-Add-and-operators-for-points-v01.patch
Description: Binary data

Re: Bogus documentation for bogus geometric operators

[Tom Lane]

Emre Hasegeli  writes:
>> Perhaps it's too late in the v13 cycle to actually do anything
>> about this code-wise, but what should I do documentation-wise?
>> I'm certainly not eager to document that these operators behave
>> inconsistently depending on which type you're talking about.

> I don't think we need to worry too much about doing something in the
> v13 cycle.  The geometric operators had and evidently still have so
> many bugs.  Nobody complains about them other than the developers who
> read the code.

Yeah, I ended up just documenting the current state of affairs.

> I am happy to prepare a patch for the next release to fix the current
> operators and add the missing ones.

Sounds great!

regards, tom lane

Re: Bogus documentation for bogus geometric operators

[Emre Hasegeli]

> Perhaps it's too late in the v13 cycle to actually do anything
> about this code-wise, but what should I do documentation-wise?
> I'm certainly not eager to document that these operators behave
> inconsistently depending on which type you're talking about.

I don't think we need to worry too much about doing something in the
v13 cycle.  The geometric operators had and evidently still have so
many bugs.  Nobody complains about them other than the developers who
read the code.

I am happy to prepare a patch for the next release to fix the current
operators and add the missing ones.

Bogus documentation for bogus geometric operators

[Tom Lane]

While revising the docs for the geometric operators, I came across
these entries:

<^  Is below (allows touching)? circle '((0,0),1)' <^ circle '((0,5),1)'
>^  Is above (allows touching)? circle '((0,5),1)' >^ circle '((0,0),1)'

These have got more than a few problems:

1. There are no such operators for circles, so the examples are pure
fantasy.

2. What these operators do exist for is points (point_below, point_above
respectively) and boxes (box_below_eq, box_above_eq).  However, the
stated behavior is not what the point functions actually do:

point_below(PG_FUNCTION_ARGS)
...
PG_RETURN_BOOL(FPlt(pt1->y, pt2->y));

point_above(PG_FUNCTION_ARGS)
...
PG_RETURN_BOOL(FPgt(pt1->y, pt2->y));

So point_below would be more accurately described as "is strictly below",
so its operator name really ought to be <<|.  And point_above is "is
strictly above", so its operator name ought to be |>>.

3. The box functions do seem to be correctly documented:

box_below_eq(PG_FUNCTION_ARGS)
...
PG_RETURN_BOOL(FPle(box1->high.y, box2->low.y));

box_above_eq(PG_FUNCTION_ARGS)
...
PG_RETURN_BOOL(FPge(box1->low.y, box2->high.y));

But there are comments in the source code to the effect of

 * box_below_eq and box_above_eq are obsolete versions that (probably
 * erroneously) accept the equal-boundaries case.  Since these are not
 * in sync with the box_left and box_right code, they are deprecated and
 * not supported in the PG 8.1 rtree operator class extension.

I'm not sure how seriously to take this deprecation comment, but it
is true that box_below (<<|) and box_above (|>>) have analogs for
other data types while these functions don't.

4. Just for extra fun, these point operators are listed in some
GIST and SP-GIST opclasses; though the box ones are not, as per
that code comment.

Perhaps it's too late in the v13 cycle to actually do anything
about this code-wise, but what should I do documentation-wise?
I'm certainly not eager to document that these operators behave
inconsistently depending on which type you're talking about.

regards, tom lane