Re: [HACKERS] postgres_fdw: evaluate placeholdervars on remote server

[Etsuro Fujita]

On 2017/09/16 0:19, Robert Haas wrote:

On Fri, Sep 15, 2017 at 10:15 AM, Daniel Gustafsson  wrote:

Have you had a chance to look at this such that we can expect a rebased version
of this patch during the commitfest?


Frankly, I think things where there was a ping multiple weeks before
the CommitFest started and no rebase before it started should be
regarded as untimely submissions, and summarily marked Returned with
Feedback.  The CommitFest is supposed to be a time to get things that
are ready before it starts committed before it ends.  Some amount of
back-and-forth during the CF is of course to be expected, but we don't
even really have enough bandwidth to deal with the patches that are
being timely updated, never mind the ones that aren't.


Agreed.  I marked this as RWF.  Thank you.

Best regards,
Etsuro Fujita



--
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] postgres_fdw: evaluate placeholdervars on remote server

[Daniel Gustafsson]

> On 15 Sep 2017, at 17:19, Robert Haas  wrote:
> 
> On Fri, Sep 15, 2017 at 10:15 AM, Daniel Gustafsson  wrote:
>> Have you had a chance to look at this such that we can expect a rebased 
>> version
>> of this patch during the commitfest?
> 
> Frankly, I think things where there was a ping multiple weeks before
> the CommitFest started and no rebase before it started should be
> regarded as untimely submissions, and summarily marked Returned with
> Feedback.  The CommitFest is supposed to be a time to get things that
> are ready before it starts committed before it ends.  Some amount of
> back-and-forth during the CF is of course to be expected, but we don't
> even really have enough bandwidth to deal with the patches that are
> being timely updated, never mind the ones that aren’t.

I don’t necessarily disagree with this, and especially not the part about
bandwidth which is absolutely correct.

What has happened a lot however is that these patches have been moved to the
next CF, and possibly the next from there.  In that scenario, if we can get the
patches rebased now they wont be in a worse state when pushed to the next
compared to if they bitrot further.  That being said, perhaps we should move
closer to a model like what you describe, but thats for another thread to
discuss rather than threadjacking this one more IMO.

cheers ./daniel

-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Robert Haas]

On Fri, Sep 15, 2017 at 10:15 AM, Daniel Gustafsson  wrote:
> Have you had a chance to look at this such that we can expect a rebased 
> version
> of this patch during the commitfest?

Frankly, I think things where there was a ping multiple weeks before
the CommitFest started and no rebase before it started should be
regarded as untimely submissions, and summarily marked Returned with
Feedback.  The CommitFest is supposed to be a time to get things that
are ready before it starts committed before it ends.  Some amount of
back-and-forth during the CF is of course to be expected, but we don't
even really have enough bandwidth to deal with the patches that are
being timely updated, never mind the ones that aren't.

-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Daniel Gustafsson]

> On 15 Aug 2017, at 01:00, Peter Eisentraut  
> wrote:
> 
> On 4/3/17 22:00, Etsuro Fujita wrote:
>> On 2017/04/04 3:21, Andres Freund wrote:
>>> On 2017-02-28 21:45:22 +0900, Etsuro Fujita wrote:
 Here is a patch for $subject.
>>> 
>>> This is a nontrivial patch, submitted just before the start of the last
>>> CF for postgres 10.  Therefore I think we should move this to the next
>>> CF.
>> 
>> Honestly, I'm not satisfied with this patch and I think it would need 
>> more work.  Moved to the next CF.
> 
> This patch needs to be rebased for the upcoming commit fest.

Have you had a chance to look at this such that we can expect a rebased version
of this patch during the commitfest?

cheers ./daniel

-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Peter Eisentraut]

On 4/3/17 22:00, Etsuro Fujita wrote:
> On 2017/04/04 3:21, Andres Freund wrote:
>> On 2017-02-28 21:45:22 +0900, Etsuro Fujita wrote:
>>> Here is a patch for $subject.
>>
>> This is a nontrivial patch, submitted just before the start of the last
>> CF for postgres 10.  Therefore I think we should move this to the next
>> CF.
> 
> Honestly, I'm not satisfied with this patch and I think it would need 
> more work.  Moved to the next CF.

This patch needs to be rebased for the upcoming commit fest.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] postgres_fdw: evaluate placeholdervars on remote server

[Etsuro Fujita]

On 2017/04/04 3:21, Andres Freund wrote:

On 2017-02-28 21:45:22 +0900, Etsuro Fujita wrote:

Here is a patch for $subject.


This is a nontrivial patch, submitted just before the start of the last
CF for postgres 10.  Therefore I think we should move this to the next
CF.


Honestly, I'm not satisfied with this patch and I think it would need 
more work.  Moved to the next CF.


Best regards,
Etsuro Fujita




--
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] postgres_fdw: evaluate placeholdervars on remote server

[Andres Freund]

Hi,

On 2017-02-28 21:45:22 +0900, Etsuro Fujita wrote:
> Here is a patch for $subject.

This is a nontrivial patch, submitted just before the start of the last
CF for postgres 10.  Therefore I think we should move this to the next
CF.


- Andres


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

[HACKERS] postgres_fdw: evaluate placeholdervars on remote server

[Etsuro Fujita]

Hi,

Here is a patch for $subject.  This is the same as what I proposed in  
combination with a feature for full joins [1]; this would allow us to  
push down left/right/full joins with PHVs to the remote and improve how  
to deparse whole-row references.  Since this is implemented on top of  
the feature for full-joins (ie, the deparser logic for subqueries), I  
proposed this on that thread, but this is slightly independent from that  
feature (and we haven't discussed this in detail on that thread), so I  
think it's better to start new thread.  Attached is a new version, which  
is created on top of [2].  I'll add this to the upcoming CF.


Best regards,
Etsuro Fujita

[1]  
https://www.postgresql.org/message-id/c449261a-b033-dc02-9254-2fe5b7044795%40lab.ntt.co.jp
[2]  
https://www.postgresql.org/message-id/920e660b-6fec-6022-759d-e96e37dd5984%40lab.ntt.co.jp
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 49,54 
--- 49,55 
  #include "nodes/nodeFuncs.h"
  #include "nodes/plannodes.h"
  #include "optimizer/clauses.h"
+ #include "optimizer/placeholder.h"
  #include "optimizer/prep.h"
  #include "optimizer/tlist.h"
  #include "optimizer/var.h"
***
*** 158,163  static void deparseRelabelType(RelabelType *node, deparse_expr_cxt *context);
--- 159,165 
  static void deparseBoolExpr(BoolExpr *node, deparse_expr_cxt *context);
  static void deparseNullTest(NullTest *node, deparse_expr_cxt *context);
  static void deparseArrayExpr(ArrayExpr *node, deparse_expr_cxt *context);
+ static void deparseExprInPlaceHolderVar(PlaceHolderVar *node, deparse_expr_cxt *context);
  static void printRemoteParam(int paramindex, Oid paramtype, int32 paramtypmod,
   deparse_expr_cxt *context);
  static void printRemotePlaceholder(Oid paramtype, int32 paramtypmod,
***
*** 184,192  static Node *deparseSortGroupClause(Index ref, List *tlist,
  /*
   * Helper functions
   */
! static bool is_subquery_var(Var *node, RelOptInfo *foreignrel,
! 			int *relno, int *colno);
! static void get_relation_column_alias_ids(Var *node, RelOptInfo *foreignrel,
  		  int *relno, int *colno);
  
  
--- 186,195 
  /*
   * Helper functions
   */
! static bool is_subquery_expr(Expr *node, PlannerInfo *root,
! 			 RelOptInfo *foreignrel,
! 			 int *relno, int *colno);
! static void get_relation_column_alias_ids(Expr *node, RelOptInfo *foreignrel,
  		  int *relno, int *colno);
  
  
***
*** 771,776  foreign_expr_walker(Node *node,
--- 774,798 
  	state = FDW_COLLATE_UNSAFE;
  			}
  			break;
+ 		case T_PlaceHolderVar:
+ 			{
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+ PlaceHolderInfo *phinfo = find_placeholder_info(glob_cxt->root,
+ phv, false);
+ 
+ /*
+  * If the PHV's contained expression is computable on the
+  * remote server, we consider the PHV safe to send.
+  */
+ if (phv->phlevelsup == 0 &&
+ 	bms_is_subset(phinfo->ph_eval_at,
+   glob_cxt->foreignrel->relids))
+ 	return foreign_expr_walker((Node *) phv->phexpr,
+ 			   glob_cxt, outer_cxt);
+ else
+ 	return false;
+ 			}
+ 			break;
  		default:
  
  			/*
***
*** 865,871  deparse_type_name(Oid type_oid, int32 typemod)
   * foreign server.
   */
  List *
! build_tlist_to_deparse(RelOptInfo *foreignrel)
  {
  	List	   *tlist = NIL;
  	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
--- 887,893 
   * foreign server.
   */
  List *
! build_tlist_to_deparse(PlannerInfo *root, RelOptInfo *foreignrel)
  {
  	List	   *tlist = NIL;
  	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
***
*** 878,892  build_tlist_to_deparse(RelOptInfo *foreignrel)
  		return fpinfo->grouped_tlist;
  
  	/*
! 	 * We require columns specified in foreignrel->reltarget->exprs and those
! 	 * required for evaluating the local conditions.
  	 */
! 	tlist = add_to_flat_tlist(tlist,
! 	   pull_var_clause((Node *) foreignrel->reltarget->exprs,
! 	   PVC_RECURSE_PLACEHOLDERS));
! 	tlist = add_to_flat_tlist(tlist,
! 			  pull_var_clause((Node *) fpinfo->local_conds,
! 			  PVC_RECURSE_PLACEHOLDERS));
  
  	return tlist;
  }
--- 900,972 
  		return fpinfo->grouped_tlist;
  
  	/*
! 	 * Fetch all expressions in the given relation's reltarget if the
! 	 * reltarget_is_shippable flag is set TRUE.  Otherwise, fetch shipplable
! 	 * expressions in the reltarget plus expressions required for evaluating
! 	 * non-shippable expressions in the reltarget.
  	 */
! 	if (fpinfo->reltarget_is_shippable)
! 		tlist = add_to_flat_tlist(tlist, foreignrel->reltarget->exprs);
! 	else
! 	{
! 		List	   *exprs = NIL;
! 		ListCell   *lc;
! 
! 		/* Note: we have at least one non-shippable PHV in the reltarget. */
! 		foreach(lc, foreignrel->reltarget->exprs)
! 		{
! 			Node	   *node = (Node *) lfirst(lc);
!