[Fwd: Re: [GENERAL] Crosstab Problems]

Oops, just noticed I sent this to the General list instead of Patches --
sorry about that.

Joe

-------- Original Message --------
Subject: Re: [GENERAL] Crosstab Problems
Date: Wed, 24 Oct 2007 19:26:16 -0700
From: Joe Conway <mail@joeconway.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
CC: Jorge Godoy <jgodoy@gmail.com>,  pgsql-general@postgresql.org,
Stefan Schwarzer <stefan.schwarzer@grid.unep.ch>
References: <4E6E765B-2899-4B85-9131-A8847FB06305@grid.unep.ch>
<24199.1192722457@sss.pgh.pa.us> <4717A807.1030209@joeconway.com>
<200710182102.56340.jgodoy@gmail.com> <3161.1192763352@sss.pgh.pa.us>

Tom Lane wrote:
> Jorge Godoy <jgodoy@gmail.com> writes:
>> Em Thursday 18 October 2007 16:37:59 Joe Conway escreveu:
>>> The row is pretty useless without a rowid in this context -- it seems
>>> like the best thing to do would be to skip those rows entirely. Of
>>> course you could argue I suppose that it ought to throw an ERROR and
>>> bail out entirely. Maybe a good compromise would be to skip the row but
>>> throw a NOTICE?
>
>> If I were using it and having this problem I'd rather have an ERROR.
>
> I can think of four reasonably credible alternatives:
>
> 1. Treat NULL rowid as a category in its own right.  This would conform
> with the behavior of GROUP BY and DISTINCT, for instance.

  > 4. Silently ignore rows with NULL rowid.

After looking closer I realized that #4 was my original intention, and
there was even code attempting to implement it, but just not very well ;-(.

In any case, the attached changes the behavior to #1 for both flavors of
crosstab (the original crosstab(text, int) and the usually more useful
crosstab(text, text)).

It is appropriate for 8.3 but not back-patching as it changes behavior
in a non-backward compatible way and is probably too invasive anyway.
I'll do something much simpler just to prevent crashing for 8.2 and earlier.

If there are no objections I'll apply Thursday.

Joe

Index: tablefunc.c
===================================================================
RCS file: /opt/src/cvs/pgsql/contrib/tablefunc/tablefunc.c,v
retrieving revision 1.47
diff -c -r1.47 tablefunc.c
*** tablefunc.c    3 Mar 2007 19:32:54 -0000    1.47
--- tablefunc.c    25 Oct 2007 02:11:06 -0000
***************
*** 355,360 ****
--- 355,361 ----
      crosstab_fctx *fctx;
      int            i;
      int            num_categories;
+     bool        firstpass = false;
      MemoryContext oldcontext;

      /* stuff done only on the first call of the function */
***************
*** 469,474 ****
--- 470,476 ----
          funcctx->max_calls = proc;

          MemoryContextSwitchTo(oldcontext);
+         firstpass = true;
      }

      /* stuff done on every call of the function */
***************
*** 500,506 ****
          HeapTuple    tuple;
          Datum        result;
          char      **values;
!         bool        allnulls = true;

          while (true)
          {
--- 502,508 ----
          HeapTuple    tuple;
          Datum        result;
          char      **values;
!         bool        skip_tuple = false;

          while (true)
          {
***************
*** 530,555 ****
                  rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);

                  /*
!                  * If this is the first pass through the values for this rowid
!                  * set it, otherwise make sure it hasn't changed on us. Also
!                  * check to see if the rowid is the same as that of the last
!                  * tuple sent -- if so, skip this tuple entirely
                   */
                  if (i == 0)
-                     values[0] = pstrdup(rowid);
-
-                 if ((rowid != NULL) && (strcmp(rowid, values[0]) == 0))
                  {
!                     if ((lastrowid != NULL) && (strcmp(rowid, lastrowid) == 0))
                          break;
!                     else if (allnulls == true)
!                         allnulls = false;

                      /*
!                      * Get the next category item value, which is alway
                       * attribute number three.
                       *
!                      * Be careful to sssign the value to the array index based
                       * on which category we are presently processing.
                       */
                      values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
--- 532,574 ----
                  rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);

                  /*
!                  * If this is the first pass through the values for this
!                  * rowid, set the first column to rowid
                   */
                  if (i == 0)
                  {
!                     if (rowid)
!                         values[0] = pstrdup(rowid);
!                     else
!                         values[0] = NULL;
!
!                     /*
!                      * Check to see if the rowid is the same as that of the last
!                      * tuple sent -- if so, skip this tuple entirely
!                      */
!                     if (!firstpass &&
!                         (((lastrowid == NULL) && (rowid == NULL)) ||
!                          ((lastrowid != NULL) &&
!                           (rowid != NULL) &&
!                           (strcmp(rowid, lastrowid) == 0))))
!                     {
!                         skip_tuple = true;
                          break;
!                     }
!                 }

+                 /*
+                  * If rowid hasn't changed on us, continue building the
+                  * ouput tuple.
+                  */
+                 if ((rowid && values[0] && (strcmp(rowid, values[0]) == 0)) ||
+                     ((rowid == NULL) && (values[0] == NULL)))
+                 {
                      /*
!                      * Get the next category item value, which is always
                       * attribute number three.
                       *
!                      * Be careful to assign the value to the array index based
                       * on which category we are presently processing.
                       */
                      values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
***************
*** 572,584 ****
                      call_cntr = --funcctx->call_cntr;
                      break;
                  }
!
!                 if (rowid != NULL)
!                     xpfree(rowid);
              }

              xpfree(fctx->lastrowid);
-
              if (values[0] != NULL)
              {
                  /*
--- 591,600 ----
                      call_cntr = --funcctx->call_cntr;
                      break;
                  }
!                 xpfree(rowid);
              }

              xpfree(fctx->lastrowid);
              if (values[0] != NULL)
              {
                  /*
***************
*** 586,597 ****
                   * calls
                   */
                  oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
-
                  lastrowid = fctx->lastrowid = pstrdup(values[0]);
                  MemoryContextSwitchTo(oldcontext);
              }

!             if (!allnulls)
              {
                  /* build the tuple */
                  tuple = BuildTupleFromCStrings(attinmeta, values);
--- 602,614 ----
                   * calls
                   */
                  oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
                  lastrowid = fctx->lastrowid = pstrdup(values[0]);
                  MemoryContextSwitchTo(oldcontext);
              }
+             else
+                 lastrowid = fctx->lastrowid = NULL;

!             if (!skip_tuple)
              {
                  /* build the tuple */
                  tuple = BuildTupleFromCStrings(attinmeta, values);
***************
*** 625,630 ****
--- 642,650 ----
                      SPI_finish();
                      SRF_RETURN_DONE(funcctx);
                  }
+
+                 /* need to reset this before the next tuple is started */
+                 skip_tuple = false;
              }
          }
      }
***************
*** 856,861 ****
--- 876,882 ----
          int            ncols = spi_tupdesc->natts;
          char       *rowid;
          char       *lastrowid = NULL;
+         bool        firstpass = true;
          int            i,
                      j;
          int            result_ncols;
***************
*** 918,938 ****
              /* get the rowid from the current sql result tuple */
              rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);

-             /* if rowid is null, skip this tuple entirely */
-             if (rowid == NULL)
-                 continue;
-
              /*
               * if we're on a new output row, grab the column values up to
               * column N-2 now
               */
!             if ((lastrowid == NULL) || (strcmp(rowid, lastrowid) != 0))
              {
                  /*
                   * a new row means we need to flush the old one first, unless
                   * we're on the very first row
                   */
!                 if (lastrowid != NULL)
                  {
                      /* rowid changed, flush the previous output row */
                      tuple = BuildTupleFromCStrings(attinmeta, values);
--- 939,958 ----
              /* get the rowid from the current sql result tuple */
              rowid = SPI_getvalue(spi_tuple, spi_tupdesc, 1);

              /*
               * if we're on a new output row, grab the column values up to
               * column N-2 now
               */
!             if (firstpass ||
!                 (lastrowid == NULL && rowid != NULL) ||
!                 (lastrowid != NULL && rowid == NULL) ||
!                 (lastrowid != NULL && rowid != NULL && (strcmp(rowid, lastrowid) != 0)))
              {
                  /*
                   * a new row means we need to flush the old one first, unless
                   * we're on the very first row
                   */
!                 if (!firstpass)
                  {
                      /* rowid changed, flush the previous output row */
                      tuple = BuildTupleFromCStrings(attinmeta, values);
***************
*** 949,954 ****
--- 969,977 ----
                  values[0] = rowid;
                  for (j = 1; j < ncols - 2; j++)
                      values[j] = SPI_getvalue(spi_tuple, spi_tupdesc, j + 1);
+
+                 /* we're no longer on the first pass */
+                 firstpass = false;
              }

              /* look up the category and fill in the appropriate column */
***************
*** 964,970 ****
              }

              xpfree(lastrowid);
!             lastrowid = pstrdup(rowid);
          }

          /* flush the last output row */
--- 987,994 ----
              }

              xpfree(lastrowid);
!             if (rowid)
!                 lastrowid = pstrdup(rowid);
          }

          /* flush the last output row */