Thread: Bug in intarray?

Bug in intarray?

From
Guillaume Lelarge
Date:
Hi,

On a french PostgreSQL web forum, one of our users asked about a curious
behaviour of the intarray extension.

This query:
  SELECT ARRAY[-1,3,1] & ARRAY[1, 2];
should give {1} as a result.

But, on HEAD (and according to his tests, on 9.0.6 and 9.1.2), it
appears to give en empty array.

Digging on this issue, another user (Julien Rouhaud) made an interesting
comment on this line of code:

if (i + j == 0 || (i + j > 0 && *(dr - 1) != db[j]))

(line 159 of contrib/intarray/_int_tool.c, current HEAD)

Apparently, the code tries to check the current value of the right side
array with the previous value of the resulting array. Which clearly
cannot work if there is no previous value in the resulting array.

So I worked on a patch to fix this, as I think it is a bug (but I may be
wrong). Patch is attached and fixes the issue AFAICT.

Thanks.


--
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.com

Attachment

Re: Bug in intarray?

From
Tom Lane
Date:
Guillaume Lelarge <guillaume@lelarge.info> writes:
> This query:
>   SELECT ARRAY[-1,3,1] & ARRAY[1, 2];
> should give {1} as a result.

> But, on HEAD (and according to his tests, on 9.0.6 and 9.1.2), it
> appears to give en empty array.

Definitely a bug, and I'll bet it goes all the way back.

> Digging on this issue, another user (Julien Rouhaud) made an interesting
> comment on this line of code:

> if (i + j == 0 || (i + j > 0 && *(dr - 1) != db[j]))

> (line 159 of contrib/intarray/_int_tool.c, current HEAD)

> Apparently, the code tries to check the current value of the right side
> array with the previous value of the resulting array. Which clearly
> cannot work if there is no previous value in the resulting array.

> So I worked on a patch to fix this, as I think it is a bug (but I may be
> wrong). Patch is attached and fixes the issue AFAICT.

Yeah, this code is bogus, but it's also pretty unreadable.  I think
it's better to get rid of the inconsistently-used pointer arithmetic
and the fundamentally wrong/irrelevant test on i+j, along the lines
of the attached.

            regards, tom lane


diff --git a/contrib/intarray/_int_tool.c b/contrib/intarray/_int_tool.c
index 79f018d333b1d6810aa7fd2996c158b41b0263fb..132d15316054d842593468f191eca9053846f706 100644
*** a/contrib/intarray/_int_tool.c
--- b/contrib/intarray/_int_tool.c
*************** inner_int_inter(ArrayType *a, ArrayType
*** 140,146 ****
                 *db,
                 *dr;
      int            i,
!                 j;

      if (ARRISEMPTY(a) || ARRISEMPTY(b))
          return new_intArrayType(0);
--- 140,147 ----
                 *db,
                 *dr;
      int            i,
!                 j,
!                 k;

      if (ARRISEMPTY(a) || ARRISEMPTY(b))
          return new_intArrayType(0);
*************** inner_int_inter(ArrayType *a, ArrayType
*** 152,166 ****
      r = new_intArrayType(Min(na, nb));
      dr = ARRPTR(r);

!     i = j = 0;
      while (i < na && j < nb)
      {
          if (da[i] < db[j])
              i++;
          else if (da[i] == db[j])
          {
!             if (i + j == 0 || (i + j > 0 && *(dr - 1) != db[j]))
!                 *dr++ = db[j];
              i++;
              j++;
          }
--- 153,167 ----
      r = new_intArrayType(Min(na, nb));
      dr = ARRPTR(r);

!     i = j = k = 0;
      while (i < na && j < nb)
      {
          if (da[i] < db[j])
              i++;
          else if (da[i] == db[j])
          {
!             if (k == 0 || dr[k - 1] != db[j])
!                 dr[k++] = db[j];
              i++;
              j++;
          }
*************** inner_int_inter(ArrayType *a, ArrayType
*** 168,180 ****
              j++;
      }

!     if ((dr - ARRPTR(r)) == 0)
      {
          pfree(r);
          return new_intArrayType(0);
      }
      else
!         return resize_intArrayType(r, dr - ARRPTR(r));
  }

  void
--- 169,181 ----
              j++;
      }

!     if (k == 0)
      {
          pfree(r);
          return new_intArrayType(0);
      }
      else
!         return resize_intArrayType(r, k);
  }

  void
diff --git a/contrib/intarray/expected/_int.out b/contrib/intarray/expected/_int.out
index 6ed3cc6ced096e2e97665e76ceba7fc139415029..4080b9633fe98a91861684e0d82f1297c21b91af 100644
*** a/contrib/intarray/expected/_int.out
--- b/contrib/intarray/expected/_int.out
*************** SELECT '{123,623,445}'::int[] & '{1623,6
*** 137,142 ****
--- 137,148 ----
   {623}
  (1 row)

+ SELECT '{-1,3,1}'::int[] & '{1,2}';
+  ?column?
+ ----------
+  {1}
+ (1 row)
+
  --test query_int
  SELECT '1'::query_int;
   query_int
diff --git a/contrib/intarray/sql/_int.sql b/contrib/intarray/sql/_int.sql
index b60e936dc520d8308bd16e50681f061dc86e2dfe..216c5c58d615a7cd1a5fe3714c9a5c91fb255138 100644
*** a/contrib/intarray/sql/_int.sql
--- b/contrib/intarray/sql/_int.sql
*************** SELECT '{123,623,445}'::int[] | 623;
*** 24,29 ****
--- 24,30 ----
  SELECT '{123,623,445}'::int[] | 1623;
  SELECT '{123,623,445}'::int[] | '{1623,623}';
  SELECT '{123,623,445}'::int[] & '{1623,623}';
+ SELECT '{-1,3,1}'::int[] & '{1,2}';


  --test query_int

Re: Bug in intarray?

From
Guillaume Lelarge
Date:
On Thu, 2012-02-16 at 19:27 -0500, Tom Lane wrote:
> Guillaume Lelarge <guillaume@lelarge.info> writes:
> > This query:
> >   SELECT ARRAY[-1,3,1] & ARRAY[1, 2];
> > should give {1} as a result.
> 
> > But, on HEAD (and according to his tests, on 9.0.6 and 9.1.2), it
> > appears to give en empty array.
> 
> Definitely a bug, and I'll bet it goes all the way back.
> 
> > Digging on this issue, another user (Julien Rouhaud) made an interesting
> > comment on this line of code:
> 
> > if (i + j == 0 || (i + j > 0 && *(dr - 1) != db[j]))
> 
> > (line 159 of contrib/intarray/_int_tool.c, current HEAD)
> 
> > Apparently, the code tries to check the current value of the right side
> > array with the previous value of the resulting array. Which clearly
> > cannot work if there is no previous value in the resulting array.
> 
> > So I worked on a patch to fix this, as I think it is a bug (but I may be
> > wrong). Patch is attached and fixes the issue AFAICT.
> 
> Yeah, this code is bogus, but it's also pretty unreadable.  I think
> it's better to get rid of the inconsistently-used pointer arithmetic
> and the fundamentally wrong/irrelevant test on i+j, along the lines
> of the attached.
> 

Completely agree.

Thank you.


-- 
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.com