Re: [HACKERS] Fix some corner cases that cube_in rejects

[Tom Lane]

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

> Note for committer :  There are unnecessary files (cube_1.out, cube_2.out & 
> cube_3.out) in contrib directory, that need to be removed at code checkin,  
> other than this concern, I think this patch looks pretty reasonable. Thanks.

Thanks for the review --- pushed.

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] Fix some corner cases that cube_in rejects

[Amul Sul]

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

Note for committer :  There are unnecessary files (cube_1.out, cube_2.out & 
cube_3.out) in contrib directory, that need to be removed at code checkin,  
other than this concern, I think this patch looks pretty reasonable. Thanks.

Regards,
Amul

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] Fix some corner cases that cube_in rejects

[Tom Lane]

Greg Stark  writes:
> On Mon, Aug 29, 2016 at 7:19 PM, Tom Lane  wrote:
>> To deal with the infinity/NaN issues, I made cube_in and cube_out rely
>> on float8in_internal and float8out_internal, as we recently did for the
>> core geometric types.  That causes the response to "1e-700" to be an
>> out-of-range error rather than silent underflow, which seems to me to
>> be fine, especially since it removes the platform dependency that's
>> responsible for needing the separate cube_1.out and cube_3.out files.

> So what happens to a database that has invalid cubes in it already?
> Offhand I suspect they mostly become valid since float8in will handle
> NaN and the like.

Nothing really.  cube_out works fine.  The point of substituting
float8out_internal is mostly to make sure we get platform-independent
spellings for inf and nan.

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] Fix some corner cases that cube_in rejects

[Greg Stark]

On Mon, Aug 29, 2016 at 7:19 PM, Tom Lane  wrote:
> To deal with the infinity/NaN issues, I made cube_in and cube_out rely
> on float8in_internal and float8out_internal, as we recently did for the
> core geometric types.  That causes the response to "1e-700" to be an
> out-of-range error rather than silent underflow, which seems to me to
> be fine, especially since it removes the platform dependency that's
> responsible for needing the separate cube_1.out and cube_3.out files.

So what happens to a database that has invalid cubes in it already?
Offhand I suspect they mostly become valid since float8in will handle
NaN and the like.

Incidentally, I so wish this module were named "vector" instead of
cube. I don't think I was confused by it for ages and still find it
confuses me for a few moments before I remember what it does.

-- 
greg


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

[HACKERS] Fix some corner cases that cube_in rejects

[Tom Lane]

In bug #14300 it's pointed out that cube_in rejects zero-element
cubes, as well as infinity and NaN coordinate values.  Since it's
easy to make such cube values via the cube-from-float-array
constructors, this is a dump/reload hazard.  The attached proposed
patch attempts to fix it up.

To deal with the infinity/NaN issues, I made cube_in and cube_out rely
on float8in_internal and float8out_internal, as we recently did for the
core geometric types.  That causes the response to "1e-700" to be an
out-of-range error rather than silent underflow, which seems to me to
be fine, especially since it removes the platform dependency that's
responsible for needing the separate cube_1.out and cube_3.out files.

I also took the opportunity to make cube_in's error strings and ERRCODE
results match project convention.  This is maybe a bit more debatable,
but I think it's worth doing as long as we're touching the function's
behavior.

I found only one other place that seemed to be assuming that cubes
aren't zero-length, but it would be worth someone reviewing it again
to see if I missed anything.  I'll put this on the commitfest queue.

regards, tom lane

diff --git a/contrib/cube/cube.c b/contrib/cube/cube.c
index 3feddef..9f81ec8 100644
*** a/contrib/cube/cube.c
--- b/contrib/cube/cube.c
*** cube_in(PG_FUNCTION_ARGS)
*** 122,128 
  	cube_scanner_init(str);
  
  	if (cube_yyparse() != 0)
! 		cube_yyerror(, "bogus input");
  
  	cube_scanner_finish();
  
--- 122,128 
  	cube_scanner_init(str);
  
  	if (cube_yyparse() != 0)
! 		cube_yyerror(, "cube parser failed");
  
  	cube_scanner_finish();
  
*** cube_out(PG_FUNCTION_ARGS)
*** 276,302 
  	StringInfoData buf;
  	int			dim = DIM(cube);
  	int			i;
- 	int			ndig;
  
  	initStringInfo();
  
- 	/*
- 	 * Get the number of digits to display.
- 	 */
- 	ndig = DBL_DIG + extra_float_digits;
- 	if (ndig < 1)
- 		ndig = 1;
- 
- 	/*
- 	 * while printing the first (LL) corner, check if it is equal to the
- 	 * second one
- 	 */
  	appendStringInfoChar(, '(');
  	for (i = 0; i < dim; i++)
  	{
  		if (i > 0)
  			appendStringInfoString(, ", ");
! 		appendStringInfo(, "%.*g", ndig, LL_COORD(cube, i));
  	}
  	appendStringInfoChar(, ')');
  
--- 276,290 
  	StringInfoData buf;
  	int			dim = DIM(cube);
  	int			i;
  
  	initStringInfo();
  
  	appendStringInfoChar(, '(');
  	for (i = 0; i < dim; i++)
  	{
  		if (i > 0)
  			appendStringInfoString(, ", ");
! 		appendStringInfoString(, float8out_internal(LL_COORD(cube, i)));
  	}
  	appendStringInfoChar(, ')');
  
*** cube_out(PG_FUNCTION_ARGS)
*** 307,313 
  		{
  			if (i > 0)
  appendStringInfoString(, ", ");
! 			appendStringInfo(, "%.*g", ndig, UR_COORD(cube, i));
  		}
  		appendStringInfoChar(, ')');
  	}
--- 295,301 
  		{
  			if (i > 0)
  appendStringInfoString(, ", ");
! 			appendStringInfoString(, float8out_internal(UR_COORD(cube, i)));
  		}
  		appendStringInfoChar(, ')');
  	}
*** g_cube_distance(PG_FUNCTION_ARGS)
*** 1370,1376 
  	{
  		int			coord = PG_GETARG_INT32(1);
  
! 		if (IS_POINT(cube))
  			retval = cube->x[(coord - 1) % DIM(cube)];
  		else
  			retval = Min(cube->x[(coord - 1) % DIM(cube)],
--- 1358,1366 
  	{
  		int			coord = PG_GETARG_INT32(1);
  
! 		if (DIM(cube) == 0)
! 			retval = 0.0;
! 		else if (IS_POINT(cube))
  			retval = cube->x[(coord - 1) % DIM(cube)];
  		else
  			retval = Min(cube->x[(coord - 1) % DIM(cube)],
diff --git a/contrib/cube/cubedata.h b/contrib/cube/cubedata.h
index 7eaac39..7dd9b11 100644
*** a/contrib/cube/cubedata.h
--- b/contrib/cube/cubedata.h
***
*** 1,5 
--- 1,9 
  /* contrib/cube/cubedata.h */
  
+ /*
+  * This limit is pretty arbitrary, but don't make it so large that you
+  * risk overflow in sizing calculations.
+  */
  #define CUBE_MAX_DIM (100)
  
  typedef struct NDBOX
diff --git a/contrib/cube/cubeparse.y b/contrib/cube/cubeparse.y
index 33606c7..1b65fa9 100644
*** a/contrib/cube/cubeparse.y
--- b/contrib/cube/cubeparse.y
***
*** 4,15 
  /* NdBox = [(lowerleft),(upperright)] */
  /* [(xLL(1)...xLL(N)),(xUR(1)...xUR(n))] */
  
- #define YYSTYPE char *
- #define YYDEBUG 1
- 
  #include "postgres.h"
  
  #include "cubedata.h"
  
  /*
   * Bison doesn't allocate anything that needs to live across parser calls,
--- 4,16 
  /* NdBox = [(lowerleft),(upperright)] */
  /* [(xLL(1)...xLL(N)),(xUR(1)...xUR(n))] */
  
  #include "postgres.h"
  
  #include "cubedata.h"
+ #include "utils/builtins.h"
+ 
+ /* All grammar constructs return strings */
+ #define YYSTYPE char *
  
  /*
   * Bison doesn't allocate anything that needs to live across parser calls,
***
*** 25,33 
  static char *scanbuf;
  static int	scanbuflen;
  
! static int delim_count(char *s, char delim);
! static NDBOX * write_box(unsigned int dim, char *str1, char *str2);
! static NDBOX * write_point_as_box(char *s, int dim);
  
  %}
  
--- 26,34