• Andres Freund's avatar
    Minimally fix partial aggregation for aggregates that don't have one argument. · 26572832
    Andres Freund authored
    For partial aggregation combine steps,
    AggStatePerTrans->numTransInputs was set to the transition function's
    number of inputs, rather than the combine function's number of
    inputs (always 1).
    
    That lead to partial aggregates with strict combine functions to
    wrongly check for NOT NULL input as required by strictness. When the
    aggregate wasn't exactly passed one argument, the strictness check was
    either omitted (in the 0 args case) or too many arguments were
    checked. In the latter case we'd read beyond the end of
    FunctionCallInfoData->args (only in master).
    
    AggStatePerTrans->numTransInputs actually has been wrong since since
    9.6, where partial aggregates were added. But it turns out to not be
    an active problem in 9.6 and 10, because numTransInputs wasn't used at
    all for combine functions: Before c253b722f6 there simply was no NULL
    check for the input to strict trans functions, and after that the
    check was simply hardcoded for the right offset in fcinfo, as it's
    done by code specific to combine functions.
    
    In bf6c614a (11) the strictness check was generalized, with common
    code doing the strictness checks for both plain and combine transition
    functions, based on numTransInputs. For combine functions this lead to
    not emitting an expression step to check for strict input in the 0
    arguments case, and in the > 1 arguments case, we'd check too many
    arguments.Due to the fact that the relevant fcinfo->isnull[2..] was
    always zero-initialized (more or less by accident, by being part of
    the AggStatePerTrans struct, which is palloc0'ed), there was no
    observable damage in the latter case before a9c35cf8, we just
    checked too many array elements.
    
    Due to the changes in a9c35cf8, > 1 argument bug became visible,
    because these days fcinfo is a) dynamically allocated without being
    zeroed b) exactly the length required for the number of specified
    arguments (hardcoded to 2 in this case).
    
    This commit only contains a fairly minimal fix, setting numTransInputs
    to a hardcoded 1 when building a pertrans for a combine function. It
    seems likely that we'll want to clean this up further (e.g. the
    arguments build_pertrans_for_aggref() aren't particularly meaningful
    for combine functions). But the wrap date for 12 beta1 is coming up
    fast, so it seems good to have a minimal fix in place.
    
    Backpatch to 11. While AggStatePerTrans->numTransInputs was set
    wrongly before that, the value was not used for combine functions.
    
    Reported-By: Rajkumar Raghuwanshi
    Diagnosed-By: Kyotaro Horiguchi, Jeevan Chalke, Andres Freund, David Rowley
    Author: David Rowley, Kyotaro Horiguchi, Andres Freund
    Discussion: https://postgr.es/m/CAKcux6=uZEyWyLw0N7HtR9OBc-sWEFeByEZC7t-KDf15FKxVew@mail.gmail.com
    26572832
aggregates.sql 31.1 KB