Fix two crashes/wrong-answer bugs in the gp_percentile_* transition functions (in REL_2_STABLE) - #1935
Open
Alena0704 wants to merge 2 commits into
Open
Fix two crashes/wrong-answer bugs in the gp_percentile_* transition functions (in REL_2_STABLE)#1935Alena0704 wants to merge 2 commits into
Alena0704 wants to merge 2 commits into
Conversation
Alena0704
force-pushed
the
gp-percentile-rel2
branch
from
August 27, 2026 16:04
8251c47 to
fcf4d74
Compare
leborchuk
approved these changes
Aug 28, 2026
leborchuk
left a comment
Contributor
There was a problem hiding this comment.
LGTM, we should just fix it )
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 <g.shelkovy@arenadata.io>
Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770)
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 <g.shelkovy@arenadata.io> Ported from Greengage/open-gpdb commit 477b04a (ADBDEV-7770).
Alena0704
force-pushed
the
gp-percentile-rel2
branch
from
August 28, 2026 11:01
fcf4d74 to
3d386da
Compare
Contributor
Author
|
Fixed commit messages a bit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gp_percentile_cont() / gp_percentile_disc() are the split ordered-set
aggregates ORCA rewrites percentile_cont(), percentile_disc() and median()
into. Two bugs in their transition functions. Only the ORCA path reaches
these aggregates on its own, so percentile_* WITHIN GROUP on the Postgres
planner is unaffected.
The transition functions read five arguments in C (state + the four
aggregate arguments), but pg_proc.dat declares four. As transition
functions they are called correctly; a direct SQL call makes
PG_GETARG_INT64(4) pick up garbage — wrong results, an assertion on
pfree(NULL), or a segfault.
Fixing the declaration would force an initdb, which is not acceptable on a
stable branch, so the argument count is checked and a plain error is
raised. Direct calls were never useful.
When the current row isn't one the percentile is computed from, the
previous state is handed back untouched. On the first call that state is
NULL, and returning a bare Datum(0) with isnull false drops the flag: 0
instead of NULL for by-value types, a NULL pointer dereference for
by-reference ones — an empty input set crashed the backend for interval,
timestamp and timestamptz.
Both ported from Greengage 477b04a (ADBDEV-7770)
The bug reproduction:
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
CI Skip Instructions