Hi Antonin, To understand the feature you have proposed, I have tried understanding your patch. Here are few comments so far on it:
1. + if (aggref->aggvariadic || + aggref->aggdirectargs || aggref->aggorder || + aggref->aggdistinct || aggref->aggfilter) I did not understand, why you are not pushing aggregate in above cases? Can you please explain? 2. "make check" in contrib/postgres_fdw crashes. SELECT COUNT(*) FROM ft1 t1; ! server closed the connection unexpectedly ! This probably means the server terminated abnormally ! before or while processing the request. ! connection to server was lost >From your given setup, if I wrote a query like: EXPLAIN VERBOSE SELECT count(*) FROM orders_0; it crashes. Seems like missing few checks. 3. In case of pushing partial aggregation on the remote side, you use schema named "partial", I did not get that change. If I have AVG() aggregate, then I end up getting an error saying "ERROR: schema "partial" does not exist". 4. I am not sure about the code changes related to pushing partial aggregate on the remote side. Basically, you are pushing a whole aggregate on the remote side and result of that is treated as partial on the basis of aggtype = transtype. But I am not sure that is correct unless I miss something here. The type may be same but the result may not. 5. There are lot many TODO comments in the patch-set, are you working on those? Thanks On Thu, Aug 17, 2017 at 8:52 PM, Antonin Houska <a...@cybertec.at> wrote: > Antonin Houska <a...@cybertec.at> wrote: > > > Antonin Houska <a...@cybertec.at> wrote: > > > > > This is a new version of the patch I presented in [1]. > > > > Rebased. > > > > cat .git/refs/heads/master > > b9a3ef55b253d885081c2d0e9dc45802cab71c7b > > This is another version of the patch. > > Besides other changes, it enables the aggregation push-down for > postgres_fdw, > although only for aggregates whose transient aggregation state is equal to > the > output type. For other aggregates (like avg()) the remote nodes will have > to > return the transient state value in an appropriate form (maybe bytea type), > which does not depend on PG version. > > shard.tgz demonstrates the typical postgres_fdw use case. One query shows > base > scans of base relation's partitions being pushed to shard nodes, the other > pushes down a join and performs aggregation of the join result on the > remote > node. Of course, the query can only references one particular partition, > until > the "partition-wise join" [1] patch gets committed and merged with this my > patch. > > One thing I'm not sure about is whether the patch should remove > GetForeignUpperPaths function from FdwRoutine, which it essentially makes > obsolete. Or should it only be deprecated so far? I understand that > deprecation is important for "ordinary SQL users", but FdwRoutine is an > interface for extension developers, so the policy might be different. > > [1] https://commitfest.postgresql.org/14/994/ > > Any feedback is appreciated. > > -- > Antonin Houska > Cybertec Schönig & Schönig GmbH > Gröhrmühlgasse 26 > A-2700 Wiener Neustadt > Web: http://www.postgresql-support.de, http://www.cybertec.at > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -- Jeevan Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
