From db40debe3aa965ad5442277a7e8553fec18f2198 Mon Sep 17 00:00:00 2001 From: Alena Rybakina Date: Wed, 26 Aug 2026 13:05:23 +0300 Subject: [PATCH 1/2] Reject direct calls to 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, declares them with four. As transition functions they are called correctly, 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. Correcting the declaration would change the catalog and force an initdb, which is not acceptable on a stable branch, so check the argument count instead and raise a plain error. Direct calls were never useful - the functions only make sense as the transition step of their aggregates - and 'percentile_* WITHIN GROUP' queries are unaffected either way. Co-authored-by: Georgy Shelkovy Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770) --- src/backend/utils/adt/orderedsetaggs.c | 22 ++++++++++++++++++++++ src/test/regress/expected/percentile.out | 24 ++++++++++++++++++++++++ src/test/regress/sql/percentile.sql | 14 ++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c index 46b2694d89e..efcadadce05 100644 --- a/src/backend/utils/adt/orderedsetaggs.c +++ b/src/backend/utils/adt/orderedsetaggs.c @@ -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)); @@ -1619,6 +1630,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)); diff --git a/src/test/regress/expected/percentile.out b/src/test/regress/expected/percentile.out index c1f4ec69da1..1589d50d8b6 100644 --- a/src/test/regress/expected/percentile.out +++ b/src/test/regress/expected/percentile.out @@ -876,6 +876,30 @@ 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 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..b7b622424e7 100644 --- a/src/test/regress/sql/percentile.sql +++ b/src/test/regress/sql/percentile.sql @@ -216,6 +216,20 @@ 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); drop view percv2; drop view percv; drop table perct; From 3d386dab357646a296678f357152d5436707dc81 Mon Sep 17 00:00:00 2001 From: Alena Rybakina Date: Wed, 26 Aug 2026 13:06:01 +0300 Subject: [PATCH 2/2] 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 Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770). --- src/backend/utils/adt/orderedsetaggs.c | 21 +++++-- src/test/regress/expected/percentile.out | 70 ++++++++++++++++++++++++ src/test/regress/sql/percentile.sql | 20 +++++++ 3 files changed, 106 insertions(+), 5 deletions(-) diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c index efcadadce05..32002dca684 100644 --- a/src/backend/utils/adt/orderedsetaggs.c +++ b/src/backend/utils/adt/orderedsetaggs.c @@ -1573,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) @@ -1656,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); @@ -1681,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; @@ -1691,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); } diff --git a/src/test/regress/expected/percentile.out b/src/test/regress/expected/percentile.out index 1589d50d8b6..563a4864995 100644 --- a/src/test/regress/expected/percentile.out +++ b/src/test/regress/expected/percentile.out @@ -900,6 +900,76 @@ 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; diff --git a/src/test/regress/sql/percentile.sql b/src/test/regress/sql/percentile.sql index b7b622424e7..dde74fc29f5 100644 --- a/src/test/regress/sql/percentile.sql +++ b/src/test/regress/sql/percentile.sql @@ -230,6 +230,26 @@ 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;