Re: Statistical aggregate functions are not working with PARTIALaggregation

At Tue, 07 May 2019 20:47:28 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20190507.204728.233299873.horiguchi.kyotaro@lab.ntt.co.jp>
> Hello.
> 
> At Tue, 7 May 2019 14:39:55 +0530, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com> wrote in
<CAKcux6=YBMCntcafSs_22dS1ab6mGay_QUaHx-nvg+_FVPMg3Q@mail.gmail.com>
> > Hi,
> > As this issue is reproducible without partition-wise aggregate also,
> > changing email subject from "Statistical aggregate functions are not
> > working with partitionwise aggregate " to "Statistical aggregate functions
> > are not working with PARTIAL aggregation".
> > 
> > original reported test case and discussion can be found at below link.
> > https://www.postgresql.org/message-id/flat/CAKcux6%3DuZEyWyLw0N7HtR9OBc-sWEFeByEZC7t-KDf15FKxVew%40mail.gmail.com
> 
> The immediate reason for the behavior seems that
> EEOP_AGG_STRICT_INPUT_CHECK_ARGS considers non existent second
> argument as null, which is out of arguments list in
> trans_fcinfo->args[].
> 
> The invalid deserialfn_oid case in ExecBuildAggTrans, it
> initializes args[1] using the second argument of the functoin
> (int8pl() in the case) so the correct numTransInputs here is 1,
> not 2.
> 
> I don't understand this fully but at least the attached patch
> makes the test case work correctly and this seems to be the only
> case of this issue.

This behavior is introduced by 69c3936a14 (in v11).  At that time
FunctionCallInfoData is pallioc0'ed and has fixed length members
arg[6] and argnull[7]. So nulls[1] is always false even if nargs
= 1 so the issue had not been revealed.

After introducing a9c35cf85c (in v12) the same check is done on
FunctionCallInfoData that has NullableDatum args[] of required
number of elements. In that case args[1] is out of palloc'ed
memory so this issue has been revealed.

In a second look, I seems to me that the right thing to do here
is setting numInputs instaed of numArguments to numTransInputs in
combining step.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
From c5dcbc32646e8e4865307bb0525a143da573e240 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horiguchi.kyotaro@lab.ntt.co.jp>
Date: Wed, 8 May 2019 13:01:01 +0900
Subject: [PATCH] Give correct number of arguments to combine function.

Combine function's first parameter is used as transition value. The
correct number of transition input for the function is not the number
of arguments but of transition input.
---
 src/backend/executor/nodeAgg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index d01fc4f52e..e13a8cb304 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2912,7 +2912,8 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
     pertrans->aggtranstype = aggtranstype;
 
     /* Detect how many arguments to pass to the transfn */
-    if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
+    if (AGGKIND_IS_ORDERED_SET(aggref->aggkind) ||
+        DO_AGGSPLIT_COMBINE(aggstate->aggsplit))
         pertrans->numTransInputs = numInputs;
     else
         pertrans->numTransInputs = numArguments;
-- 
2.16.3