Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-10-01 Thread Daniel Gustafsson
> On 08 Apr 2017, at 16:14, David Steele wrote: > > On 3/22/17 6:20 AM, Etsuro Fujita wrote: >> On 2017/02/22 19:57, Rushabh Lathia wrote: >>> Marked this as Ready for Committer. >> >> I noticed that this item in the CF app was incorrectly marked as >> Committed. This

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-04-08 Thread David Steele
On 3/22/17 6:20 AM, Etsuro Fujita wrote: > On 2017/02/22 19:57, Rushabh Lathia wrote: >> Marked this as Ready for Committer. > > I noticed that this item in the CF app was incorrectly marked as > Committed. This patch isn't committed, so I returned it to the previous > status. I also rebased

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-04-07 Thread Robert Haas
On Wed, Mar 22, 2017 at 6:20 AM, Etsuro Fujita wrote: > On 2017/02/22 19:57, Rushabh Lathia wrote: >> Marked this as Ready for Committer. > > I noticed that this item in the CF app was incorrectly marked as Committed. > This patch isn't committed, so I returned it to

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-03-22 Thread Etsuro Fujita
On 2017/02/22 19:57, Rushabh Lathia wrote: Marked this as Ready for Committer. I noticed that this item in the CF app was incorrectly marked as Committed. This patch isn't committed, so I returned it to the previous status. I also rebased the patch. Attached is a new version of the patch.

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-03-21 Thread Etsuro Fujita
On 2017/03/17 2:35, Robert Haas wrote: And ... I don't see anything to complain about, so, committed. Thanks for committing, Robert! Thanks for reviewing, Ashutosh and David! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-03-16 Thread Robert Haas
On Thu, Mar 16, 2017 at 12:48 PM, Robert Haas wrote: > On Thu, Mar 16, 2017 at 11:46 AM, David Steele wrote: >> $ patch -p1 < ../other/postgres-fdw-subquery-support-v15.patch >> patching file contrib/postgres_fdw/deparse.c >> Hunk #11 succeeded at 1371

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-03-16 Thread Robert Haas
On Thu, Mar 16, 2017 at 11:46 AM, David Steele wrote: > $ patch -p1 < ../other/postgres-fdw-subquery-support-v15.patch > patching file contrib/postgres_fdw/deparse.c > Hunk #11 succeeded at 1371 (offset -3 lines). > Hunk #12 succeeded at 1419 (offset -3 lines). > Hunk #13

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-03-16 Thread David Steele
On 1/30/17 6:30 AM, Etsuro Fujita wrote: > On 2017/01/27 21:25, Etsuro Fujita wrote: >> On 2017/01/27 20:04, Ashutosh Bapat wrote: >>> I think we should pick up your patch on >>> 27th December, update the comment per your mail on 5th Jan. I will >>> review it once and list down the things left to

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-02-22 Thread Rushabh Lathia
On Wed, Feb 22, 2017 at 12:15 PM, Etsuro Fujita wrote: > On 2017/02/21 19:31, Rushabh Lathia wrote: > >> On Mon, Feb 20, 2017 at 1:41 PM, Etsuro Fujita >> > wrote: >> > > On 2017/02/13 18:24,

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-02-21 Thread Etsuro Fujita
On 2017/02/21 19:31, Rushabh Lathia wrote: On Mon, Feb 20, 2017 at 1:41 PM, Etsuro Fujita > wrote: On 2017/02/13 18:24, Rushabh Lathia wrote: Here are few comments: 1) @@ -211,6 +211,12 @@ typedef

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-02-21 Thread Rushabh Lathia
On Mon, Feb 20, 2017 at 1:41 PM, Etsuro Fujita wrote: > On 2017/02/13 18:24, Rushabh Lathia wrote: > >> I started reviewing the patch again. Patch applied cleanly on latest >> source >> as well as regression pass through with the patch. I also performed >> few manual

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-02-20 Thread Etsuro Fujita
On 2017/02/13 18:24, Rushabh Lathia wrote: I started reviewing the patch again. Patch applied cleanly on latest source as well as regression pass through with the patch. I also performed few manual testing and haven't found any regression. Patch look much cleaner the earlier version, and don't

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-02-13 Thread Rushabh Lathia
Sorry for delay in the review. I started reviewing the patch again. Patch applied cleanly on latest source as well as regression pass through with the patch. I also performed few manual testing and haven't found any regression. Patch look much cleaner the earlier version, and don't have any major

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-01-31 Thread Michael Paquier
On Wed, Jan 25, 2017 at 7:20 PM, Etsuro Fujita wrote: > Attached is the new version of the patch. I also addressed other comments > from you: moved rewriting the fdw_scan_tlist to postgres_fdw.c, > added/revised comments, and added regression tests for the case where

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-31 Thread Michael Paquier
On Mon, Jan 30, 2017 at 8:30 PM, Etsuro Fujita wrote: > Other changes: > * I went back to make_outerrel_subquery and make_innerrel_subquery, which > are flags to indicate whether to deparse the input relations as subqueries. > is_subquery_rel would work well for

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-31 Thread Etsuro Fujita
On 2017/01/30 21:05, Ashutosh Bapat wrote: On Mon, Jan 30, 2017 at 5:00 PM, Etsuro Fujita wrote: On 2017/01/27 21:25, Etsuro Fujita wrote: Sorry, I started thinking we went in the wrong direction. I added to deparseSelectStmtForRel build_subquery_tlists, which

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-30 Thread Ashutosh Bapat
On Mon, Jan 30, 2017 at 5:00 PM, Etsuro Fujita wrote: > On 2017/01/27 21:25, Etsuro Fujita wrote: >> >> On 2017/01/27 20:04, Ashutosh Bapat wrote: >>> >>> I think we should pick up your patch on >>> 27th December, update the comment per your mail on 5th Jan. I will

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-30 Thread Etsuro Fujita
On 2017/01/27 21:25, Etsuro Fujita wrote: On 2017/01/27 20:04, Ashutosh Bapat wrote: I think we should pick up your patch on 27th December, update the comment per your mail on 5th Jan. I will review it once and list down the things left to committer's judgement. Sorry, I started thinking we

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-27 Thread Etsuro Fujita
On 2017/01/27 20:04, Ashutosh Bapat wrote: On Fri, Jan 13, 2017 at 12:30 PM, Etsuro Fujita wrote: A more clean way I'm thinking is: (1) in postgresGetForeignJoinPaths(), create a tlist by build_tlist_to_deparse() and save it in fpinfo->tlist before

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-27 Thread Ashutosh Bapat
On Fri, Jan 13, 2017 at 12:30 PM, Etsuro Fujita wrote: > On 2017/01/12 18:25, Ashutosh Bapat wrote: >> >> On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita >> wrote: > > > On 2017/01/05 21:11, Ashutosh Bapat wrote: >> >> IIUC,

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2017-01-25 Thread Etsuro Fujita
On 2016/11/30 17:29, Etsuro Fujita wrote: On 2016/11/23 20:28, Rushabh Lathia wrote: I wrote: How about inserting that before the out param **retrieved_attrs, like this? static void deparseExplicitTargetList(List *tlist, bool is_returning,

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-12 Thread Etsuro Fujita
On 2017/01/12 18:25, Ashutosh Bapat wrote: On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita wrote: On 2017/01/05 21:11, Ashutosh Bapat wrote: IIUC, for a relation with use_remote_estimates we will deparse the query twice and will build the targetlist twice.

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-12 Thread Ashutosh Bapat
On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita wrote: > On 2017/01/05 21:38, Ashutosh Bapat wrote: >> >> On Thu, Jan 5, 2017 at 5:51 PM, Etsuro Fujita >> wrote: >>> >>> On 2017/01/05 21:11, Ashutosh Bapat wrote: > > > On 2017/01/03

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-11 Thread Etsuro Fujita
On 2017/01/05 21:38, Ashutosh Bapat wrote: On Thu, Jan 5, 2017 at 5:51 PM, Etsuro Fujita wrote: On 2017/01/05 21:11, Ashutosh Bapat wrote: On 2017/01/03 17:28, Ashutosh Bapat wrote: Also, in this function, if fpinfo->tlist is already set, why do we want to

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-05 Thread Ashutosh Bapat
On Thu, Jan 5, 2017 at 5:51 PM, Etsuro Fujita wrote: > On 2017/01/05 21:11, Ashutosh Bapat wrote: >> >> On Thu, Jan 5, 2017 at 5:14 PM, Etsuro Fujita >> wrote: >>> >>> On 2017/01/03 17:28, Ashutosh Bapat wrote: In

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-05 Thread Etsuro Fujita
On 2017/01/05 21:11, Ashutosh Bapat wrote: On Thu, Jan 5, 2017 at 5:14 PM, Etsuro Fujita wrote: On 2017/01/03 17:28, Ashutosh Bapat wrote: In build_subquery_tlists(), why don't we handle base relations? + if (foreignrel->reloptkind != RELOPT_JOINREL) +

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-05 Thread Ashutosh Bapat
On Thu, Jan 5, 2017 at 5:14 PM, Etsuro Fujita wrote: > On 2017/01/03 17:28, Ashutosh Bapat wrote: > > I wrote: >>> >>> I updated the patch a bit further: simplified the function name >>> (s/build_subquery_rel_tlists/build_subquery_tlists/), and revised >>> comments a

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-05 Thread Etsuro Fujita
On 2017/01/03 17:28, Ashutosh Bapat wrote: I wrote: I updated the patch a bit further: simplified the function name (s/build_subquery_rel_tlists/build_subquery_tlists/), and revised comments a little bit. Attached is an updated version (postgres-fdw-subquery-support-v14.patch). Few comments

Re: [HACKERS] Push down more full joins in postgres_fdw

2017-01-03 Thread Ashutosh Bapat
> > I updated the patch a bit further: simplified the function name > (s/build_subquery_rel_tlists/build_subquery_tlists/), and revised comments a > little bit. Attached is an updated version > (postgres-fdw-subquery-support-v14.patch). And I rebased another patch for > PHVs against that patch,

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-27 Thread Etsuro Fujita
On 2016/12/08 21:08, Etsuro Fujita wrote: On 2016/12/07 20:23, Etsuro Fujita wrote: My proposal here would be a bit different from what you proposed; right before deparseSelectSql in deparseSelectStmtForRel, build the tlists for relations present in the given jointree that will be deparsed as

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-08 Thread Etsuro Fujita
On 2016/12/07 20:23, Etsuro Fujita wrote: On 2016/12/07 15:39, Ashutosh Bapat wrote: On 2016/11/22 18:28, Ashutosh Bapat wrote: I guess, the reason why you are doing it this way, is SELECT clause for the outermost query gets deparsed before FROM clause. For later we call

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-08 Thread Etsuro Fujita
On 2016/12/07 21:11, Ashutosh Bapat wrote: On Wed, Dec 7, 2016 at 4:51 PM, Etsuro Fujita wrote: On 2016/12/05 20:20, Ashutosh Bapat wrote: 3. Why is foreignrel variable changed to rel? -extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, -

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-07 Thread Ashutosh Bapat
On Wed, Dec 7, 2016 at 4:51 PM, Etsuro Fujita wrote: > On 2016/12/05 20:20, Ashutosh Bapat wrote: >> >> On Mon, Nov 28, 2016 at 3:52 PM, Etsuro Fujita >> wrote: >>> >>> On 2016/11/24 21:45, Etsuro Fujita wrote: * I removed

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-07 Thread Etsuro Fujita
On 2016/12/07 15:39, Ashutosh Bapat wrote: On 2016/11/22 18:28, Ashutosh Bapat wrote: I guess, the reason why you are doing it this way, is SELECT clause for the outermost query gets deparsed before FROM clause. For later we call deparseRangeTblRef(), which builds the tlist. So, while

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-07 Thread Etsuro Fujita
On 2016/12/05 20:20, Ashutosh Bapat wrote: On Mon, Nov 28, 2016 at 3:52 PM, Etsuro Fujita wrote: On 2016/11/24 21:45, Etsuro Fujita wrote: * I removed make_outerrel_subquery/make_innerrel_subquery from fpinfo and added a new member "is_subquery_rel" to fpinfo, as

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-06 Thread Ashutosh Bapat
> > > Ashutosh proposed this to do the comparison: > > On 2016/11/22 18:28, Ashutosh Bapat wrote: >> >> I guess, the reason why you are doing it this way, is SELECT clause for >> the >> outermost query gets deparsed before FROM clause. For later we call >> deparseRangeTblRef(), which builds the

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-06 Thread Ashutosh Bapat
On Wed, Dec 7, 2016 at 1:57 AM, Robert Haas wrote: > On Mon, Dec 5, 2016 at 6:20 AM, Ashutosh Bapat > wrote: >> 4. I am still not happy with this change >> +/* >> + * Since (1) the expressions in foreignrel's reltarget

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-06 Thread Etsuro Fujita
On 2016/12/07 5:27, Robert Haas wrote: On Mon, Dec 5, 2016 at 6:20 AM, Ashutosh Bapat wrote: 4. I am still not happy with this change +/* + * Since (1) the expressions in foreignrel's reltarget doesn't contain + * any PHVs and (2)

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-06 Thread Robert Haas
On Mon, Dec 5, 2016 at 6:20 AM, Ashutosh Bapat wrote: > 4. I am still not happy with this change > +/* > + * Since (1) the expressions in foreignrel's reltarget doesn't > contain > + * any PHVs and (2) foreignrel's local_conds is empty,

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-12-05 Thread Ashutosh Bapat
On Mon, Nov 28, 2016 at 3:52 PM, Etsuro Fujita wrote: > On 2016/11/24 21:45, Etsuro Fujita wrote: >> >> On 2016/11/22 18:28, Ashutosh Bapat wrote: >>> >>> The comments should explain why is the assertion true. >>> +/* Shouldn't be NIL */ >>> +

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-30 Thread Etsuro Fujita
On 2016/11/23 20:28, Rushabh Lathia wrote: On Tue, Nov 22, 2016 at 6:24 PM, Etsuro Fujita > wrote: 1) -static void deparseExplicitTargetList(List *tlist, List **retrieved_attrs, +static void

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-28 Thread Etsuro Fujita
On 2016/11/24 21:45, Etsuro Fujita wrote: On 2016/11/22 18:28, Ashutosh Bapat wrote: The comments should explain why is the assertion true. +/* Shouldn't be NIL */ +Assert(tlist != NIL); I noticed that I was wrong; in the Assertion the tlist can be empty. An example for such

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-24 Thread Etsuro Fujita
On 2016/11/22 18:28, Ashutosh Bapat wrote: The comments should explain why is the assertion true. +/* Shouldn't be NIL */ +Assert(tlist != NIL); I noticed that I was wrong; in the Assertion the tlist can be empty. An example for such a case is: SELECT 1 FROM (SELECT c1 FROM

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-24 Thread Etsuro Fujita
On 2016/11/24 18:20, Ashutosh Bapat wrote: I wrote: You missed the point; the foreignrel->reltarget->exprs doesn't contain any PHVs, so the tlist created by build_tlist_to_depase will be guaranteed to be one-to-one with the foreignrel->reltarget->exprs. You wrote: It's guaranteed now, but

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-24 Thread Ashutosh Bapat
> > build_tlist_to_depase() calls pull_var_nodes() before creating the tlist, whereas the code that searches does not do that. Code-wise those are not the same things. > > >>> You missed the point; the foreignrel->reltarget->exprs doesn't contain >>> any >>> PHVs, so the tlist

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-24 Thread Etsuro Fujita
On 2016/11/24 17:39, Ashutosh Bapat wrote: On Thu, Nov 24, 2016 at 1:27 PM, Etsuro Fujita wrote: On 2016/11/24 16:46, Ashutosh Bapat wrote: table will be misleading as subquery can represent a join and corresponding alias would represent the join. Relation is

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-24 Thread Ashutosh Bapat
On Thu, Nov 24, 2016 at 1:27 PM, Etsuro Fujita wrote: > On 2016/11/24 16:46, Ashutosh Bapat wrote: Sorry. I think the current version is better than previous one. The term "subselect alias" is confusing in the previous version. In the current

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-24 Thread Etsuro Fujita
On 2016/11/24 16:46, Ashutosh Bapat wrote: Sorry. I think the current version is better than previous one. The term "subselect alias" is confusing in the previous version. In the current version, "Get the relation and column alias for a given Var node," we need to add word "identifiers" like

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-23 Thread Ashutosh Bapat
> >> Sorry. I think the current version is better than previous one. The >> term "subselect alias" is confusing in the previous version. In the >> current version, "Get the relation and column alias for a given Var >> node," we need to add word "identifiers" like "Get the relation and >> column

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-23 Thread Etsuro Fujita
On 2016/11/23 0:30, Ashutosh Bapat wrote: You wrote: The comment below says "get the aliases", but what the function really returns is the identifiers used for creating aliases. Please correct the comment. +/* + * Get the relation and column alias for a given Var node, which belongs to + *

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-23 Thread Rushabh Lathia
On Tue, Nov 22, 2016 at 6:24 PM, Etsuro Fujita wrote: > Hi Rushabh, > > On 2016/11/22 19:05, Rushabh Lathia wrote: > >> I started reviewing the patch and here are few initial review points and >> questions for you. >> > > Thanks for the review! > > 1) >> -static void

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-22 Thread Ashutosh Bapat
> > >> There is a already a function to build targetlist for a given relation >> build_tlist_to_deparse(), which does the same thing as this code for a >> join or >> base relation and when there are no local conditions. Why don't we use >> that >> function instead of duplicating that logic? If

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-22 Thread Etsuro Fujita
Hi Rushabh, On 2016/11/22 19:05, Rushabh Lathia wrote: I started reviewing the patch and here are few initial review points and questions for you. Thanks for the review! 1) -static void deparseExplicitTargetList(List *tlist, List **retrieved_attrs, +static void

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-22 Thread Etsuro Fujita
On 2016/11/22 18:28, Ashutosh Bapat wrote: The comments should explain why is the assertion true. +/* Shouldn't be NIL */ +Assert(tlist != NIL); +/* Should be same length */ +Assert(list_length(tlist) == list_length(foreignrel->reltarget->exprs)); Will revise.

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-22 Thread Rushabh Lathia
I started reviewing the patch and here are few initial review points and questions for you. 1) -static void deparseExplicitTargetList(List *tlist, List **retrieved_attrs, +static void deparseExplicitTargetList(bool is_returning, + List *tlist, +

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-22 Thread Ashutosh Bapat
The comments should explain why is the assertion true. +/* Shouldn't be NIL */ +Assert(tlist != NIL); +/* Should be same length */ +Assert(list_length(tlist) == list_length(foreignrel->reltarget->exprs)); > > OK, I'd like to propose referencing to

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-21 Thread Etsuro Fujita
On 2016/11/21 22:02, Ashutosh Bapat wrote: You wrote: Instead we should calculate tlist, the first time we need it and then add it to the fpinfo. I wrote: Having said that, I agree on that point. I'd like to propose (1) adding a new member to fpinfo, to store a list of output Vars from the

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-21 Thread Ashutosh Bapat
> > Yeah, I modified the patch so, as I thought that would be consistent with > the aggregate pushdown patch. aggregate pushdown needs the tlist to judge the shippability of targetlist. For joins that's not required, so we should defer, if we can. > >>> Instead we should calculate tlist, the >>>

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-21 Thread Etsuro Fujita
On 2016/11/19 0:16, Ashutosh Bapat wrote: Also another point I guess, this note doesn't add much value in the given context. Probably we should remove it. +* Note: the tlist would have one-to-one correspondence with the +* joining relation's reltarget->exprs because (1)

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-18 Thread Ashutosh Bapat
Also another point I guess, this note doesn't add much value in the given context. Probably we should remove it. +* Note: the tlist would have one-to-one correspondence with the +* joining relation's reltarget->exprs because (1) the above test +* on PHVs

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-18 Thread Ashutosh Bapat
> > /* > * For a join relation or an upper relation, use > deparseExplicitTargetList. > * Likewise, for a base relation that is being deparsed as a subquery, > in > * which case the caller would have passed tlist that is non-NIL, use > that > * function. Otherwise, use

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-17 Thread Etsuro Fujita
On 2016/11/16 16:38, Etsuro Fujita wrote: On 2016/11/16 13:10, Ashutosh Bapat wrote: I don't see any reason why DML/UPDATE pushdown should depend upon subquery deparsing or least PHV patch. Combined together they can help in more cases, but without those patches, we will be able to push-down

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-16 Thread Etsuro Fujita
On 2016/11/16 18:14, Ashutosh Bapat wrote: (1) You added the following comments to deparseSelectSql: + /* +* For a non-base relation, use the input tlist. If a base relation is +* being deparsed as a subquery, use input tlist, if the caller has passed +* one. The

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-16 Thread Ashutosh Bapat
Thanks. > except for few things; (1) You added the > following comments to deparseSelectSql: > > + /* > +* For a non-base relation, use the input tlist. If a base relation > is > +* being deparsed as a subquery, use input tlist, if the caller has > passed > +* one.

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-15 Thread Etsuro Fujita
On 2016/11/16 13:10, Ashutosh Bapat wrote: On Wed, Nov 16, 2016 at 8:25 AM, Etsuro Fujita wrote: On 2016/11/15 19:04, Rushabh Lathia wrote: Your latest patch doesn't not get apply cleanly apply on master branch. Did you apply the patch set in [1]

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-15 Thread Ashutosh Bapat
On Wed, Nov 16, 2016 at 8:25 AM, Etsuro Fujita wrote: > On 2016/11/15 19:04, Rushabh Lathia wrote: >> >> Thanks Fujita-san for working on this. I've signed up to review this >> patch. > > > Thank you for reviewing the patch! > >> Your latest patch doesn't not get

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-15 Thread Etsuro Fujita
On 2016/11/15 19:04, Rushabh Lathia wrote: Thanks Fujita-san for working on this. I've signed up to review this patch. Thank you for reviewing the patch! Your latest patch doesn't not get apply cleanly apply on master branch. Did you apply the patch set in [1]

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-15 Thread Etsuro Fujita
On 2016/11/11 20:50, Etsuro Fujita wrote: On 2016/11/11 19:22, Ashutosh Bapat wrote: The patch looks in good shape now. Here are some comments. I have also made several changes to comments correcting grammar, typos, style and at few places logic. Let me know if the patch looks good. OK, will

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-15 Thread Rushabh Lathia
Thanks Fujita-san for working on this. I've signed up to review this patch. Your latest patch doesn't not get apply cleanly apply on master branch. patching file contrib/postgres_fdw/deparse.c 6 out of 17 hunks FAILED -- saving rejects to file contrib/postgres_fdw/deparse.c.rej patching file

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-11 Thread Etsuro Fujita
On 2016/11/11 19:22, Ashutosh Bapat wrote: The patch looks in good shape now. Thanks for the review! The patch looks in good shape now. Here are some comments. I have also made several changes to comments correcting grammar, typos, style and at few places logic. Let me know if the patch

Re: [HACKERS] Push down more UPDATEs/DELETEs in postgres_fdw

2016-11-11 Thread Etsuro Fujita
On 2016/09/08 19:55, Etsuro Fujita wrote: On 2016/09/07 13:21, Ashutosh Bapat wrote: * with the patch: postgres=# explain verbose delete from ft1 using ft2 where ft1.a = ft2.a; QUERY PLAN

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-11 Thread Ashutosh Bapat
On Mon, Nov 7, 2016 at 5:50 PM, Etsuro Fujita wrote: > On 2016/11/07 11:24, Etsuro Fujita wrote: >> >> On 2016/11/04 19:55, Etsuro Fujita wrote: >>> >>> Attached is an updated version of the patch. > > >> I noticed that I have included an unrelated regression test in

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-07 Thread Etsuro Fujita
On 2016/11/07 11:24, Etsuro Fujita wrote: On 2016/11/04 19:55, Etsuro Fujita wrote: Attached is an updated version of the patch. I noticed that I have included an unrelated regression test in the patch. Attached is a patch with the test removed. I noticed that I inadvertently removed some

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-06 Thread Etsuro Fujita
On 2016/11/04 19:55, Etsuro Fujita wrote: Attached is an updated version of the patch. I noticed that I have included an unrelated regression test in the patch. Attached is a patch with the test removed. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/deparse.c ---

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-04 Thread Etsuro Fujita
On 2016/11/04 13:08, Ashutosh Bapat wrote: On Fri, Nov 4, 2016 at 9:19 AM, Etsuro Fujita wrote: On 2016/11/03 18:52, Ashutosh Bapat wrote: I wrote: Here is the updated version, Other than the above issue and the alias issue we discussed, I addressed all your

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-03 Thread Ashutosh Bapat
On Fri, Nov 4, 2016 at 9:19 AM, Etsuro Fujita wrote: > On 2016/11/03 18:52, Ashutosh Bapat wrote: >>> >>> Here is the updated version, which includes the restructuring you >>> proposed. >>> Other than the above issue and the alias issue we discussed, I addressed >>>

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-03 Thread Etsuro Fujita
On 2016/11/03 18:52, Ashutosh Bapat wrote: Here is the updated version, which includes the restructuring you proposed. Other than the above issue and the alias issue we discussed, I addressed all your comments except one on testing; I tried to add test cases where the remote query is deparsed as

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-03 Thread Ashutosh Bapat
> Here is the updated version, which includes the restructuring you proposed. > Other than the above issue and the alias issue we discussed, I addressed all > your comments except one on testing; I tried to add test cases where the > remote query is deparsed as nested subqueries, but I couldn't

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-11-01 Thread Etsuro Fujita
On 2016/10/27 20:41, Etsuro Fujita wrote: Here is the updated version, which includes the restructuring you proposed. Other than the above issue and the alias issue we discussed, I addressed all your comments except one on testing; I tried to add test cases where the remote query is deparsed as

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-27 Thread Etsuro Fujita
On 2016/10/27 18:06, Ashutosh Bapat wrote: On Thu, Oct 27, 2016 at 12:46 PM, Etsuro Fujita wrote: On 2016/10/26 19:53, Ashutosh Bapat wrote: On Wed, Oct 26, 2016 at 3:35 PM, Etsuro Fujita My concern about your proposal is: it might not be worth complicating the

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-27 Thread Ashutosh Bapat
On Thu, Oct 27, 2016 at 12:46 PM, Etsuro Fujita wrote: > On 2016/10/26 19:53, Ashutosh Bapat wrote: >> >> On Wed, Oct 26, 2016 at 3:35 PM, Etsuro Fujita >> wrote: > > >>> In practice, the search cost would be negligible compared to the

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-27 Thread Etsuro Fujita
On 2016/10/26 19:53, Ashutosh Bapat wrote: On Wed, Oct 26, 2016 at 3:35 PM, Etsuro Fujita wrote: In practice, the search cost would be negligible compared to the cost of explaining/executing the query. My concern about your proposal is: it might not be worth

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-26 Thread Ashutosh Bapat
On Wed, Oct 26, 2016 at 3:35 PM, Etsuro Fujita wrote: > On 2016/10/26 18:25, Ashutosh Bapat wrote: > >> Your patch calls isSubqueryExpr() recursively for every Var in the >> targetlist, which can be many for foreign tables with many columns. >> For every such Var it

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-26 Thread Etsuro Fujita
On 2016/10/26 18:25, Ashutosh Bapat wrote: Your patch calls isSubqueryExpr() recursively for every Var in the targetlist, which can be many for foreign tables with many columns. For every such Var it may need to reach upto the relation which is converted into subquery, which can as bad as

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-26 Thread Ashutosh Bapat
> I guess, the arrays need to be computed only once for any relation when the query for that relation is deparsed the first time. > > >>> Does this algorithm extend to the case where we consider paths for every >>> join order? > > >> Yes, if we store the information about which of

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-26 Thread Etsuro Fujita
On 2016/10/26 16:11, Ashutosh Bapat wrote: You wrote: For example, let's assume that a relation (1, 2, 3) is required to be deparsed as subquery for an immediate upper relation (1, 2, 3, 4, 5) (thus the other joining relation being (4,5)). While deparsing for relation (1,2,3,4,5), the context

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-26 Thread Ashutosh Bapat
> > >> For every relation that is deparsed as a subquery, we will create a >> separate context. Each such context will have an array described >> above. This array will contain the targetlist and aliases for all the >> relations, covered by that relation, that are required to be deparsed >> as

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-25 Thread Etsuro Fujita
On 2016/10/25 18:58, Ashutosh Bapat wrote: You wrote: 13. The comment below is missing the main purpose i.e. creating a a unique alias, in case the relation gets converted into a subquery. Lowest or highest relid will create a unique alias at given level of join and that would be more future

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-25 Thread Ashutosh Bapat
> >> 13. The comment below is missing the main purpose i.e. creating a a unique >> alias, in case the relation gets converted into a subquery. Lowest or >> highest >> relid will create a unique alias at given level of join and that would be >> more >> future proof. If we decide to consider paths

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-24 Thread Etsuro Fujita
On 2016/10/22 17:19, Ashutosh Bapat wrote: Review for postgres-fdw-more-full-join-pushdown-v6 patch. The patch applies cleanly and regression is clean (make check in regress directory and that in postgres_fdw). Thanks for the review! Here are some comments. 1. Because of the following code

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-22 Thread Ashutosh Bapat
> 11. I have reworded following comment and restructured the code that follows > it > in the attached patch. > +/* > + * Set the subquery information. If the relation performs a full outer > + * join and if the input relations have non-NIL remote_conds, the input > + * relations

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-10-22 Thread Ashutosh Bapat
Review for postgres-fdw-more-full-join-pushdown-v6 patch. The patch applies cleanly and regression is clean (make check in regress directory and that in postgres_fdw). Here are some comments. 1. Because of the following code change, for a joinrel we might end up using attrs_used, which will be

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-29 Thread Etsuro Fujita
On 2016/09/28 18:35, Etsuro Fujita wrote: Attached is an updated version of the patch. I found a minor bug in that patch; the relation_index added to PgFdwRelationInfo was defined as Index, but I used the modifier %d to print that. So, I changed the data type to int. Also, I added a bit

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-28 Thread Etsuro Fujita
On 2016/09/26 16:30, Etsuro Fujita wrote: On 2016/09/13 14:17, Ashutosh Bapat wrote: It won't remain minimal as the number of paths created increases, increasing the number of times a query is deparsed. We deparse query every time time we cost a path for a relation with use_remote_estimates

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-28 Thread Etsuro Fujita
On 2016/09/27 13:33, Ashutosh Bapat wrote: I wrote: ISTM that the use of the same RTI for subqueries in multi-levels in a remote SQL makes the SQL a bit difficult to read. How about using the position of the join rel in join_rel_list, (more precisely, the position plus

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-26 Thread Ashutosh Bapat
On Tue, Sep 27, 2016 at 8:48 AM, Etsuro Fujita wrote: > On 2016/09/26 20:20, Ashutosh Bapat wrote: >> >> On Mon, Sep 26, 2016 at 4:06 PM, Etsuro Fujita >> wrote: >>> >>> On 2016/09/26 18:06, Ashutosh Bapat wrote: On Mon, Sep 26,

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-26 Thread Etsuro Fujita
On 2016/09/26 20:20, Ashutosh Bapat wrote: On Mon, Sep 26, 2016 at 4:06 PM, Etsuro Fujita wrote: On 2016/09/26 18:06, Ashutosh Bapat wrote: On Mon, Sep 26, 2016 at 1:05 PM, Etsuro Fujita wrote: ISTM that the use of the same RTI for

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-26 Thread Ashutosh Bapat
On Mon, Sep 26, 2016 at 4:06 PM, Etsuro Fujita wrote: > On 2016/09/26 18:06, Ashutosh Bapat wrote: >> >> On Mon, Sep 26, 2016 at 1:05 PM, Etsuro Fujita >> wrote: > > >>> ISTM that the use of the same RTI for subqueries in multi-levels in

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-26 Thread Etsuro Fujita
On 2016/09/26 18:06, Ashutosh Bapat wrote: On Mon, Sep 26, 2016 at 1:05 PM, Etsuro Fujita wrote: ISTM that the use of the same RTI for subqueries in multi-levels in a remote SQL makes the SQL a bit difficult to read. How about using the position of the join rel

Re: [HACKERS] Push down more full joins in postgres_fdw

2016-09-26 Thread Ashutosh Bapat
On Mon, Sep 26, 2016 at 1:05 PM, Etsuro Fujita wrote: > On 2016/09/15 15:29, Ashutosh Bapat wrote: >> >> On Wed, Sep 14, 2016 at 8:52 PM, Robert Haas >> wrote: > > >>> I'm not sure why it wouldn't work >>> to just use the lowest RTI involved in

  1   2   >