Skip to content

Fix two crashes/wrong-answer bugs in the gp_percentile_* transition functions (in REL_2_STABLE) #1935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Alena0704 wants to merge 2 commits into
base: REL_2_STABLE
Choose a base branch
from
+166 −5
Open

Fix two crashes/wrong-answer bugs in the gp_percentile_* transition functions (in REL_2_STABLE) #1935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db40deb
Reject direct calls to the gp_percentile transition functions
Alena0704 Aug 26, 2026
3d386da
Keep the isnull flag when gp_percentile_* returns its previous state
Alena0704 Aug 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 38 additions & 5 deletions src/backend/utils/adt/orderedsetaggs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1516,6 +1516,17 @@ gp_percentile_cont_transition(FunctionCallInfo fcinfo,
int64 first_row;
int64 second_row;

/*
* Note: 'proargtypes' for this function in pg_proc.dat has 4 arguments.
* There are actually 5 arguments coming in here - the result of the
* previous call and 4 main arguments.
*/
if (PG_NARGS() != 5)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("wrong number of arguments to gp_percentile_cont_transition()"),
errhint("expected 5, got %d", PG_NARGS())));

/* Return state for NULL inputs of val*/
if (PG_ARGISNULL(1) && !PG_ARGISNULL(0))
PG_RETURN_DATUM(PG_GETARG_DATUM(0));
Expand Down Expand Up @@ -1562,6 +1573,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)
Expand Down Expand Up @@ -1619,6 +1641,17 @@ gp_percentile_disc_transition(PG_FUNCTION_ARGS)
{
int64 rownum;

/*
* Note: 'proargtypes' for this function in pg_proc.dat has 4 arguments.
* There are actually 5 arguments coming in here - the result of the
* previous call and 4 main arguments.
*/
if (PG_NARGS() != 5)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("wrong number of arguments to gp_percentile_disc_transition()"),
errhint("expected 5, got %d", PG_NARGS())));

/* Return state for NULL inputs of val*/
if (PG_ARGISNULL(1) && !PG_ARGISNULL(0))
PG_RETURN_DATUM(PG_GETARG_DATUM(0));
Expand All @@ -1634,7 +1667,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);
Expand All @@ -1659,6 +1691,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;

Expand All @@ -1669,10 +1706,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);
}

Expand Down
94 changes: 94 additions & 0 deletions src/test/regress/expected/percentile.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,100 @@ 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 read five arguments, while pg_proc.dat
-- describes four. That is left alone here so as not to force an initdb on a
-- stable branch; instead a direct call, which would read past the end of the
-- argument array, is rejected.
--
select gp_percentile_cont_float8_transition(NULL::float8, 1, 1, 1);
ERROR: wrong number of arguments to gp_percentile_cont_transition()
HINT: expected 5, got 4
select gp_percentile_cont_interval_transition(NULL::interval, 1, 1, 1);
ERROR: wrong number of arguments to gp_percentile_cont_transition()
HINT: expected 5, got 4
select gp_percentile_cont_timestamp_transition(NULL::timestamp, 1, 1, 1);
ERROR: wrong number of arguments to gp_percentile_cont_transition()
HINT: expected 5, got 4
select gp_percentile_cont_timestamptz_transition(NULL::timestamptz, 1, 1, 1);
ERROR: wrong number of arguments to gp_percentile_cont_transition()
HINT: expected 5, got 4
select gp_percentile_disc_transition(NULL::numeric, 1, 1, 1);
ERROR: wrong number of arguments to gp_percentile_disc_transition()
HINT: expected 5, got 4
-- 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;
Expand Down
34 changes: 34 additions & 0 deletions src/test/regress/sql/percentile.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,40 @@ 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 read five arguments, while pg_proc.dat
-- describes four. That is left alone here so as not to force an initdb on a
-- stable branch; instead a direct call, which would read past the end of the
-- argument array, is rejected.
--
select gp_percentile_cont_float8_transition(NULL::float8, 1, 1, 1);
select gp_percentile_cont_interval_transition(NULL::interval, 1, 1, 1);
select gp_percentile_cont_timestamp_transition(NULL::timestamp, 1, 1, 1);
select gp_percentile_cont_timestamptz_transition(NULL::timestamptz, 1, 1, 1);
select gp_percentile_disc_transition(NULL::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;
Expand Down
Loading