Re: [HACKERS] 9.4rc bug in percentile_cont

2014-12-13 Thread Tom Lane
Andrew Gierth and...@tao11.riddles.org.uk writes:
 Just got a report on IRC of a bug in the array version of
 percentile_cont; if two of the requested percentiles were between the
 same pair of input rows, the result could be wrong or an error would
 be generated.

Oooh, good catch.

 Proposed patch (against current master) attached.

I think this patch could be improved a bit; will work on it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] 9.4rc bug in percentile_cont

2014-12-12 Thread Andrew Gierth
Just got a report on IRC of a bug in the array version of
percentile_cont; if two of the requested percentiles were between the
same pair of input rows, the result could be wrong or an error would
be generated.

e.g.

select percentile_cont(array[0.4,0.6]) within group (order by gs)
  from generate_series(1,2) gs;
ERROR: missing row in percentile_cont

Proposed patch (against current master) attached.

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 135a14b..1efbf07 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -919,7 +919,7 @@ percentile_cont_multi_final_common(FunctionCallInfo fcinfo,
 
 rownum = target_row;
 			}
-			else
+			else if (target_row == rownum)
 			{
 /*
  * We are already at the target row, so we must previously
@@ -928,15 +928,23 @@ percentile_cont_multi_final_common(FunctionCallInfo fcinfo,
 first_val = second_val;
 			}
 
-			/* Fetch second_row if needed */
-			if (need_lerp)
+			/*
+			 * We might be lerping between the same two rows of input as the
+			 * previous value. If so, rownum will already be ahead of
+			 * target_row, and we skip any attempt to read more.
+			 */
+			if (target_row == rownum)
 			{
-if (!tuplesort_getdatum(osastate-sortstate, true, second_val, isnull) || isnull)
-	elog(ERROR, missing row in percentile_cont);
-rownum++;
+/* Fetch second_row if needed */
+if (need_lerp)
+{
+	if (!tuplesort_getdatum(osastate-sortstate, true, second_val, isnull) || isnull)
+		elog(ERROR, missing row in percentile_cont);
+	rownum++;
+}
+else
+	second_val = first_val;
 			}
-			else
-second_val = first_val;
 
 			/* Compute appropriate result */
 			if (need_lerp)
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 58df854..40f5398 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -1423,11 +1423,11 @@ from tenk1;
  {{NULL,999,499},{749,249,NULL}}
 (1 row)
 
-select percentile_cont(array[0,1,0.25,0.75,0.5,1]) within group (order by x)
+select percentile_cont(array[0,1,0.25,0.75,0.5,1,0.3,0.32,0.35,0.38,0.4]) within group (order by x)
 from generate_series(1,6) x;
-percentile_cont

- {1,6,2.25,4.75,3.5,6}
+ percentile_cont  
+--
+ {1,6,2.25,4.75,3.5,6,2.5,2.6,2.75,2.9,3}
 (1 row)
 
 select ten, mode() within group (order by string4) from tenk1 group by ten;
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 8096a6f..a84327d 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -533,7 +533,7 @@ select percentile_cont(array[0,0.25,0.5,0.75,1]) within group (order by thousand
 from tenk1;
 select percentile_disc(array[[null,1,0.5],[0.75,0.25,null]]) within group (order by thousand)
 from tenk1;
-select percentile_cont(array[0,1,0.25,0.75,0.5,1]) within group (order by x)
+select percentile_cont(array[0,1,0.25,0.75,0.5,1,0.3,0.32,0.35,0.38,0.4]) within group (order by x)
 from generate_series(1,6) x;
 
 select ten, mode() within group (order by string4) from tenk1 group by ten;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers