Re: [GENERAL] Many Pl/PgSQL parameters -> AllocSetAlloc(128)? - Mailing list pgsql-patches

(moving to patches)

Tom Lane wrote:
> Joe Conway <mail@joeconway.com> writes:
>>Actually, adding a "pfree(oneres);" to the end of that for loop plugs
>>the memory leak and allows me to see the error message:
>
> On second look, you can't pfree oneres at the bottom of
> gen_cross_product() because it's part of the returned data structure
> --- note the assignment
>         *iter++ = oneres;

Yup, I see that now ;-)

> I think the bug here is that gen_cross_product should be ignoring
> argument positions that have nsupers == 0; those should always be
> assigned the same type as the input, since the regular type resolution
> algorithm is responsible for dealing with 'em.
>
> It might work to get rid of the "wild card" case (line 1115), which'd
> reduce the number of outputs to product(nsupers+1).  I doubt we really
> want any wild cards in there anymore.
>

The above didn't work, but if I understand correctly what that function
is intended to do, it seemed very broken. Basically this code:

   nanswers = 1;
   for (i = 0; i < nargs; i++)
   {
     nanswers *= (arginh[i].nsupers + 2);
     cur[i] = 0;
   }

for 24 arguments means 2^24 answers, even when there are no superclasses.

In that case (no arguments with superclasses), we ought to get:

nanswers = 1
iter should be sized at 1(all arginh[i].self)
                       + 1(all wildcard)
                       + 1 (null terminator)

The attached patch fixes this issue, but I'm still not entirely sure I
understand what the function was supposed to be doing, so please check
over my logic. Here's how I tested (after `make installcheck`)

CREATE OR REPLACE FUNCTION inhtest (d, integer, stud_emp, integer)
returns text as 'select ''OK''::text' language 'sql';
CREATE OR REPLACE FUNCTION get_d() returns d as 'select * from d limit
1' language 'sql';
CREATE OR REPLACE FUNCTION get_stud_emp() returns stud_emp as 'select *
from stud_emp limit 1' language 'sql';
select inhtest(get_d(), 'a'::text, get_stud_emp(), 1);

During testing, I had an elog(NOTICE,...) in there, and got the
following (reformatted) results:

    arg num =    3    2    1    0
    iter[j]
NOTICE:  j = 0        23    1764386    25    1881220
NOTICE:  j = 1        23    1764391    25    1881220
NOTICE:  j = 2        23    1764381    25    1881220
NOTICE:  j = 3        23    1764386    25    1881225
NOTICE:  j = 4        23    1764391    25    1881225
NOTICE:  j = 5        23    1764381    25    1881225
NOTICE:  j = 6        23    1764386    25    1881215
NOTICE:  j = 7        23    1764391    25    1881215
NOTICE:  j = 8        23    1764381    25    1881215

Seems correct to me.

Passes all regression tests.

Joe

Index: src/backend/parser/parse_func.c
===================================================================
RCS file: /opt/src/cvs/pgsql-server/src/backend/parser/parse_func.c,v
retrieving revision 1.149
diff -c -r1.149 parse_func.c
*** src/backend/parser/parse_func.c    6 Jun 2003 15:04:02 -0000    1.149
--- src/backend/parser/parse_func.c    24 Jun 2003 23:33:50 -0000
***************
*** 1075,1124 ****
                **iter;
      Oid           *oneres;
      int            i,
!                 j;
!     int            cur[FUNC_MAX_ARGS];

      nanswers = 1;
      for (i = 0; i < nargs; i++)
      {
!         nanswers *= (arginh[i].nsupers + 2);
!         cur[i] = 0;
      }

!     iter = result = (Oid **) palloc(sizeof(Oid *) * nanswers);

      /* compute the cross product from right to left */
!     for (;;)
      {
          oneres = (Oid *) palloc0(FUNC_MAX_ARGS * sizeof(Oid));

!         for (i = nargs - 1; i >= 0 && cur[i] > arginh[i].nsupers; i--)
!             continue;
!
!         /* if we're done, terminate with NULL pointer */
!         if (i < 0)
!         {
!             *iter = NULL;
!             return result;
!         }
!
!         /* no, increment this column and zero the ones after it */
!         cur[i] = cur[i] + 1;
!         for (j = nargs - 1; j > i; j--)
!             cur[j] = 0;
!
!         for (i = 0; i < nargs; i++)
          {
!             if (cur[i] == 0)
!                 oneres[i] = arginh[i].self;
!             else if (cur[i] > arginh[i].nsupers)
!                 oneres[i] = 0;    /* wild card */
              else
!                 oneres[i] = arginh[i].supervec[cur[i] - 1];
          }
!
!         *iter++ = oneres;
      }
  }


--- 1076,1117 ----
                **iter;
      Oid           *oneres;
      int            i,
!                 j,
!                 cum;

      nanswers = 1;
      for (i = 0; i < nargs; i++)
      {
!         if (arginh[i].nsupers)
!             nanswers *= arginh[i].nsupers;
      }

!     /* need room for wildcard vector and null terminator */
!     iter = result = (Oid **) palloc(sizeof(Oid *) * (nanswers + 2));

      /* compute the cross product from right to left */
!     for (j = 0; j < nanswers; j++)
      {
          oneres = (Oid *) palloc0(FUNC_MAX_ARGS * sizeof(Oid));
+         cum = 1;

!         for (i = nargs - 1; i >= 0; i--)
          {
!             if (arginh[i].nsupers)
!             {
!                 oneres[i] = arginh[i].supervec[(j / cum) % arginh[i].nsupers];
!                 cum *= arginh[i].nsupers;
!             }
              else
!                 oneres[i] = arginh[i].self;
          }
!         iter[j] = oneres;
      }
+     oneres = (Oid *) palloc0(FUNC_MAX_ARGS * sizeof(Oid));
+     iter[nanswers] = oneres;
+     iter[nanswers + 1] = NULL;
+
+     return result;
  }