Re: Partial aggregates pushdown - Mailing list pgsql-hackers

Fujii.Yuki@df.MitsubishiElectric.co.jp писал 2022-11-22 04:01:
> Hi Mr.Vondra, Mr.Pyhalov, Everyone.
> 
> I discussed with Mr.Pyhalov about the above draft by directly sending 
> mail to
>  him(outside of pgsql-hackers). Mr.Pyhalov allowed me to update his 
> patch
> along with the above draft. So I update Mr.Pyhalov's patch v10.
> 

Hi, Yuki. Thank you for your work on this.

I've looked through the patch. Overall I like this approach, but have 
the following comments.

1) Why should we require partialaggfn for min()/max()/count()? We could 
just use original functions for a lot of aggregates, and so it would be 
possible to push down some partial aggregates to older servers. I'm not 
sure that it's a strict requirement, but a nice thing to think about. 
Can we use the function itself as partialaggfn, for example, for 
sum(int4)? For functions with internal aggtranstype (like sum(int8) it 
would be more difficult).

2) fpinfo->server_version is not aggregated, for example, when we form 
fpinfo in foreign_join_ok(), it seems we should spread it in more places 
in postgres_fdw.c.

3) In add_foreign_grouping_paths() it seems there's no need for 
additional argument, we can look at extra->patype. Also Assert() in 
add_foreign_grouping_paths() will fire in --enable-cassert build.

4) Why do you modify lookup_agg_function() signature? I don't see tests, 
showing that it's neccessary. Perhaps, more precise function naming 
should be used instead?

5) In tests:
          - Why version_num does have "name" type in 
f_alter_server_version() function?
          - You modify server_version option of 'loopback' server, but 
don't reset it after test. This could affect further tests.
          - "It's unsafe to push down partial aggregates with distinct" 
in postgres_fdw.sql:3002 seems to be misleading.
3001
3002 -- It's unsafe to push down partial aggregates with distinct
3003 SELECT f_alter_server_version('loopback', 'set', -1);
3004 EXPLAIN (VERBOSE, COSTS OFF)
3005 SELECT avg(d) FROM pagg_tab;
3006 SELECT avg(d) FROM pagg_tab;
3007 select * from pg_foreign_server;

6) While looking at it, could cause a crash with something like

CREATE TYPE COMPLEX AS (re FLOAT, im FLOAT);

CREATE OR REPLACE FUNCTION
sum_complex (sum complex, el complex)
RETURNS complex AS
$$
DECLARE
s complex;
BEGIN
if el is not null and sum is not null then
sum.re:=coalesce(sum.re,0)+el.re;
sum.im:=coalesce(sum.im,0)+el.im;
end if;
RETURN sum;
END;
$$ LANGUAGE plpgSQL;

CREATE AGGREGATE SUM(COMPLEX) (
SFUNC=sum_complex,
STYPE=complex,
partialaggfunc=aaaa,
partialagg_minversion=1400
);

where aaaa - something nonexisting


enforce_generic_type_consistency (actual_arg_types=0x56269873d200, 
declared_arg_types=0x0, nargs=1, rettype=0, allow_poly=true) at 
parse_coerce.c:2132
2132                    Oid                     decl_type = 
declared_arg_types[j];
(gdb) bt
#0  enforce_generic_type_consistency (actual_arg_types=0x56269873d200, 
declared_arg_types=0x0, nargs=1, rettype=0, allow_poly=true) at 
parse_coerce.c:2132
#1  0x00005626960072de in lookup_agg_function (fnName=0x5626986715a0, 
nargs=1, input_types=0x56269873d200, variadicArgType=0, 
rettype=0x7ffd1a4045d8, only_normal=false) at pg_aggregate.c:916
#2  0x00005626960064ba in AggregateCreate (aggName=0x562698671000 "sum", 
aggNamespace=2200, replace=false, aggKind=110 'n', numArgs=1, 
numDirectArgs=0, parameterTypes=0x56269873d1e8, allParameterTypes=0, 
parameterModes=0,
     parameterNames=0, parameterDefaults=0x0, variadicArgType=0, 
aggtransfnName=0x5626986712c0, aggfinalfnName=0x0, aggcombinefnName=0x0, 
aggserialfnName=0x0, aggdeserialfnName=0x0, aggmtransfnName=0x0, 
aggminvtransfnName=0x0,
     aggmfinalfnName=0x0, partialaggfnName=0x5626986715a0, 
finalfnExtraArgs=false, mfinalfnExtraArgs=false, finalfnModify=114 'r', 
mfinalfnModify=114 'r', aggsortopName=0x0, aggTransType=16390, 
aggTransSpace=0, aggmTransType=0,
     aggmTransSpace=0, partialaggMinversion=1400, agginitval=0x0, 
aggminitval=0x0, proparallel=117 'u') at pg_aggregate.c:582
#3  0x00005626960a1e1c in DefineAggregate (pstate=0x56269869ab48, 
name=0x562698671038, args=0x5626986711b0, oldstyle=false, 
parameters=0x5626986713b0, replace=false) at aggregatecmds.c:450
#4  0x000056269643061f in ProcessUtilitySlow (pstate=0x56269869ab48, 
pstmt=0x562698671a68,
     queryString=0x5626986705d8 "CREATE AGGREGATE SUM(COMPLEX) 
(\nSFUNC=sum_complex,\nSTYPE=COMPLEX,\npartialaggfunc=scomplex,\npartialagg_minversion=1400\n);", 
context=PROCESS_UTILITY_TOPLEVEL, params=0x0, queryEnv=0x0,
     dest=0x562698671b48, qc=0x7ffd1a4053c0) at utility.c:1407
#5  0x000056269642fbb4 in standard_ProcessUtility (pstmt=0x562698671a68, 
queryString=0x5626986705d8 "CREATE AGGREGATE SUM(COMPLEX) 
(\nSFUNC=sum_complex,\nSTYPE=COMPLEX,\npartialaggfunc=scomplex,\npartialagg_minversion=1400\n);",
     readOnlyTree=false, context=PROCESS_UTILITY_TOPLEVEL, params=0x0, 
queryEnv=0x0, dest=0x562698671b48, qc=0x7ffd1a4053c0) at utility.c:1074


Later will look at it again.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional