Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-24 Thread Jeevan Chalke
On Sat, Oct 22, 2016 at 9:09 PM, Tom Lane  wrote:

> brolga is still not terribly happy with this patch: it's choosing not to
> push down the aggregates in one of the queries.  While I failed to
> duplicate that result locally, investigation suggests that brolga's result
> is perfectly sane; in fact it's not very clear why we're not getting that
> from multiple critters, because the plan brolga is choosing is not
> inferior to the expected one.
>
> The core of the problem is this subquery:
>
> contrib_regression=# explain verbose select min(13), avg(ft1.c1),
> sum(ft2.c1) from ft1 right join ft2 on (ft1.c1 = ft2.c1) where ft1.c1 = 12;
>
>  QUERY PLAN
> 
> 
> -
>  Foreign Scan  (cost=108.61..108.64 rows=1 width=44)
>Output: (min(13)), (avg(ft1.c1)), (sum(ft2.c1))
>Relations: Aggregate on ((public.ft1) INNER JOIN (public.ft2))
>Remote SQL: SELECT min(13), avg(r1."C 1"), sum(r2."C 1") FROM ("S 1"."T
> 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r2."C 1" = 12)) AND ((r1."C 1" =
> 12
> (4 rows)
>
> If you look at the estimate to just fetch the data, it's:
>
> contrib_regression=# explain verbose select ft1.c1, ft2.c1 from ft1 right
> join ft2 on (ft1.c1 = ft2.c1) where ft1.c1 = 12;
>   QUERY PLAN
> 
> --
>  Foreign Scan  (cost=100.55..108.62 rows=1 width=8)
>Output: ft1.c1, ft2.c1
>Relations: (public.ft1) INNER JOIN (public.ft2)
>Remote SQL: SELECT r1."C 1", r2."C 1" FROM ("S 1"."T 1" r1 INNER JOIN
> "S 1"."T 1" r2 ON (((r2."C 1" = 12)) AND ((r1."C 1" = 12
> (4 rows)
>
> Note we're expecting only one row out of the join.  Now the cost of doing
> three aggregates on a single row of input is not a lot.  Comparing these
> local queries:
>
> regression=# explain select min(13),avg(q1),sum(q2) from int8_tbl where
> q2=456;
>   QUERY PLAN
> ---
>  Aggregate  (cost=1.07..1.08 rows=1 width=68)
>->  Seq Scan on int8_tbl  (cost=0.00..1.06 rows=1 width=16)
>  Filter: (q2 = 456)
> (3 rows)
>
> regression=# explain select (q1),(q2) from int8_tbl where q2=456;
>QUERY PLAN
> -
>  Seq Scan on int8_tbl  (cost=0.00..1.06 rows=1 width=16)
>Filter: (q2 = 456)
> (2 rows)
>
> we seem to have startup = input cost + .01 and then another .01
> for total.  So the estimate to do the above remote scan and then
> aggregate locally should have been 108.63 startup and 108.64 total,
> give or take.  The estimate for aggregating remotely is a hair better,
> but it's not nearly better enough to guarantee that the planner won't
> see it as fuzzily the same cost.
>
> In short: the problem with this test case is that it's considering
> aggregation over only a single row, which is a situation in which
> pushing down the aggregate actually doesn't save us anything, because
> we're retrieving one row from the remote either way.  So it's not at all
> surprising that we don't get a stable plan choice.  The test query needs
> to be adjusted so that the aggregation is done over multiple rows,
> allowing fdw_tuple_cost to kick in and provide some daylight between
> the cost estimates.
>

Attached patch which performs aggrgation over 1000 rows as suggested by Tom.

I believe it will have a stable output/plan now.

Thanks


>
> regards, tom lane
>



-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


agg_push_down_fix_testcase.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-22 Thread Tom Lane
brolga is still not terribly happy with this patch: it's choosing not to
push down the aggregates in one of the queries.  While I failed to
duplicate that result locally, investigation suggests that brolga's result
is perfectly sane; in fact it's not very clear why we're not getting that
from multiple critters, because the plan brolga is choosing is not
inferior to the expected one.

The core of the problem is this subquery:

contrib_regression=# explain verbose select min(13), avg(ft1.c1), sum(ft2.c1) 
from ft1 right join ft2 on (ft1.c1 = ft2.c1) where ft1.c1 = 12;
   QUERY 
PLAN
-
 Foreign Scan  (cost=108.61..108.64 rows=1 width=44)
   Output: (min(13)), (avg(ft1.c1)), (sum(ft2.c1))
   Relations: Aggregate on ((public.ft1) INNER JOIN (public.ft2))
   Remote SQL: SELECT min(13), avg(r1."C 1"), sum(r2."C 1") FROM ("S 1"."T 1" 
r1 INNER JOIN "S 1"."T 1" r2 ON (((r2."C 1" = 12)) AND ((r1."C 1" = 12
(4 rows)

If you look at the estimate to just fetch the data, it's:

contrib_regression=# explain verbose select ft1.c1, ft2.c1 from ft1 right join 
ft2 on (ft1.c1 = ft2.c1) where ft1.c1 = 12; 
  QUERY PLAN
  
--
 Foreign Scan  (cost=100.55..108.62 rows=1 width=8)
   Output: ft1.c1, ft2.c1
   Relations: (public.ft1) INNER JOIN (public.ft2)
   Remote SQL: SELECT r1."C 1", r2."C 1" FROM ("S 1"."T 1" r1 INNER JOIN "S 
1"."T 1" r2 ON (((r2."C 1" = 12)) AND ((r1."C 1" = 12
(4 rows)

Note we're expecting only one row out of the join.  Now the cost of doing
three aggregates on a single row of input is not a lot.  Comparing these
local queries:

regression=# explain select min(13),avg(q1),sum(q2) from int8_tbl where q2=456;
  QUERY PLAN   
---
 Aggregate  (cost=1.07..1.08 rows=1 width=68)
   ->  Seq Scan on int8_tbl  (cost=0.00..1.06 rows=1 width=16)
 Filter: (q2 = 456)
(3 rows)

regression=# explain select (q1),(q2) from int8_tbl where q2=456;
   QUERY PLAN
-
 Seq Scan on int8_tbl  (cost=0.00..1.06 rows=1 width=16)
   Filter: (q2 = 456)
(2 rows)

we seem to have startup = input cost + .01 and then another .01
for total.  So the estimate to do the above remote scan and then
aggregate locally should have been 108.63 startup and 108.64 total,
give or take.  The estimate for aggregating remotely is a hair better,
but it's not nearly better enough to guarantee that the planner won't
see it as fuzzily the same cost.

In short: the problem with this test case is that it's considering
aggregation over only a single row, which is a situation in which
pushing down the aggregate actually doesn't save us anything, because
we're retrieving one row from the remote either way.  So it's not at all
surprising that we don't get a stable plan choice.  The test query needs
to be adjusted so that the aggregation is done over multiple rows,
allowing fdw_tuple_cost to kick in and provide some daylight between
the cost estimates.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-22 Thread Ashutosh Bapat
On Fri, Oct 21, 2016 at 9:09 PM, Robert Haas  wrote:
> On Fri, Oct 21, 2016 at 11:20 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane  wrote:
 dromedary seems to have found one, or at least an unstable test result.
>>
>>> OK, looking at that now.  Thanks.
>>
>> Looking at further failures, it looks like 32-bit machines in general
>> get that result.  Might be just a cost estimation difference.
>>
>> Also, some of the windows machines are folding "sqrt(2)" to a different
>> constant than is hard-wired into the expected-result file.  That's
>> slightly annoying because it calls into question whether we can ship
>> floating-point computations to the far end at all :-(.
>
> IMHO, not shipping floating-point computations for that reason would
> be more annoying than helpful.

+1

> To really guarantee that the remote
> and identical results are identical, you'd need far more
> infrastructure than we have - you'd have to make sure the operating
> system collations matched, for example.  And we're already assuming
> (without any proof) that the default collations match and, for that
> matter, that the datatypes match.  If you're pushing down to
> PostgreSQL on the other end, you at least have a hope that the other
> side might be sufficiently identical to the local side that the
> results will be the same, but if somebody implements these pushdowns
> for Oracle or SQL server, you almost might as well give up and go
> home.  Even for PostgreSQL, getting the same results in every possible
> corner case requires a certain degree of optimism.
>
> For my money, the way to handle that is to add more control over
> pushdown rather than automatically disabling it in certain cases.  For
> example, we could have GUCs that disable all pushdown or certain types
> of pushdown - e.g. you could specify that you don't trust the remote
> side to sort data properly, or that you don't like it's floating-point
> implementation.  I'm not sure what controls are most useful here, but
> I'd be willing to bet you a nice glass of white wine that many people
> will be happier with a system that they can control than they will be
> with one that just disables the optimization in every case where it
> might hypothetically not work out.  It's not clear that we can even
> reasonably foresee all such cases.

+1. But having too many of them will make the customer go crazy.
Everytime s/he stumbles upon such a problem (or bug from her/his point
of view), we will add a GUC/option OR tell, "oh! you have to turn
on/off that GUC/option". That will just make the users loose faith in
FDW (or at least postgres_fdw), if we end up with too many of them.

Instead, we may want to add the intelligence to detect when floating
point calculations on the foreign server are different from local one
OR that the default locale or collation on the foreign server are
different from the local one and accordingly set the foreign server
options automatically. This may be part of the postgres_fdw or a
separate utility. I guess, this will be similar to what configure does
to detect compatible libraries. The information gathered will
certainly become obsolete if the user re-configures the server or
changes settings, but at least we can advice them to run the utility
to detect any inconsistencies caused by changes and report or correct
them. I think this is a bigger project, separate from any FDW
pushdown.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-22 Thread Ashutosh Bapat
On Fri, Oct 21, 2016 at 8:50 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane  wrote:
>>> dromedary seems to have found one, or at least an unstable test result.
>
>> OK, looking at that now.  Thanks.
>
> Looking at further failures, it looks like 32-bit machines in general
> get that result.  Might be just a cost estimation difference.

How do I or Jeevan get to look at those results?

>
> Also, some of the windows machines are folding "sqrt(2)" to a different
> constant than is hard-wired into the expected-result file.  That's
> slightly annoying because it calls into question whether we can ship
> floating-point computations to the far end at all :-(.
>

There are two things where the floating point push-down can go wrong:
a. the floating point calculation on the foreign server itself is
different from the local server, b. while converting the binary
floating point representation into text, the actual number gets
truncated because of limit on the number of digits in the text
representation. The later can be avoided by using binary data transfer
mode for floating point number. Is that something worth considering?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-21 Thread Robert Haas
On Fri, Oct 21, 2016 at 11:20 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane  wrote:
>>> dromedary seems to have found one, or at least an unstable test result.
>
>> OK, looking at that now.  Thanks.
>
> Looking at further failures, it looks like 32-bit machines in general
> get that result.  Might be just a cost estimation difference.
>
> Also, some of the windows machines are folding "sqrt(2)" to a different
> constant than is hard-wired into the expected-result file.  That's
> slightly annoying because it calls into question whether we can ship
> floating-point computations to the far end at all :-(.

IMHO, not shipping floating-point computations for that reason would
be more annoying than helpful.  To really guarantee that the remote
and identical results are identical, you'd need far more
infrastructure than we have - you'd have to make sure the operating
system collations matched, for example.  And we're already assuming
(without any proof) that the default collations match and, for that
matter, that the datatypes match.  If you're pushing down to
PostgreSQL on the other end, you at least have a hope that the other
side might be sufficiently identical to the local side that the
results will be the same, but if somebody implements these pushdowns
for Oracle or SQL server, you almost might as well give up and go
home.  Even for PostgreSQL, getting the same results in every possible
corner case requires a certain degree of optimism.

For my money, the way to handle that is to add more control over
pushdown rather than automatically disabling it in certain cases.  For
example, we could have GUCs that disable all pushdown or certain types
of pushdown - e.g. you could specify that you don't trust the remote
side to sort data properly, or that you don't like it's floating-point
implementation.  I'm not sure what controls are most useful here, but
I'd be willing to bet you a nice glass of white wine that many people
will be happier with a system that they can control than they will be
with one that just disables the optimization in every case where it
might hypothetically not work out.  It's not clear that we can even
reasonably foresee all such cases.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-21 Thread Tom Lane
Robert Haas  writes:
> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane  wrote:
>> dromedary seems to have found one, or at least an unstable test result.

> OK, looking at that now.  Thanks.

Looking at further failures, it looks like 32-bit machines in general
get that result.  Might be just a cost estimation difference.

Also, some of the windows machines are folding "sqrt(2)" to a different
constant than is hard-wired into the expected-result file.  That's
slightly annoying because it calls into question whether we can ship
floating-point computations to the far end at all :-(.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-21 Thread Robert Haas
On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I didn't find anything structurally wrong with this patch, so I've
>> committed it with many cosmetic changes.  Judging by what happened
>> with join pushdown, there are probably some residual bugs, but
>> hopefully not too many.
>
> dromedary seems to have found one, or at least an unstable test result.

OK, looking at that now.  Thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-21 Thread Tom Lane
Robert Haas  writes:
> I didn't find anything structurally wrong with this patch, so I've
> committed it with many cosmetic changes.  Judging by what happened
> with join pushdown, there are probably some residual bugs, but
> hopefully not too many.

dromedary seems to have found one, or at least an unstable test result.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-21 Thread Robert Haas
On Thu, Oct 20, 2016 at 5:38 AM, Jeevan Chalke
 wrote:
> Changes look good to me.
> Thanks for the detailed review.

I didn't find anything structurally wrong with this patch, so I've
committed it with many cosmetic changes.  Judging by what happened
with join pushdown, there are probably some residual bugs, but
hopefully not too many.  Anyhow, I don't think waiting longer to
commit this is necessarily going to help, so let's see what happens...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-20 Thread Jeevan Chalke
On Thu, Oct 20, 2016 at 12:49 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> The patch compiles and make check-world doesn't show any failures.
>
> >>
> >
> >
> > I have tried it. Attached separate patch for it.
> > However I have noticed that istoplevel is always false (at-least for the
> > testcase we have, I get it false always). And I also think that taking
> > this decision only on PlanState is enough. Does that in attached patch.
> > To fix this, I have passed deparse_namespace to the private argument and
> > accessed it in get_special_variable(). Changes looks very simple. Please
> > have a look and let me know your views.
> > This issue is not introduced by the changes done for the aggregate push
> > down, it only got exposed as we now ship expressions in the target list.
> > Thus I think it will be good if we make these changes separately as new
> > patch, if required.
> >
>
>
> The patch looks good and pretty simple.
>
> +* expression.  However if underneath PlanState is ForeignScanState,
> then
> +* don't force parentheses.
> We need to explain why it's safe not to add paranthesis. The reason
> this function adds paranthesis so as to preserve any operator
> precedences imposed by the expression tree of which this IndexVar is
> part. For ForeignScanState, the Var may represent the root of the
> expression tree, and thus not need paranthesis. But we need to verify
> this and explain it in the comments.
>
> As you have explained this is an issue exposed by this patch;
> something not must have in this patch. If committers feel that
> aggregate pushdown needs this fix as well, please provide a patch
> addressing the above comment.
>
>
Sure.


>
> Ok. PFA patch with cosmetic changes (mostly rewording comments). Let
> me know if those look good. I am marking this patch is ready for
> committer.
>

Changes look good to me.
Thanks for the detailed review.

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-18 Thread Robert Haas
On Wed, Oct 12, 2016 at 9:18 AM, Jeevan Chalke
 wrote:
> I did performance testing for aggregate push down and see good performance
> with the patch.

Are you planning another update to this patch based on Ashutosh's comments?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-13 Thread Prabhat Sahu
On Fri, Sep 30, 2016 at 5:23 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:


> In the attached patch I have fixed all other review comments you have
> posted. All the comments were excellent and helped me a lot to improve
> in various areas.
>


Hi,

I have tested and created few extra testcases which we can consider to add
in "postgres_fdw.sql".
Please find attached patch.

Note: This patch is an addition on top of Jeevan Chalke's patch Dt: 30th,
Sept 2016.


*Thanks & Regards,*


*Prabhat Kumar Sahu*

Mob: 7758988455

Skype ID: prabhat.sahu1984

www.enterprisedb.co m



pg_agg_push_down_extra_test.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-12 Thread Jeevan Chalke
On Thu, Sep 8, 2016 at 10:41 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
>
> I think we should try to measure performance gain because of aggregate
> pushdown. The EXPLAIN
> doesn't show actual improvement in the execution times.
>

I did performance testing for aggregate push down and see good performance
with the patch.

Attached is the script I have used to get the performance numbers along with
the results I got with and without patch (pg_agg_push_down_v3.patch).  Also
attached few GNU plots from the readings I got.  These were run on my local
VM having following details:

Linux centos7 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC
2016 x86_64 x86_64 x86_64 GNU/Linux
RAM alloted: 8 GB
CPUs alloted: 8
postgresql.conf is default.

With aggregate push down I see around 12x performance for count(*)
operation.
In another test, I have observed that if number of groups returned from
remote server is same as that of input rows, then aggregate push down
performs
slightly poor which I think is expected. However in all other cases where
number of groups are less than input rows, pushing down aggregate gives
better
performance than performing grouping on the local server.

I did this performance testing on my local setup. I would expected even
better
numbers on a specialized high-end performance machine.  I would be more than
happy if someone does this testing on such high-end machine.

Let me know if you need any help.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
=== WITH PATCH ===

query|  rows   
| avg_exe_time | std_dev_exe_time | min_exe_time | max_exe_time 
-+-+--+--+--+--
 select count(*) from fperf1 |   1 
| 141.8686 |  4.4668353500495 |  138.903 |  152.241
 select c2, avg(c1) from fperf1 group by c2 having count(*) < 34 |   2 
| 405.7661 | 11.9403142844368 |  400.689 |  439.675
 select c2, sum(c1) from fperf1 group by c2  |   3 
| 363.2299 | 4.29278180851687 |  354.815 |  369.739
 select c3, avg(c1), sum(c2) from fperf1 group by c3 |   5 
| 454.4478 | 3.98680590057494 |  447.248 |  457.955
 select c4, avg(c1), sum(c2) from fperf1 group by c4 |  10 
| 501.0197 | 5.26951028823454 |  491.726 |  508.574
 select c5, avg(c1), sum(c2) from fperf1 group by c5 | 100 
| 490.0783 | 5.64261263462349 |   480.78 |  496.662
 select c6, avg(c1), sum(c2) from fperf1 group by c6 |1000 
| 582.6842 |  9.9196984474776 |  564.425 |   592.69
 select c1%1, avg(c1), sum(c2) from fperf1 group by c1%1 |   1 
| 901.1682 | 9.58382273302904 |  888.383 |  923.386
 select c1%10, avg(c1), sum(c2) from fperf1 group by c1%10   |  10 
|1032.1959 | 6.89087326268629 | 1018.598 | 1045.423
 select c1%100, avg(c1), sum(c2) from fperf1 group by c1%100 | 100 
|1076.3834 | 11.1022883947539 | 1061.305 | 1093.892
 select c1%1000, avg(c1), sum(c2) from fperf1 group by c1%1000   |1000 
|1113.6001 | 11.2143472634172 | 1092.863 | 1133.007
 select c1%1, avg(c1), sum(c2) from fperf1 group by c1%1 |   1 
|1182.1374 | 32.5953859659133 | 1148.961 | 1231.296
 select c1%10, avg(c1), sum(c2) from fperf1 group by c1%10   |  10 
|1467.1811 | 14.3535175437048 |  1443.95 | 1485.645
 select c1%100, avg(c1), sum(c2) from fperf1 group by c1%100 | 100 
|5466.2306 | 633.367848489717 | 5127.339 | 7248.381
(14 rows)


=== WITHOUT PATCH ===

query|  rows   
| avg_exe_time | std_dev_exe_time | min_exe_time | max_exe_time 
-+-+--+--+--+--
 select count(*) from fperf1 |   1 
|1674.5339 | 27.1549108570754 | 1637.345 | 1725.057
 select c2, avg(c1) from fperf1 group by c2 having count(*) < 34 |   2 
|2467.8368 |  22.606929678949 | 2437.674 | 2506.438
 select c2, sum(c1) from fperf1 group by c2  |   3 
|  2387.39 | 34.3686766983568 | 2350.396 | 2444.313
 select c3, avg(c1), sum(c2) from fperf1 group by c3 |   5 
|2702.3344 | 28.0312843452488 | 2665.317 | 2746.862
 select c4, avg(c1), sum(c2) from fperf1 group by c4 |  10 
|2850.9818 | 42.5758532759606 | 2813.562 | 2946.991
 select c5, avg(c1), 

Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-12 Thread Ashutosh Bapat
Thanks Jeevan for taking care of the comments.

On Fri, Sep 30, 2016 at 5:23 PM, Jeevan Chalke
 wrote:
> Hi Ashutosh,
>
> On Mon, Sep 26, 2016 at 2:28 PM, Ashutosh Bapat
>  wrote:
>>
>> Thanks Jeevan for working on the comments.
>>
>> Ok. Yes, we should also handle bare conditions in
>> classifyConditions(). I am fine doing it as a separate patch.
>
>
> Doing that with separate patch would be good.

Ok.

>
>>
>>
>> I don't think add_foreign_grouping_path() is the right place to change
>> a structure managed by the core and esp when we are half-way adding
>> paths. An FDW should not meddle with an upper RelOptInfo. Since
>> create_foreignscan_plan() picks those up from RelOptInfo and both of
>> those are part of core, we need a place in core to set the
>> RelOptInfo::relids for an upper relation OR we have stop using
>> fs_relids for upper relation foreign scans.
>
>
> Yes, agree that relids must be set by the core rather than a fdw.
> However I don't want to touch the core for this patch and also not
> exactly sure where we can do that. I think, for this patch, we can
> copy relids into grouped_rel in create_grouping_paths() at place
> where we assign fdwroutine, userid etc. from the input relation.

I don't think this is acceptable since it changes the search key for
an upper without going through fetch_upper_rel(). Rest of the code,
which tries to find this upper rel with relids = NULL, will not be
able to find it anymore. I have started a discussion on this topic
[1]. One possible solution discussed there is to use all_baserels for
an upper rel. But that solution looks temporary. The discussion has
not yet concluded.

In case we decide not to have any relids in the upper relation, we
will have to obtain those from the scanrel in the context, which is
not being used right now.

Also there are places in code where we check reloptkind and assume it
to be either JOINREL or BASEREL e.g. deparseLockingClause. Those need
to be examined again since now we handle an upper relation. As
discussed before, we might user bms_num_members() on relids to decide
whether the underlying scan is join or base relation.

>
>>
>>
>> 2. SortGroupClause is a parser node, so we can name
>> appendSortGroupClause() as
>> deparseSortGroupClause(), to be consistent with the naming convention. If
>> we do
>> that change, may be it's better to pass SortGroupClause as is and handle
>> other
>> members of that structure as well.
>
>
> Renamed appendSortGroupClause() to deparseSortGroupClause().
> I have kept this function in sync with get_rule_sortgroupclause() which
> takes the Index ref from SortGroupClause(). This function require just
> an index and thus passing SortGroupClause as whole is unnecessary. However
> we cannot pass entire order by list or group by list, because in case of
> order by list we need some extra processing on list elements. So passing
> just Index is sufficient and in sync with get_rule_sortgroupclause() too.

Ok.
In this function
 +   /* Must force parens for other expressions */
Please explain why we need parenthesis.

>
>>
>> 5. In deparseConst(), showtype is compared with hardcoded values. The
>> callers of this function pass hardcoded values. Instead, we should
>> define an enum and use it. I understand that this logic has been borrowed
>> from
>> get_const_expr(), which also uses hardcoded values. Any reason why not to
>> adopt
>> a better style here? In fact, it only uses two states for showtype, 0 and
>> ">
>> 0". Why don't we use a boolean then OR why isn't the third state in
>> get_const_expr() applicable here?
>
>
> We certainly can add an enum here, but for consistency with other related
> code I think using hard-coded value is good for now. Also I see this
> comment in prologue of deparseConst()
>  * This function has to be kept in sync with ruleutils.c's get_const_expr.
>
> So better to have handling like it.
> Also, currently we use only two values for showtype. But for consistency
> let use int instead of bool. In future if we add support for coercion
> expr, then we need this third value. At that time we will not need changes
> here.
> However if required, we can submit a separate patch for adding enum
> instead of int for showtype in ruleutils.c.
>

Since this patch adds showtype argument which is present in
get_const_expr(), it's clear that we haven't really kept those two
functions in sync, even though the comment says so. It's kind of ugly
to see those hardcoded values. But you have a point. Let's see what
committer says.

>>
>>
>> 7. The changes in block ...
>> should be in sync. The later block adds both aggregates and Vars but the
>> first
>> one doesn't. Why is this difference?
>
>
> add_to_flat_tlist() is taking care of duplicate entries and thus in second
> block, I was calling it after the loop to avoid calling it for every list
> element. Well, moved that inside loop like first block.
>

+   /*
+* We may need 

Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-10-02 Thread Michael Paquier
On Fri, Sep 30, 2016 at 8:58 PM, Jeevan Chalke
 wrote:
> On Mon, Sep 26, 2016 at 6:15 PM, Ashutosh Bapat
>  wrote:
>>
>> This patch will need some changes to conversion_error_callback(). That
>> function reports an error in case there was an error converting the
>> result obtained from the foreign server into an internal datum e.g.
>> when the string returned by the foreign server is not acceptable by
>> local input function for the expected datatype. In such cases, the
>> input function will throw error and conversion_error_callback() will
>> provide appropriate context for that error. postgres_fdw.sql has tests
>> to test proper context
>> We need to fix the error context to provide meaningful information or
>> at least not crash. This has been discussed briefly in [1].
>
> Oops. I had that in mind when working on this. Somehow skipped checking
> for conversion error context. I have fixed that in v3 patch.
> Removed assert and for non Var expressions, printing generic context.
> This context is almost in-line with the discussion you referred here.

Moved to next CF.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-30 Thread Jeevan Chalke
On Mon, Sep 26, 2016 at 6:15 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> This patch will need some changes to conversion_error_callback(). That
> function reports an error in case there was an error converting the
> result obtained from the foreign server into an internal datum e.g.
> when the string returned by the foreign server is not acceptable by
> local input function for the expected datatype. In such cases, the
> input function will throw error and conversion_error_callback() will
> provide appropriate context for that error. postgres_fdw.sql has tests
> to test proper context
> We need to fix the error context to provide meaningful information or
> at least not crash. This has been discussed briefly in [1].
>

Oops. I had that in mind when working on this. Somehow skipped checking
for conversion error context. I have fixed that in v3 patch.
Removed assert and for non Var expressions, printing generic context.
This context is almost in-line with the discussion you referred here.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-26 Thread Ashutosh Bapat
This patch will need some changes to conversion_error_callback(). That
function reports an error in case there was an error converting the
result obtained from the foreign server into an internal datum e.g.
when the string returned by the foreign server is not acceptable by
local input function for the expected datatype. In such cases, the
input function will throw error and conversion_error_callback() will
provide appropriate context for that error. postgres_fdw.sql has tests
to test proper context

-- ===
-- conversion error
-- ===
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
SELECT * FROM ft1 WHERE c1 = 1;  -- ERROR
SELECT  ft1.c1,  ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1
AND ft1.c1 = 1; -- ERROR
SELECT  ft1.c1,  ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND
ft1.c1 = 1; -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;

Right now we report the column name in the error context. This needs
to change for aggregate pushdown, which is pushing down expressions.
Right now, in case the aggregate on foreign server produces a string
unacceptable locally, it would crash at
4982 Assert(IsA(var, Var));
4983
4984 rte = rt_fetch(var->varno, estate->es_range_table);

since it's not a Var node as expected.

We need to fix the error context to provide meaningful information or
at least not crash. This has been discussed briefly in [1].

[1] 
https://www.postgresql.org/message-id/flat/CAFjFpRdcC7Ykb1SkARBYikx9ubKniBiAgHqMD9e47TxzY2EYFw%40mail.gmail.com#cafjfprdcc7ykb1skarbyikx9ubknibiaghqmd9e47txzy2e...@mail.gmail.com

On Fri, Sep 16, 2016 at 7:15 PM, Jeevan Chalke
 wrote:
> Hi,
>
> On Mon, Sep 12, 2016 at 5:17 PM, Jeevan Chalke
>  wrote:
>>
>> Thanks Ashutosh for the detailed review comments.
>>
>> I am working on it and will post updated patch once I fix all your
>> concerns.
>>
>>
>
> Attached new patch fixing the review comments.
>
> Here are few comments on the review points:
>
> 1. Renamed deparseFromClause() to deparseFromExpr() and
> deparseAggOrderBy() to appendAggOrderBy()
>
> 2. Done
>
> 3. classifyConditions() assumes list expressions of type RestrictInfo. But
> HAVING clause expressions (and JOIN conditions) are plain expressions. Do
> you mean we should modify the classifyConditions()? If yes, then I think it
> should be done as a separate (cleanup) patch.
>
> 4, 5. Both done.
>
> 6. Per my understanding, I think checking for just aggregate function is
> enough as we are interested in whole aggregate result. Meanwhile I will
> check whether we need to find and check shippability of transition,
> combination and finalization functions or not.
>
> 7, 8, 9, 10, 11, 12. All done.
>
> 13. Fixed. However instead of adding new function made those changes inline.
> Adding support for deparsing List expressions separating list by comma can
> be
> considered as cleanup patch as it will touch other code area not specific to
> aggregate push down.
>
> 14, 15, 16, 17. All done.
>
> 18. I think re-factoring estimate_path_cost_size() should be done separately
> as a cleanup patch too.
>
> 19, 20, 21, 22, 23, 24, 25, 26, 27, 28. All done.
>
> 29. I have tried passing input rel's relids to fetch_upper_rel() call in
> create_grouping_paths(). It solved this patch's issue, but ended up with
> few regression failures which is mostly plan changes. I am not sure whether
> we get good plan now or not as I have not analyzed them.
> So for this patch, I am setting relids in add_foreign_grouping_path()
> itself.
> However as suggested, used bms_copy(). I still kept the FIXME over there as
> I think it should have done by the core itself.
>
> 30, 31, 32, 33. All done.
>
> Let me know your views.
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-26 Thread Ashutosh Bapat
Thanks Jeevan for working on the comments.

>
> 3. classifyConditions() assumes list expressions of type RestrictInfo. But
> HAVING clause expressions (and JOIN conditions) are plain expressions. Do
> you mean we should modify the classifyConditions()? If yes, then I think it
> should be done as a separate (cleanup) patch.

Ok. Yes, we should also handle bare conditions in
classifyConditions(). I am fine doing it as a separate patch.


>
> 6. Per my understanding, I think checking for just aggregate function is
> enough as we are interested in whole aggregate result. Meanwhile I will
> check whether we need to find and check shippability of transition,
> combination and finalization functions or not.

Ok. I agree with this analysis.


> 13. Fixed. However instead of adding new function made those changes inline.
> Adding support for deparsing List expressions separating list by comma can
> be
> considered as cleanup patch as it will touch other code area not specific to
> aggregate push down.

Ok.

>
> 18. I think re-factoring estimate_path_cost_size() should be done separately
> as a cleanup patch too.

Ok.

>
> 29. I have tried passing input rel's relids to fetch_upper_rel() call in
> create_grouping_paths(). It solved this patch's issue, but ended up with
> few regression failures which is mostly plan changes. I am not sure whether
> we get good plan now or not as I have not analyzed them.
> So for this patch, I am setting relids in add_foreign_grouping_path()
> itself.
> However as suggested, used bms_copy(). I still kept the FIXME over there as
> I think it should have done by the core itself.

I don't think add_foreign_grouping_path() is the right place to change
a structure managed by the core and esp when we are half-way adding
paths. An FDW should not meddle with an upper RelOptInfo. Since
create_foreignscan_plan() picks those up from RelOptInfo and both of
those are part of core, we need a place in core to set the
RelOptInfo::relids for an upper relation OR we have stop using
fs_relids for upper relation foreign scans.

Here are some more comments on the patch, mostly focused on the tests.
1. If we dig down each usage of deparse_expr_cxt::scanrel, it boils down
checking whether a given Var comes from given scan relation (deparseVar()) or
checking whether a given Var needs relation qualification while deparsing
(again deparseVar()). I think both of those purposes can be served by looking
at scanrel::relids, multiple relids implying relation qualification. So,
instead of having a separate member scan_rel, we should instead fetch the
relids from deparse_expr_cxt::foreignrel. I am assuming that the proposed
change to set relids in upper relations is acceptable. If it's not, we should
pass scanrel->relids through the context instead of scanrel itself.

2. SortGroupClause is a parser node, so we can name appendSortGroupClause() as
deparseSortGroupClause(), to be consistent with the naming convention. If we do
that change, may be it's better to pass SortGroupClause as is and handle other
members of that structure as well.

3. Following block may be reworded
+/*
+ * aggorder too present into args so no need to check its
+ * shippability explicitly.  However, if any expression has
+ * USING clause with sort operator, we need to make sure the
+ * shippability of that operator.
+ */

as "For aggorder elements, check whether the sort operator, if specified, is
shippable or not." and mention aggorder expression along with aggdirectargs in
the comment before this one.

4. Following line is correct as long as there is only one upper relation.
+context.scanrel = (rel->reloptkind == RELOPT_UPPER_REL) ?
fpinfo->outerrel : rel;
But as we push down more and more of upper operation, there will be a chain of
upper relations on top of scan relation. So, it's better to assert that the
scanrel is a join or a base relation to be future proof.

5. In deparseConst(), showtype is compared with hardcoded values. The
callers of this function pass hardcoded values. Instead, we should
define an enum and use it. I understand that this logic has been borrowed from
get_const_expr(), which also uses hardcoded values. Any reason why not to adopt
a better style here? In fact, it only uses two states for showtype, 0 and ">
0". Why don't we use a boolean then OR why isn't the third state in
get_const_expr() applicable here?

6. "will" or "should" is missing from the following sentence.
"Plain var nodes either be same as some GROUP BY expression or part of some
GROUP BY expression.

7. The changes in block
else
{
/*
 * If we have sortgroupref set, then it means that we have an
 * ORDER BY entry pointing to this expression.  Since we are
 * not pushing ORDER BY with GROUP BY, clear it.
 */
if (sgref)

Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-16 Thread Robert Haas
On Fri, Sep 16, 2016 at 9:45 AM, Jeevan Chalke
 wrote:
> 6. Per my understanding, I think checking for just aggregate function is
> enough as we are interested in whole aggregate result.

+1.

> Meanwhile I will
> check whether we need to find and check shippability of transition,
> combination and finalization functions or not.

I don't think that'd be appropriate.  The user is calling the
aggregate, not its constituent functions.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-16 Thread Jeevan Chalke
On Mon, Sep 12, 2016 at 5:19 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

>
>
> On Mon, Sep 12, 2016 at 12:20 PM, Prabhat Sahu <
> prabhat.s...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> While testing "Aggregate pushdown", i found the below error:
>> -- GROUP BY alias showing different behavior after adding patch.
>>
>> -- Create table "t1", insert few records.
>> create table t1(c1 int);
>> insert into t1 values(10), (20);
>>
>> -- Create foreign table:
>> create foreign table f_t1 (c1 int) server db1_server options (table_name
>> 't1');
>>
>> -- with local table:
>> postgres=# select 2 a, avg(c1) from t1 group by a;
>>  a | avg
>> ---+-
>>  2 | 15.
>> (1 row)
>>
>> -- with foreign table:
>> postgres=# select 2 a, avg(c1) from f_t1 group by a;
>> ERROR:  aggregate functions are not allowed in GROUP BY
>> CONTEXT:  Remote SQL command: EXPLAIN SELECT 2, avg(c1) FROM public.t1
>> GROUP BY 2
>>
>>
>>
> Thanks for reporting this bug in *v1.patch Prabhat.
>
> I will have a look over this issue and will post a fix in next version.
>

To fix this issue, we need to make deparseConst() function aware of showtype
flag exactly as that of get_const_expr().  While deparsing Const in GROUP BY
clause, we need to show "::typename" so that it won't treat the constant
value
as a column position in the target list and rather treat it as constant
value.

Fixed this in earlier attached patch and added test-case too.

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-12 Thread Jeevan Chalke
On Mon, Sep 12, 2016 at 12:20 PM, Prabhat Sahu <
prabhat.s...@enterprisedb.com> wrote:

> Hi,
>
> While testing "Aggregate pushdown", i found the below error:
> -- GROUP BY alias showing different behavior after adding patch.
>
> -- Create table "t1", insert few records.
> create table t1(c1 int);
> insert into t1 values(10), (20);
>
> -- Create foreign table:
> create foreign table f_t1 (c1 int) server db1_server options (table_name
> 't1');
>
> -- with local table:
> postgres=# select 2 a, avg(c1) from t1 group by a;
>  a | avg
> ---+-
>  2 | 15.
> (1 row)
>
> -- with foreign table:
> postgres=# select 2 a, avg(c1) from f_t1 group by a;
> ERROR:  aggregate functions are not allowed in GROUP BY
> CONTEXT:  Remote SQL command: EXPLAIN SELECT 2, avg(c1) FROM public.t1
> GROUP BY 2
>
>
>
Thanks for reporting this bug in *v1.patch Prabhat.

I will have a look over this issue and will post a fix in next version.

Thanks
-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-12 Thread Jeevan Chalke
On Thu, Sep 8, 2016 at 10:41 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
>
>
>
>> While checking for shippability, we build the target list which is passed
>> to
>> the foreign server as fdw_scan_tlist. The target list contains
>> a. All the GROUP BY expressions
>> b. Shippable entries from the target list of upper relation
>> c. Var and Aggref nodes from non-shippable entries from the target list of
>> upper relation
>>
>
> The code in the patch doesn't seem to add Var nodes explicitly. It assumes
> that
> the Var nodes will be part of GROUP BY clause. The code is correct, I
> think.
>

Yes. Code is correct. Var nodes are already part of GROUP BY else we hit
error well before this point.

Thanks Ashutosh for the detailed review comments.

I am working on it and will post updated patch once I fix all your concerns.

Thanks
-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-12 Thread Prabhat Sahu
Hi,

While testing "Aggregate pushdown", i found the below error:
-- GROUP BY alias showing different behavior after adding patch.

-- Create table "t1", insert few records.
create table t1(c1 int);
insert into t1 values(10), (20);

-- Create foreign table:
create foreign table f_t1 (c1 int) server db1_server options (table_name
't1');

-- with local table:
postgres=# select 2 a, avg(c1) from t1 group by a;
 a | avg
---+-
 2 | 15.
(1 row)

-- with foreign table:
postgres=# select 2 a, avg(c1) from f_t1 group by a;
ERROR:  aggregate functions are not allowed in GROUP BY
CONTEXT:  Remote SQL command: EXPLAIN SELECT 2, avg(c1) FROM public.t1
GROUP BY 2



On Thu, Sep 8, 2016 at 10:41 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
>
>
>
>> While checking for shippability, we build the target list which is passed
>> to
>> the foreign server as fdw_scan_tlist. The target list contains
>> a. All the GROUP BY expressions
>> b. Shippable entries from the target list of upper relation
>> c. Var and Aggref nodes from non-shippable entries from the target list of
>> upper relation
>>
>
> The code in the patch doesn't seem to add Var nodes explicitly. It assumes
> that
> the Var nodes will be part of GROUP BY clause. The code is correct, I
> think.
>
>
>> d. Var and Aggref nodes from non-shippable HAVING conditions.
>>
>
> This needs to be changed as per the comments below.
>
>>
>> Costing:
>>
>> If use_foreign_estimate is true for input relation, like JOIN case, we use
>> EXPLAIN output to get the cost of query with aggregation/grouping on the
>> foreign server. If not we calculate the costs locally. Similar to core,
>> we use
>> get_agg_clause_costs() to get costs for aggregation and then using logic
>> similar to cost_agg() we calculate startup and total cost. Since we have
>> no
>> idea which aggregation strategy will be used at foreign side, we add all
>> startup cost (startup cost of input relation, aggregates etc.) into
>> startup
>> cost for the grouping path and similarly for total cost.
>>
>
> This looks OK to me.
>
>
>>
>> Deparsing the query:
>>
>> Target list created while checking for shippability is deparsed using
>> deparseExplicitTargetList(). sortgroupref are adjusted according to this
>> target list. Most of the logic to deparse an Aggref is inspired from
>> get_agg_expr(). For an upper relation, FROM and WHERE clauses come from
>> the
>> underlying scan relation and thus for simplicity, FROM clause deparsing
>> logic
>> is moved from deparseSelectSql() to a new function deparseFromClause().
>> The
>> same function adds WHERE clause to the remote SQL.
>>
>
> Ok.
>
>
>>
>>
>> Area of future work:
>>
>> 1. Adding path with path-keys to push ORDER BY clause along with
>> aggregation/
>> grouping.  Should be supported as a separate patch.
>>
>> 2. Grouping Sets/Rollup/Cube is not supported in current version. I have
>> left
>> this aside to keep patch smaller. If required I can add that support in
>> the
>> next version of the patch.
>>
>
> I am fine with these limitations for first cut of this feature.
>
> I think we should try to measure performance gain because of aggregate
> pushdown. The EXPLAIN
> doesn't show actual improvement in the execution times.
>
> Here are the comments on the patch.
>
> Patch compiles without errors/warnings - Yes
> make check passes - Yes.
> make check in contrib/postgres_fdw passes - Yes
>
> Attached patch (based on your patch) has some typos corrected, some
> comments
> rephrased. It also has some code changes, as explained in various comments
> below. Please see if those look good.
>
> 1. Usually for any deparseSomething function, Something is the type of node
> produced by the parser when the string output by that function is parsed.
> deparseColumnRef, for example, produces a string, which when parsed
> produces a
> columnRef node. There is are nodes of type FromClause and AggOrderBy. I
> guess,
> you meant FromExpr instead of FromClause. deparseAggOrderBy() may be
> renamed as
> appendOrderByFromList() or something similar. It may be utilized for window
> functions if required.
>
> 2. Can we infer the value of foragg flag from the RelOptInfo passed to
> is_foreign_expr()? For any upper relation, it is ok to have aggregate in
> there. For any other relation aggregate is not expected.
>
> 3. In function foreign_grouping_ok(), should we use classifyConditions()?
> The
> function is written and used for base relations. There's nothing in that
> function, which prohibits it being used for other relations. I feel that
> foreign_join_ok() should have used the same function to classify the other
> clauses.
>
> 4. May be the individual criteria in the comment block
> +/*
> + * Aggregate is safe to pushdown if
> + * 1. It is a built-in aggregate
> + * 2. All its arguments are safe to push-down
> + * 3. The functions involved are 

Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-09-08 Thread Ashutosh Bapat
> While checking for shippability, we build the target list which is passed
> to
> the foreign server as fdw_scan_tlist. The target list contains
> a. All the GROUP BY expressions
> b. Shippable entries from the target list of upper relation
> c. Var and Aggref nodes from non-shippable entries from the target list of
> upper relation
>

The code in the patch doesn't seem to add Var nodes explicitly. It assumes
that
the Var nodes will be part of GROUP BY clause. The code is correct, I think.


> d. Var and Aggref nodes from non-shippable HAVING conditions.
>

This needs to be changed as per the comments below.

>
> Costing:
>
> If use_foreign_estimate is true for input relation, like JOIN case, we use
> EXPLAIN output to get the cost of query with aggregation/grouping on the
> foreign server. If not we calculate the costs locally. Similar to core, we
> use
> get_agg_clause_costs() to get costs for aggregation and then using logic
> similar to cost_agg() we calculate startup and total cost. Since we have no
> idea which aggregation strategy will be used at foreign side, we add all
> startup cost (startup cost of input relation, aggregates etc.) into startup
> cost for the grouping path and similarly for total cost.
>

This looks OK to me.


>
> Deparsing the query:
>
> Target list created while checking for shippability is deparsed using
> deparseExplicitTargetList(). sortgroupref are adjusted according to this
> target list. Most of the logic to deparse an Aggref is inspired from
> get_agg_expr(). For an upper relation, FROM and WHERE clauses come from the
> underlying scan relation and thus for simplicity, FROM clause deparsing
> logic
> is moved from deparseSelectSql() to a new function deparseFromClause(). The
> same function adds WHERE clause to the remote SQL.
>

Ok.


>
>
> Area of future work:
>
> 1. Adding path with path-keys to push ORDER BY clause along with
> aggregation/
> grouping.  Should be supported as a separate patch.
>
> 2. Grouping Sets/Rollup/Cube is not supported in current version. I have
> left
> this aside to keep patch smaller. If required I can add that support in the
> next version of the patch.
>

I am fine with these limitations for first cut of this feature.

I think we should try to measure performance gain because of aggregate
pushdown. The EXPLAIN
doesn't show actual improvement in the execution times.

Here are the comments on the patch.

Patch compiles without errors/warnings - Yes
make check passes - Yes.
make check in contrib/postgres_fdw passes - Yes

Attached patch (based on your patch) has some typos corrected, some comments
rephrased. It also has some code changes, as explained in various comments
below. Please see if those look good.

1. Usually for any deparseSomething function, Something is the type of node
produced by the parser when the string output by that function is parsed.
deparseColumnRef, for example, produces a string, which when parsed
produces a
columnRef node. There is are nodes of type FromClause and AggOrderBy. I
guess,
you meant FromExpr instead of FromClause. deparseAggOrderBy() may be
renamed as
appendOrderByFromList() or something similar. It may be utilized for window
functions if required.

2. Can we infer the value of foragg flag from the RelOptInfo passed to
is_foreign_expr()? For any upper relation, it is ok to have aggregate in
there. For any other relation aggregate is not expected.

3. In function foreign_grouping_ok(), should we use classifyConditions()?
The
function is written and used for base relations. There's nothing in that
function, which prohibits it being used for other relations. I feel that
foreign_join_ok() should have used the same function to classify the other
clauses.

4. May be the individual criteria in the comment block
+/*
+ * Aggregate is safe to pushdown if
+ * 1. It is a built-in aggregate
+ * 2. All its arguments are safe to push-down
+ * 3. The functions involved are immutable.
+ * 4. Other expressions involved like aggorder,
aggdistinct are
+ *safe to be pushed down.
+ */
should be associated with the code blocks which implement those. As the
criteria change it's difficult to maintain the numbered list in sync with
the
code.

5. The comment
+/* Aggregates other than simple one are non-pushable. */
should read /* Only non-split aggregates are pushable. */ as AGGSPLIT_SIMPLE
means a complete, non-split aggregation step.

6. An aggregate function has transition, combination and finalization
function
associated with it. Should we check whether all of the functions are
shippable?
But probably it suffices to check whether aggregate function as whole is
shippable or not using is_shippable() since it's the whole aggregate we are
interested in and not the intermediate results. Probably, we should add a
comment explaining why it's sufficient to check the aggregate function 

Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-31 Thread Pavel Stehule
2016-08-31 10:03 GMT+02:00 Amit Langote :

> On 2016/08/31 16:42, Pavel Stehule wrote:
> > 2016-08-31 9:00 GMT+02:00 Robert Haas :
> >
> >> On Wed, Aug 31, 2016 at 11:56 AM, Pavel Stehule <
> pavel.steh...@gmail.com>
> >> wrote:
> >>> It is pity - lot of performance issues are related to this missing
> >> feature.
> >>
> >> I don't think you are being very clear about what feature you are
> >> talking about.  The feature that Jeevan has implemented is pushing
> >> aggregates to the remote side when postgres_fdw is in use.  The
> >> feature you are talking about is evidently something else, but you
> >> haven't really said what it is.  Or not in a way that I can
> >> understand.
> >>
> >
> > yes, It is not clear if FDW aggregate push down can be implemented
> together
> > with internal aggregate push down. Aggregate push down ~ try to aggregate
> > first when it is possible
>
> What do you mean by "internal aggregate push down"?  Partition-wise
> aggregation?  Aggregate/group by before join (something like [1])?  IIUC,
> what Jeevan proposes in this thread is to implement the aggregate push
> down API that is specific to FDWs in postgres_fdw.  Any other push down
> work would need to use different APIs and would need to separately
> proposed/discussed.
>

ok I understand now.

Regards

Pavel


>
> Thanks,
> Amit
>
> [1]
> https://www.postgresql.org/message-id/CAKJS1f9kw95K2pnCKAoPmNw==
> 7fgjsjc-82cy1rb+-x-jz0...@mail.gmail.com
>
>
>


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-31 Thread Amit Langote
On 2016/08/31 16:42, Pavel Stehule wrote:
> 2016-08-31 9:00 GMT+02:00 Robert Haas :
> 
>> On Wed, Aug 31, 2016 at 11:56 AM, Pavel Stehule 
>> wrote:
>>> It is pity - lot of performance issues are related to this missing
>> feature.
>>
>> I don't think you are being very clear about what feature you are
>> talking about.  The feature that Jeevan has implemented is pushing
>> aggregates to the remote side when postgres_fdw is in use.  The
>> feature you are talking about is evidently something else, but you
>> haven't really said what it is.  Or not in a way that I can
>> understand.
>>
> 
> yes, It is not clear if FDW aggregate push down can be implemented together
> with internal aggregate push down. Aggregate push down ~ try to aggregate
> first when it is possible

What do you mean by "internal aggregate push down"?  Partition-wise
aggregation?  Aggregate/group by before join (something like [1])?  IIUC,
what Jeevan proposes in this thread is to implement the aggregate push
down API that is specific to FDWs in postgres_fdw.  Any other push down
work would need to use different APIs and would need to separately
proposed/discussed.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAKJS1f9kw95K2pnCKAoPmNw==7fgjsjc-82cy1rb+-x-jz0...@mail.gmail.com




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-31 Thread Pavel Stehule
2016-08-31 9:00 GMT+02:00 Robert Haas :

> On Wed, Aug 31, 2016 at 11:56 AM, Pavel Stehule 
> wrote:
> > It is pity - lot of performance issues are related to this missing
> feature.
>
> I don't think you are being very clear about what feature you are
> talking about.  The feature that Jeevan has implemented is pushing
> aggregates to the remote side when postgres_fdw is in use.  The
> feature you are talking about is evidently something else, but you
> haven't really said what it is.  Or not in a way that I can
> understand.
>

yes, It is not clear if FDW aggregate push down can be implemented together
with internal aggregate push down. Aggregate push down ~ try to aggregate
first when it is possible

Regards

Pavel

>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-31 Thread Robert Haas
On Wed, Aug 31, 2016 at 11:56 AM, Pavel Stehule  wrote:
> It is pity - lot of performance issues are related to this missing feature.

I don't think you are being very clear about what feature you are
talking about.  The feature that Jeevan has implemented is pushing
aggregates to the remote side when postgres_fdw is in use.  The
feature you are talking about is evidently something else, but you
haven't really said what it is.  Or not in a way that I can
understand.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-31 Thread Pavel Stehule
2016-08-31 8:17 GMT+02:00 Jeevan Chalke :

>
>
> On Tue, Aug 30, 2016 at 6:51 PM, Pavel Stehule 
> wrote:
>
>> Hi
>>
>> 2016-08-30 15:02 GMT+02:00 Jeevan Chalke 
>> :
>>
>>> Hi all,
>>>
>>> Attached is the patch which adds support to push down aggregation and
>>> grouping
>>> to the foreign server for postgres_fdw. Performing aggregation on foreign
>>> server results into fetching fewer rows from foreign side as compared to
>>> fetching all the rows and aggregating/grouping locally. Performing
>>> grouping on
>>> foreign server may use indexes if available. So pushing down aggregates/
>>> grouping on foreign server performs better than doing that locally.
>>> (Attached
>>> EXPLAIN output for few simple grouping queries, with and without push
>>> down).
>>>
>>
>> is it work without FDW too?. It can be pretty interesting too.
>>
>
> No. Aggrgate push down is supported through the GetForeignUpperPaths() hook
> added for postgres_fdw. Thus it works only with postgres_fdw.
>
> Do you mean whether this works with any extensions via implementing
> create_upper_paths_hook() function?
> The answer is No. This patch does not touch this hook.
>

It is pity - lot of performance issues are related to this missing feature.

Regards

Pavel


>
>
>>
>> Regards
>>
>> Pavel
>>
>>
>>>
>>>
>>
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-31 Thread Jeevan Chalke
On Tue, Aug 30, 2016 at 6:51 PM, Pavel Stehule 
wrote:

> Hi
>
> 2016-08-30 15:02 GMT+02:00 Jeevan Chalke :
>
>> Hi all,
>>
>> Attached is the patch which adds support to push down aggregation and
>> grouping
>> to the foreign server for postgres_fdw. Performing aggregation on foreign
>> server results into fetching fewer rows from foreign side as compared to
>> fetching all the rows and aggregating/grouping locally. Performing
>> grouping on
>> foreign server may use indexes if available. So pushing down aggregates/
>> grouping on foreign server performs better than doing that locally.
>> (Attached
>> EXPLAIN output for few simple grouping queries, with and without push
>> down).
>>
>
> is it work without FDW too?. It can be pretty interesting too.
>

No. Aggrgate push down is supported through the GetForeignUpperPaths() hook
added for postgres_fdw. Thus it works only with postgres_fdw.

Do you mean whether this works with any extensions via implementing
create_upper_paths_hook() function?
The answer is No. This patch does not touch this hook.


>
> Regards
>
> Pavel
>
>
>>
>>
>

Thanks
-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


Re: [HACKERS] Aggregate Push Down - Performing aggregation on foreign server

2016-08-30 Thread Pavel Stehule
Hi

2016-08-30 15:02 GMT+02:00 Jeevan Chalke :

> Hi all,
>
> Attached is the patch which adds support to push down aggregation and
> grouping
> to the foreign server for postgres_fdw. Performing aggregation on foreign
> server results into fetching fewer rows from foreign side as compared to
> fetching all the rows and aggregating/grouping locally. Performing
> grouping on
> foreign server may use indexes if available. So pushing down aggregates/
> grouping on foreign server performs better than doing that locally.
> (Attached
> EXPLAIN output for few simple grouping queries, with and without push
> down).
>

is it work without FDW too?. It can be pretty interesting too.

Regards

Pavel


>
> Here are the few details of the implementation
>
> Creating Paths:
>
> Implements the FDW hook GetForeignUpperPaths, which adds foreign scan path
> to
> the output relation when upper relation kind is UPPERREL_GROUP_AGG. This
> path
> represents the aggregation/grouping operations to be performed on the
> foreign
> server. We are able to push down aggregation/grouping if (implemented in
> foreign_grouping_ok()),
> a. Underlying input relation is safe to push down and has no local
> conditions,
> as local conditions need to be applied before aggregation.
> b. All the aggregates, GROUP BY expressions are safe to push down.
> foreign_grouping_ok() functions assesses it.
>
> While checking for shippability, we build the target list which is passed
> to
> the foreign server as fdw_scan_tlist. The target list contains
> a. All the GROUP BY expressions
> b. Shippable entries from the target list of upper relation
> c. Var and Aggref nodes from non-shippable entries from the target list of
> upper relation
> d. Var and Aggref nodes from non-shippable HAVING conditions.
>
> The shippable having conditions are sent to the foreign server as part of
> the
> HAVING clause of the remote SQL.
>
> is_foreign_expr() function, now handles T_Aggref node. Aggregate is safe to
> push down if,
> a. Aggregate is a built-in aggregate
> b. All its arguments are safe to push-down
> c. Other expressions involved like aggorder, aggdistinct, aggfilter etc.
> are
> safe to be pushed down.
>
>
> Costing:
>
> If use_foreign_estimate is true for input relation, like JOIN case, we use
> EXPLAIN output to get the cost of query with aggregation/grouping on the
> foreign server. If not we calculate the costs locally. Similar to core, we
> use
> get_agg_clause_costs() to get costs for aggregation and then using logic
> similar to cost_agg() we calculate startup and total cost. Since we have no
> idea which aggregation strategy will be used at foreign side, we add all
> startup cost (startup cost of input relation, aggregates etc.) into startup
> cost for the grouping path and similarly for total cost.
>
> Deparsing the query:
>
> Target list created while checking for shippability is deparsed using
> deparseExplicitTargetList(). sortgroupref are adjusted according to this
> target list. Most of the logic to deparse an Aggref is inspired from
> get_agg_expr(). For an upper relation, FROM and WHERE clauses come from the
> underlying scan relation and thus for simplicity, FROM clause deparsing
> logic
> is moved from deparseSelectSql() to a new function deparseFromClause(). The
> same function adds WHERE clause to the remote SQL.
>
>
> Area of future work:
>
> 1. Adding path with path-keys to push ORDER BY clause along with
> aggregation/
> grouping.  Should be supported as a separate patch.
>
> 2. Grouping Sets/Rollup/Cube is not supported in current version. I have
> left
> this aside to keep patch smaller. If required I can add that support in the
> next version of the patch.
>
>
> Most of the code in this patch is inspired from the JOIN push down code.
> Ashutosh Bapat provided a high-level design and a skeleton patch to
> start-with
> offlist. Thanks to Tom Lane for his upper-planner pathification work and
> adding
> GetForeignUpperPaths callback function.
>
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 30589500
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>
>
> --
> Sent via pgsql-hackers