Have crosstab variants treat NULL rowid as a category in its own right,

per suggestion from Tom Lane. This fixes crash-bug reported by Stefan
Schwarzer.

contrib/tablefunc/data/ct.data

@@ -12,3 +12,7 @@
 12	group2	test4	att1	val4
 13	group2	test4	att2	val5
 14	group2	test4	att3	val6
+15	group1	\N	att1	val9
+16	group1	\N	att2	val10
+17	group1	\N	att3	val11
+18	group1	\N	att4	val12

contrib/tablefunc/expected/tablefunc.out

@@ -23,42 +23,48 @@ SELECT * FROM crosstab2('SELECT rowid, attribute, val FROM ct where rowclass = '
 ----------+------------+------------
  test1    | val2       | val3
  test2    | val6       | val7
-(2 rows)
+          | val10      | val11
+(3 rows)
 
 SELECT * FROM crosstab3('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' and (attribute = ''att2'' or attribute = ''att3'') ORDER BY 1,2;');
  row_name | category_1 | category_2 | category_3 
 ----------+------------+------------+------------
  test1    | val2       | val3       | 
  test2    | val6       | val7       | 
-(2 rows)
+          | val10      | val11      | 
+(3 rows)
 
 SELECT * FROM crosstab4('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' and (attribute = ''att2'' or attribute = ''att3'') ORDER BY 1,2;');
  row_name | category_1 | category_2 | category_3 | category_4 
 ----------+------------+------------+------------+------------
  test1    | val2       | val3       |            | 
  test2    | val6       | val7       |            | 
-(2 rows)
+          | val10      | val11      |            | 
+(3 rows)
 
 SELECT * FROM crosstab2('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' ORDER BY 1,2;');
  row_name | category_1 | category_2 
 ----------+------------+------------
  test1    | val1       | val2
  test2    | val5       | val6
-(2 rows)
+          | val9       | val10
+(3 rows)
 
 SELECT * FROM crosstab3('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' ORDER BY 1,2;');
  row_name | category_1 | category_2 | category_3 
 ----------+------------+------------+------------
  test1    | val1       | val2       | val3
  test2    | val5       | val6       | val7
-(2 rows)
+          | val9       | val10      | val11
+(3 rows)
 
 SELECT * FROM crosstab4('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' ORDER BY 1,2;');
  row_name | category_1 | category_2 | category_3 | category_4 
 ----------+------------+------------+------------+------------
  test1    | val1       | val2       | val3       | val4
  test2    | val5       | val6       | val7       | val8
-(2 rows)
+          | val9       | val10      | val11      | val12
+(3 rows)
 
 SELECT * FROM crosstab2('SELECT rowid, attribute, val FROM ct where rowclass = ''group2'' and (attribute = ''att1'' or attribute = ''att2'') ORDER BY 1,2;');
  row_name | category_1 | category_2 
@@ -103,25 +109,28 @@ SELECT * FROM crosstab4('SELECT rowid, attribute, val FROM ct where rowclass = '
 (2 rows)
 
 SELECT * FROM crosstab('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' ORDER BY 1,2;', 2) AS c(rowid text, att1 text, att2 text);
- rowid | att1 | att2 
+ rowid | att1 | att2  
--------+------+------
+-------+------+-------
  test1 | val1 | val2
  test2 | val5 | val6
-(2 rows)
+       | val9 | val10
+(3 rows)
 
 SELECT * FROM crosstab('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' ORDER BY 1,2;', 3) AS c(rowid text, att1 text, att2 text, att3 text);
- rowid | att1 | att2 | att3 
+ rowid | att1 | att2  | att3  
--------+------+------+------
+-------+------+-------+-------
- test1 | val1 | val2 | val3
+ test1 | val1 | val2  | val3
- test2 | val5 | val6 | val7
+ test2 | val5 | val6  | val7
-(2 rows)
+       | val9 | val10 | val11
+(3 rows)
 
 SELECT * FROM crosstab('SELECT rowid, attribute, val FROM ct where rowclass = ''group1'' ORDER BY 1,2;', 4) AS c(rowid text, att1 text, att2 text, att3 text, att4 text);
- rowid | att1 | att2 | att3 | att4 
+ rowid | att1 | att2  | att3  | att4  
--------+------+------+------+------
+-------+------+-------+-------+-------
- test1 | val1 | val2 | val3 | val4
+ test1 | val1 | val2  | val3  | val4
- test2 | val5 | val6 | val7 | val8
+ test2 | val5 | val6  | val7  | val8
-(2 rows)
+       | val9 | val10 | val11 | val12
+(3 rows)
 
 -- test connectby with text based hierarchy
 CREATE TABLE connectby_text(keyid text, parent_keyid text);

contrib/tablefunc/tablefunc.c

@@ -93,6 +93,18 @@ typedef struct
 		} \
 	} while (0)
 
+#define xpstrdup(tgtvar_, srcvar_) \
+	do { \
+		if (srcvar_) \
+			tgtvar_ = pstrdup(srcvar_); \
+		else \
+			tgtvar_ = NULL; \
+	} while (0)
+
+#define xstreq(tgtvar_, srcvar_) \
+	(((tgtvar_ == NULL) && (srcvar_ == NULL)) || \
+	 ((tgtvar_ != NULL) && (srcvar_ != NULL) && (strcmp(tgtvar_, srcvar_) == 0)))
+
 /* sign, 10 digits, '\0' */
 #define INT32_STRLEN	12
 
@@ -299,6 +311,7 @@ crosstab(PG_FUNCTION_ARGS)
 	crosstab_fctx *fctx;
 	int			i;
 	int			num_categories;
+	bool		firstpass = false;
 	MemoryContext oldcontext;
 
 	/* stuff done only on the first call of the function */
@@ -420,6 +433,7 @@ crosstab(PG_FUNCTION_ARGS)
 		funcctx->max_calls = proc;
 
 		MemoryContextSwitchTo(oldcontext);
+		firstpass = true;
 	}
 
 	/* stuff done on every call of the function */
@@ -454,7 +468,7 @@ crosstab(PG_FUNCTION_ARGS)
 		HeapTuple	tuple;
 		Datum		result;
 		char	  **values;
-		bool		allnulls = true;
+		bool		skip_tuple = false;
 
 		while (true)
 		{
@@ -485,35 +499,42 @@ crosstab(PG_FUNCTION_ARGS)
 
 				/*
 				 * If this is the first pass through the values for this
-				 * rowid set it, otherwise make sure it hasn't changed on
+				 * rowid, set the first column to rowid
-				 * 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))
+					xpstrdup(values[0], rowid);
-						break;
-					else if (allnulls == true)
-						allnulls = false;
 
 					/*
-					 * Get the next category item value, which is alway
+					 * Check to see if the rowid is the same as that of the last
+					 * tuple sent -- if so, skip this tuple entirely
+					 */
+					if (!firstpass && xstreq(lastrowid, rowid))
+					{
+						skip_tuple = true;
+						break;
+					}
+				}
+
+				/*
+				 * If rowid hasn't changed on us, continue building the
+				 * ouput tuple.
+				 */
+				if (xstreq(rowid, values[0]))
+				{
+					/*
+					 * Get the next category item value, which is always
 					 * attribute number three.
 					 *
-					 * Be careful to sssign the value to the array index
+					 * Be careful to assign the value to the array index based
-					 * based on which category we are presently
+					 * on which category we are presently processing.
-					 * processing.
 					 */
 					values[1 + i] = SPI_getvalue(spi_tuple, spi_tupdesc, 3);
 
 					/*
-					 * increment the counter since we consume a row for
+					 * increment the counter since we consume a row for each
-					 * each category, but not for last pass because the
+					 * category, but not for last pass because the API will do
-					 * API will do that for us
+					 * that for us
 					 */
 					if (i < (num_categories - 1))
 						call_cntr = ++funcctx->call_cntr;
@@ -521,33 +542,29 @@ crosstab(PG_FUNCTION_ARGS)
 				else
 				{
 					/*
-					 * We'll fill in NULLs for the missing values, but we
+					 * We'll fill in NULLs for the missing values, but we need
-					 * need to decrement the counter since this sql result
+					 * to decrement the counter since this sql result row
-					 * row doesn't belong to the current output tuple.
+					 * doesn't belong to the current output tuple.
 					 */
 					call_cntr = --funcctx->call_cntr;
 					break;
 				}
-
+				xpfree(rowid);
-				if (rowid != NULL)
-					xpfree(rowid);
 			}
 
+			/*
+			 * switch to memory context appropriate for multiple function
+			 * calls
+			 */
+			oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
+
 			xpfree(fctx->lastrowid);
+			xpstrdup(fctx->lastrowid, values[0]);
+			lastrowid = fctx->lastrowid;
 
-			if (values[0] != NULL)
+			MemoryContextSwitchTo(oldcontext);
-			{
-				/*
-				 * switch to memory context appropriate for multiple
-				 * function calls
-				 */
-				oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
 
-				lastrowid = fctx->lastrowid = pstrdup(values[0]);
+			if (!skip_tuple)
-				MemoryContextSwitchTo(oldcontext);
-			}
-
-			if (!allnulls)
 			{
 				/* build the tuple */
 				tuple = BuildTupleFromCStrings(attinmeta, values);
@@ -566,8 +583,8 @@ crosstab(PG_FUNCTION_ARGS)
 			else
 			{
 				/*
-				 * Skipping this tuple entirely, but we need to advance
+				 * Skipping this tuple entirely, but we need to advance the
-				 * the counter like the API would if we had returned one.
+				 * counter like the API would if we had returned one.
 				 */
 				call_cntr = ++funcctx->call_cntr;
 
@@ -581,11 +598,14 @@ crosstab(PG_FUNCTION_ARGS)
 					SPI_finish();
 					SRF_RETURN_DONE(funcctx);
 				}
+
+				/* need to reset this before the next tuple is started */
+				skip_tuple = false;
 			}
 		}
 	}
 	else
-/* do when there is no more left */
+		/* do when there is no more left */
 	{
 		/* release SPI related resources */
 		SPI_finish();