This is an automated email from the ASF dual-hosted git repository.
leborchuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudberry.git
The following commit(s) were added to refs/heads/main by this push:
new a12d1b65444 Fix two crashes/wrong-answer bugs in the gp_percentile_*
transition functions (in main) (#1937)
a12d1b65444 is described below
commit a12d1b65444164774eb71c39057899c89faa746e
Author: Alena Rybakina <[email protected]>
AuthorDate: Fri Sep 18 15:19:43 2026 +0300
Fix two crashes/wrong-answer bugs in the gp_percentile_* transition
functions (in main) (#1937)
* Fix catalog signatures of the gp_percentile transition functions
gp_percentile_cont_{float8,interval,timestamp,timestamptz}_transition and
gp_percentile_disc_transition all read five arguments in C: the running
transition state plus the four arguments of the gp_percentile_cont() and
gp_percentile_disc() aggregates. pg_proc.dat, however, declared them with
four. Called as a transition function that is harmless, since the executor
supplies state + 4 arguments regardless of the catalog, but a direct SQL
call
reaches past the end of the argument array: PG_GETARG_INT64(4) picks up
garbage, which yields wrong results, an assertion when the bogus peer count
makes the code pfree() a NULL pointer, or a segfault.
Co-authored-by: Georgy Shelkovy <[email protected]>
Ported from Greengage commit 477b04a (ADBDEV-7770).
* Keep the isnull flag when gp_percentile_* returns its previous state
Both gp_percentile transition functions return the previous transition state
untouched when the row they are looking at is not one of the rows the
percentile is computed from. On the very first call that state is NULL, and
returning it as a bare Datum(0) with isnull left false loses the flag: the
aggregate yields 0 instead of NULL for by-value types, and dereferences a
NULL
pointer in the output function for by-reference ones - so an empty input set
crashed the backend for the interval, timestamp and timestamptz variants.
Co-authored-by: Georgy Shelkovy <[email protected]>
Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770).
---
.../src/test/regress/expected/opr_sanity.out | 11 +--
src/backend/utils/adt/orderedsetaggs.c | 21 +++-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 10 +-
src/test/regress/expected/opr_sanity.out | 11 +--
src/test/regress/expected/percentile.out | 106 +++++++++++++++++++++
src/test/regress/sql/percentile.sql | 31 ++++++
7 files changed, 165 insertions(+), 27 deletions(-)
diff --git a/contrib/pax_storage/src/test/regress/expected/opr_sanity.out
b/contrib/pax_storage/src/test/regress/expected/opr_sanity.out
index 4cdd156d78e..6d91930ec19 100644
--- a/contrib/pax_storage/src/test/regress/expected/opr_sanity.out
+++ b/contrib/pax_storage/src/test/regress/expected/opr_sanity.out
@@ -1536,14 +1536,9 @@ WHERE a.aggfnoid = p.oid AND
-- we could carry the check further, but 4 args is enough for now
OR (p.pronargs > 4)
);
- aggfnoid | proname | oid | proname
-----------+--------------------+------+-------------------------------------------
- 9189 | gp_percentile_cont | 9184 | gp_percentile_cont_float8_transition
- 9190 | gp_percentile_cont | 9185 | gp_percentile_cont_interval_transition
- 9191 | gp_percentile_cont | 9186 | gp_percentile_cont_timestamp_transition
- 9192 | gp_percentile_cont | 9187 |
gp_percentile_cont_timestamptz_transition
- 9194 | gp_percentile_disc | 9193 | gp_percentile_disc_transition
-(5 rows)
+ aggfnoid | proname | oid | proname
+----------+---------+-----+---------
+(0 rows)
-- Cross-check finalfn (if present) against its entry in pg_proc.
SELECT a.aggfnoid::oid, p.proname, pfn.oid, pfn.proname
diff --git a/src/backend/utils/adt/orderedsetaggs.c
b/src/backend/utils/adt/orderedsetaggs.c
index 0529d65962e..008e631ce71 100644
--- a/src/backend/utils/adt/orderedsetaggs.c
+++ b/src/backend/utils/adt/orderedsetaggs.c
@@ -1569,6 +1569,17 @@ gp_percentile_cont_transition(FunctionCallInfo fcinfo,
{
return_state = lerpfunc(prev_state, val, proportion);
}
+ else if (PG_ARGISNULL(0))
+ {
+ /*
+ * Neither of the rows we are after landed in this peer group,
so we
+ * hand the previous state back unchanged. When that state is
NULL
+ * the isnull flag has to travel with it: returning a bare
Datum(0)
+ * as non-NULL gives wrong answers for by-value types and a NULL
+ * pointer dereference for by-reference ones.
+ */
+ fcinfo->isnull = true;
+ }
*cnt = *cnt + peer_count;
if(*cnt > total_rows)
@@ -1641,7 +1652,6 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS)
errmsg("percentile value %g is not between 0
and 1",
percentile)));
Datum prev_state = PG_GETARG_DATUM(0);
- bool prev_state_isnull = PG_ARGISNULL(0);
Datum val = PG_GETARG_DATUM(1);
Datum return_state = prev_state;
int64 total_rows = PG_GETARG_INT64(3);
@@ -1666,6 +1676,11 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS)
{
return_state = val;
}
+ else if (PG_ARGISNULL(0))
+ {
+ /* see gp_percentile_cont_transition() */
+ fcinfo->isnull = true;
+ }
*cnt = *cnt + peer_count;
@@ -1676,10 +1691,6 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS)
fcinfo->flinfo->fn_extra = NULL;
}
- if (return_state == prev_state) {
- fcinfo->isnull = prev_state_isnull;
- }
-
PG_RETURN_DATUM(return_state);
}
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 851e58debc3..c9c2fd83275 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -60,6 +60,6 @@
*/
/* 3yyymmddN */
-#define CATALOG_VERSION_NO 302606111
+#define CATALOG_VERSION_NO 302608261
#endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 0e0dcb0dc5d..509c2e3be2c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -12392,25 +12392,25 @@
proname => 'gp_percentile_cont_float8_transition',
proisstrict => 'f',
prorettype => 'float8',
- proargtypes => 'float8 float8 int8 int8',
+ proargtypes => 'float8 float8 float8 int8 int8',
prosrc => 'gp_percentile_cont_float8_transition' },
{ oid => 9185, descr => 'unordered percentile_cont interval transition
function',
proname => 'gp_percentile_cont_interval_transition',
proisstrict => 'f',
prorettype => 'interval',
- proargtypes => 'interval float8 int8 int8',
+ proargtypes => 'interval interval float8 int8 int8',
prosrc => 'gp_percentile_cont_interval_transition' },
{ oid => 9186, descr => 'unordered percentile_cont timestamp transition
function',
proname => 'gp_percentile_cont_timestamp_transition',
proisstrict => 'f',
prorettype => 'timestamp',
- proargtypes => 'timestamp float8 int8 int8',
+ proargtypes => 'timestamp timestamp float8 int8 int8',
prosrc => 'gp_percentile_cont_timestamp_transition' },
{ oid => 9187, descr => 'unordered percentile_cont timestamptz transition
function',
proname => 'gp_percentile_cont_timestamptz_transition',
proisstrict => 'f',
prorettype => 'timestamptz',
- proargtypes => 'timestamptz float8 int8 int8',
+ proargtypes => 'timestamptz timestamptz float8 int8 int8',
prosrc => 'gp_percentile_cont_timestamptz_transition' },
{ oid => 9188, descr => 'unordered percentile final function',
proname => 'gp_percentile_final',
@@ -12430,7 +12430,7 @@
proname => 'gp_percentile_disc_transition',
proisstrict => 'f',
prorettype => 'anyelement',
- proargtypes => 'anyelement float8 int8 int8',
+ proargtypes => 'anyelement anyelement float8 int8 int8',
prosrc => 'gp_percentile_disc_transition' },
{ oid => 9194, descr => 'unordered percentile discrete aggregate',
proname => 'gp_percentile_disc', prokind => 'a', proisstrict => 'f',
prorettype => 'anyelement', proargtypes => 'anyelement float8 int8 int8',
prosrc => 'aggregate_dummy' },
diff --git a/src/test/regress/expected/opr_sanity.out
b/src/test/regress/expected/opr_sanity.out
index 4cdd156d78e..6d91930ec19 100644
--- a/src/test/regress/expected/opr_sanity.out
+++ b/src/test/regress/expected/opr_sanity.out
@@ -1536,14 +1536,9 @@ WHERE a.aggfnoid = p.oid AND
-- we could carry the check further, but 4 args is enough for now
OR (p.pronargs > 4)
);
- aggfnoid | proname | oid | proname
-----------+--------------------+------+-------------------------------------------
- 9189 | gp_percentile_cont | 9184 | gp_percentile_cont_float8_transition
- 9190 | gp_percentile_cont | 9185 | gp_percentile_cont_interval_transition
- 9191 | gp_percentile_cont | 9186 | gp_percentile_cont_timestamp_transition
- 9192 | gp_percentile_cont | 9187 |
gp_percentile_cont_timestamptz_transition
- 9194 | gp_percentile_disc | 9193 | gp_percentile_disc_transition
-(5 rows)
+ aggfnoid | proname | oid | proname
+----------+---------+-----+---------
+(0 rows)
-- Cross-check finalfn (if present) against its entry in pg_proc.
SELECT a.aggfnoid::oid, p.proname, pfn.oid, pfn.proname
diff --git a/src/test/regress/expected/percentile.out
b/src/test/regress/expected/percentile.out
index fbdc37b763b..ae3d77026c0 100644
--- a/src/test/regress/expected/percentile.out
+++ b/src/test/regress/expected/percentile.out
@@ -876,6 +876,112 @@ group by d1, d2;
55 | 1
(1 row)
+--
+-- gp_percentile_cont()/gp_percentile_disc() are the split ordered-set
+-- aggregates that ORCA rewrites percentile_cont()/percentile_disc()/median()
+-- into. Their transition functions carry the running state on top of the
+-- four aggregate arguments, so they take five arguments, not four.
+--
+select gp_percentile_cont_float8_transition(NULL::float8, 1::float8, 1, 1, 1);
+ gp_percentile_cont_float8_transition
+--------------------------------------
+ 1
+(1 row)
+
+select gp_percentile_cont_interval_transition(NULL::interval, '1
hour'::interval, 1, 1, 1);
+ gp_percentile_cont_interval_transition
+----------------------------------------
+ @ 1 hour
+(1 row)
+
+select gp_percentile_cont_timestamp_transition(NULL::timestamp,
NULL::timestamp, 1, 1, 1);
+ gp_percentile_cont_timestamp_transition
+-----------------------------------------
+
+(1 row)
+
+select gp_percentile_cont_timestamptz_transition(NULL::timestamptz,
NULL::timestamptz, 1, 1, 1);
+ gp_percentile_cont_timestamptz_transition
+-------------------------------------------
+
+(1 row)
+
+select gp_percentile_disc_transition(NULL::numeric, 1::numeric, 1, 1, 1);
+ gp_percentile_disc_transition
+-------------------------------
+ 1
+(1 row)
+
+-- On an empty input set the transition state stays NULL. The transition
+-- functions hand that state back untouched and have to keep its isnull flag
+-- with it: a bare Datum(0) escaping here reads as a bogus value for by-value
+-- types and dereferences a NULL pointer for by-reference ones.
+select gp_percentile_cont(0::float8, 0, 0, 0);
+ gp_percentile_cont
+--------------------
+
+(1 row)
+
+select gp_percentile_cont('0 hour'::interval, 0, 0, 0);
+ gp_percentile_cont
+--------------------
+
+(1 row)
+
+select gp_percentile_cont('2006-01-01 13:10:13'::timestamp, 0, 0, 0);
+ gp_percentile_cont
+--------------------
+
+(1 row)
+
+select gp_percentile_cont('2006-01-01 13:10:13+00'::timestamptz, 0, 0, 0);
+ gp_percentile_cont
+--------------------
+
+(1 row)
+
+select gp_percentile_disc(0::numeric, 0, 0, 0);
+ gp_percentile_disc
+--------------------
+
+(1 row)
+
+-- A value that really was picked has to come back, even when its Datum
+-- representation happens to be 0.
+select gp_percentile_disc(0::float8, 0, 1, 1);
+ gp_percentile_disc
+--------------------
+ 0
+(1 row)
+
+select gp_percentile_disc(0::int, 0, 1, 1);
+ gp_percentile_disc
+--------------------
+ 0
+(1 row)
+
+select gp_percentile_cont(0::float8, 0, 1, 1);
+ gp_percentile_cont
+--------------------
+ 0
+(1 row)
+
+-- The same, end to end: the smallest value of b is 0.
+create table perczero (a int, b float8) distributed by (a);
+insert into perczero select i, (i - 1)::float8 from generate_series(1, 10) i;
+select percentile_disc(0) within group (order by b) from perczero;
+ percentile_disc
+-----------------
+ 0
+(1 row)
+
+select percentile_cont(0) within group (order by b) from perczero;
+ percentile_cont
+-----------------
+ 0
+(1 row)
+
+drop table perczero;
drop view percv2;
drop view percv;
drop table perct;
diff --git a/src/test/regress/sql/percentile.sql
b/src/test/regress/sql/percentile.sql
index bc7c327e770..61a7d9147cb 100644
--- a/src/test/regress/sql/percentile.sql
+++ b/src/test/regress/sql/percentile.sql
@@ -216,6 +216,37 @@ from mpp_22413
where d2 ='55'
group by d1, d2;
+--
+-- gp_percentile_cont()/gp_percentile_disc() are the split ordered-set
+-- aggregates that ORCA rewrites percentile_cont()/percentile_disc()/median()
+-- into. Their transition functions carry the running state on top of the
+-- four aggregate arguments, so they take five arguments, not four.
+--
+select gp_percentile_cont_float8_transition(NULL::float8, 1::float8, 1, 1, 1);
+select gp_percentile_cont_interval_transition(NULL::interval, '1
hour'::interval, 1, 1, 1);
+select gp_percentile_cont_timestamp_transition(NULL::timestamp,
NULL::timestamp, 1, 1, 1);
+select gp_percentile_cont_timestamptz_transition(NULL::timestamptz,
NULL::timestamptz, 1, 1, 1);
+select gp_percentile_disc_transition(NULL::numeric, 1::numeric, 1, 1, 1);
+-- On an empty input set the transition state stays NULL. The transition
+-- functions hand that state back untouched and have to keep its isnull flag
+-- with it: a bare Datum(0) escaping here reads as a bogus value for by-value
+-- types and dereferences a NULL pointer for by-reference ones.
+select gp_percentile_cont(0::float8, 0, 0, 0);
+select gp_percentile_cont('0 hour'::interval, 0, 0, 0);
+select gp_percentile_cont('2006-01-01 13:10:13'::timestamp, 0, 0, 0);
+select gp_percentile_cont('2006-01-01 13:10:13+00'::timestamptz, 0, 0, 0);
+select gp_percentile_disc(0::numeric, 0, 0, 0);
+-- A value that really was picked has to come back, even when its Datum
+-- representation happens to be 0.
+select gp_percentile_disc(0::float8, 0, 1, 1);
+select gp_percentile_disc(0::int, 0, 1, 1);
+select gp_percentile_cont(0::float8, 0, 1, 1);
+-- The same, end to end: the smallest value of b is 0.
+create table perczero (a int, b float8) distributed by (a);
+insert into perczero select i, (i - 1)::float8 from generate_series(1, 10) i;
+select percentile_disc(0) within group (order by b) from perczero;
+select percentile_cont(0) within group (order by b) from perczero;
+drop table perczero;
drop view percv2;
drop view percv;
drop table perct;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
