I'm working on a patch to detect overflow in the integer-arithmetic
operators.  The first stage, covering the basic int4 operators, is
attached if anyone wants to comment on details.  A couple of general
questions though:

1. Does anyone object to applying this for 8.0?  I think we already had
consensus that it's a good idea, but if not now's the time to speak up.
(There are a couple of regression tests that fail and will need to be
adjusted, if that influences anyone's thinking.)

2. For the int2 and int8 operators, should we stick to a one-size-fits-all
message "integer out of range", or be more specific: "smallint out of
range" and "bigint out of range"?  The existing messages are not
completely consistent about this.  I'm inclined to go with mentioning
the specific type but I'm not dead set on it.

			regards, tom lane


*** src/backend/utils/adt/int.c.orig	Sun Aug 29 09:55:24 2004
--- src/backend/utils/adt/int.c	Sun Oct  3 15:20:13 2004
***************
*** 45,50 ****
--- 45,52 ----
  #define SHRT_MIN (-0x8000)
  #endif
  
+ #define SAMESIGN(a,b)	(((a) < 0) == ((b) < 0))
+ 
  typedef struct
  {
  	int32		current;
***************
*** 601,608 ****
  int4um(PG_FUNCTION_ARGS)
  {
  	int32		arg = PG_GETARG_INT32(0);
  
! 	PG_RETURN_INT32(-arg);
  }
  
  Datum
--- 604,618 ----
  int4um(PG_FUNCTION_ARGS)
  {
  	int32		arg = PG_GETARG_INT32(0);
+ 	int32		result;
  
! 	result = -arg;
! 	/* overflow check (needed for INT_MIN) */
! 	if (arg != 0 && SAMESIGN(result, arg))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
! 				 errmsg("integer out of range")));
! 	PG_RETURN_INT32(result);
  }
  
  Datum
***************
*** 618,625 ****
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
  
! 	PG_RETURN_INT32(arg1 + arg2);
  }
  
  Datum
--- 628,646 ----
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
+ 	int32		result;
  
! 	result = arg1 + arg2;
! 	/*
! 	 * Overflow check.  If the inputs are of different signs then their sum
! 	 * cannot overflow.  If the inputs are of the same sign, their sum
! 	 * had better be that sign too.
! 	 */
! 	if (SAMESIGN(arg1, arg2) && !SAMESIGN(result, arg1))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
! 				 errmsg("integer out of range")));
! 	PG_RETURN_INT32(result);
  }
  
  Datum
***************
*** 627,634 ****
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
  
! 	PG_RETURN_INT32(arg1 - arg2);
  }
  
  Datum
--- 648,666 ----
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
+ 	int32		result;
  
! 	result = arg1 - arg2;
! 	/*
! 	 * Overflow check.  If the inputs are of the same sign then their
! 	 * difference cannot overflow.  If they are of different signs then
! 	 * the result should be of the same sign as the first input.
! 	 */
! 	if (!SAMESIGN(arg1, arg2) && !SAMESIGN(result, arg1))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
! 				 errmsg("integer out of range")));
! 	PG_RETURN_INT32(result);
  }
  
  Datum
***************
*** 636,643 ****
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
  
! 	PG_RETURN_INT32(arg1 * arg2);
  }
  
  Datum
--- 668,695 ----
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
+ 	int32		result;
  
! 	result = arg1 * arg2;
! 	/*
! 	 * Overflow check.  We basically check to see if result / arg2 gives
! 	 * arg1 again.  There are two cases where this fails: arg2 = 0 (which
! 	 * cannot overflow) and arg1 = INT_MIN, arg2 = -1 (where the division
! 	 * itself will overflow and thus incorrectly match).
! 	 *
! 	 * Since the division is likely much more expensive than the actual
! 	 * multiplication, we'd like to skip it where possible.  The best
! 	 * bang for the buck seems to be to check whether both inputs are in
! 	 * the int16 range; if so, no overflow is possible.
! 	 */
! 	if (!(arg1 >= (int32) SHRT_MIN && arg1 <= (int32) SHRT_MAX &&
! 		  arg2 >= (int32) SHRT_MIN && arg2 <= (int32) SHRT_MAX) &&
! 		arg2 != 0 &&
! 		(result/arg2 != arg1 || (arg2 == -1 && arg1 < 0 && result < 0)))
! 		ereport(ERROR,
! 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
! 				 errmsg("integer out of range")));
! 	PG_RETURN_INT32(result);
  }
  
  Datum
***************
*** 645,665 ****
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
  
  	if (arg2 == 0)
  		ereport(ERROR,
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	PG_RETURN_INT32(arg1 / arg2);
  }
  
  Datum
  int4inc(PG_FUNCTION_ARGS)
  {
  	int32		arg = PG_GETARG_INT32(0);
  
! 	PG_RETURN_INT32(arg + 1);
  }
  
  Datum
--- 697,736 ----
  {
  	int32		arg1 = PG_GETARG_INT32(0);
  	int32		arg2 = PG_GETARG_INT32(1);
+ 	int32		result;
  
  	if (arg2 == 0)
  		ereport(ERROR,
  				(errcode(ERRCODE_DIVISION_BY_ZERO),
  				 errmsg("division by zero")));
  
! 	result = arg1 / arg2;
! 	/*
! 	 * Overflow check.  The only possible overflow case is for
! 	 * arg1 = INT_MIN, arg2 = -1, where the correct result is -INT_MIN,
! 	 * which can't be represented on a two's-complement machine.
! 	 */
! 	if (arg2 == -1 && arg1 < 0 && result < 0)
! 		ereport(ERROR,
! 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
! 				 errmsg("integer out of range")));
! 	PG_RETURN_INT32(result);
  }
  
  Datum
  int4inc(PG_FUNCTION_ARGS)
  {
  	int32		arg = PG_GETARG_INT32(0);
+ 	int32		result;
+ 
+ 	result = arg + 1;
+ 	/* Overflow check */
+ 	if (arg > 0 && result < 0)
+ 		ereport(ERROR,
+ 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ 				 errmsg("integer out of range")));
  
! 	PG_RETURN_INT32(result);
  }
  
  Datum

Sun, 03 Oct 2004 15:38:52 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Checking for overflow of integer arithmetic
On Sun, Oct 03, 2004 at 15:38:52 -0400,
  Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 1. Does anyone object to applying this for 8.0?  I think we already had
> consensus that it's a good idea, but if not now's the time to speak up.
> (There are a couple of regression tests that fail and will need to be
> adjusted, if that influences anyone's thinking.)

I think this should go in. I think not detecting overflow is really a bug.

> 2. For the int2 and int8 operators, should we stick to a one-size-fits-all
> message "integer out of range", or be more specific: "smallint out of
> range" and "bigint out of range"?  The existing messages are not
> completely consistent about this.  I'm inclined to go with mentioning
> the specific type but I'm not dead set on it.

I think giving the type info will be helpful for debugging.

Sun, 3 Oct 2004 18:29:42 -0500
Bruno Wolff III <bruno@wolff.to>
Re: Checking for overflow of integer arithmetic
Tom Lane wrote:

>   {
>   	int32		arg1 = PG_GETARG_INT32(0);
>   	int32		arg2 = PG_GETARG_INT32(1);
> + 	int32		result;
>   
> ! 	result = arg1 * arg2;
> ! 	/*
> ! 	 * Overflow check.  We basically check to see if result / arg2 gives
> ! 	 * arg1 again.  There are two cases where this fails: arg2 = 0 (which
> ! 	 * cannot overflow) and arg1 = INT_MIN, arg2 = -1 (where the division
> ! 	 * itself will overflow and thus incorrectly match).
> ! 	 *
> ! 	 * Since the division is likely much more expensive than the actual
> ! 	 * multiplication, we'd like to skip it where possible.  The best
> ! 	 * bang for the buck seems to be to check whether both inputs are in
> ! 	 * the int16 range; if so, no overflow is possible.
> ! 	 */
> ! 	if (!(arg1 >= (int32) SHRT_MIN && arg1 <= (int32) SHRT_MAX &&
> ! 		  arg2 >= (int32) SHRT_MIN && arg2 <= (int32) SHRT_MAX) &&
> ! 		arg2 != 0 &&
> ! 		(result/arg2 != arg1 || (arg2 == -1 && arg1 < 0 && result < 0)))
> ! 		ereport(ERROR,
> ! 				(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> ! 				 errmsg("integer out of range")));
> ! 	PG_RETURN_INT32(result);
>   }

May be is a good idea postpone the division as second "OR" argument in order to hope to avoid
it.


Regards
Gaetano Mendola












Mon, 04 Oct 2004 02:11:46 +0200
Gaetano Mendola <mendola@bigfoot.com>
Re: Checking for overflow of integer arithmetic