Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/07 8:25), Robert Haas wrote:

On Thu, Apr 5, 2018 at 6:21 AM, Etsuro Fujita
  wrote:

Attached is an updated version of the patch set plus the patch in [1]. Patch
0003_foreign-routing-fdwapi-6.patch can be applied on top of patch
0001_postgres-fdw-refactoring-6.patch and
0002_copy-from-check-constraint-fix.patch.


Committed.  Let me say that you do nice work.


Thanks to Robert for committing!  Thanks to everyone who got involved in 
this patch for all of the help with review and commentary!


Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

On Sun, Apr 8, 2018 at 5:37 AM, Andres Freund  wrote:
> On 2018-04-06 19:25:20 -0400, Robert Haas wrote:
>> On Thu, Apr 5, 2018 at 6:21 AM, Etsuro Fujita
>>  wrote:
>> > Attached is an updated version of the patch set plus the patch in [1]. 
>> > Patch
>> > 0003_foreign-routing-fdwapi-6.patch can be applied on top of patch
>> > 0001_postgres-fdw-refactoring-6.patch and
>> > 0002_copy-from-check-constraint-fix.patch.
>>
>> Committed.  Let me say that you do nice work.
>
> The CF entry for this is still marked as 'ready for committer'. Does anything 
> remain?
>
> https://commitfest.postgresql.org/17/1184/

Nothing remains, so marked as committed.

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Andres Freund]

On 2018-04-06 19:25:20 -0400, Robert Haas wrote:
> On Thu, Apr 5, 2018 at 6:21 AM, Etsuro Fujita
>  wrote:
> > Attached is an updated version of the patch set plus the patch in [1]. Patch
> > 0003_foreign-routing-fdwapi-6.patch can be applied on top of patch
> > 0001_postgres-fdw-refactoring-6.patch and
> > 0002_copy-from-check-constraint-fix.patch.
> 
> Committed.  Let me say that you do nice work.

The CF entry for this is still marked as 'ready for committer'. Does anything 
remain?

https://commitfest.postgresql.org/17/1184/

Greetings,

Andres Freund

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Thu, Apr 5, 2018 at 6:21 AM, Etsuro Fujita
 wrote:
> Attached is an updated version of the patch set plus the patch in [1]. Patch
> 0003_foreign-routing-fdwapi-6.patch can be applied on top of patch
> 0001_postgres-fdw-refactoring-6.patch and
> 0002_copy-from-check-constraint-fix.patch.

Committed.  Let me say that you do nice work.

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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/05 16:31), Amit Langote wrote:

Might be a good idea to attach the bug-fix patch here as well, and perhaps
add numbers to the file names like:

0001_postgres-fdw-refactoring-5.patch
0002_BUGFIX-copy-from-check-constraint-fix.patch
0003_foreign-routing-fdwapi-5.patch


OK


Just one minor comment:

I wonder why you decided not to have the CheckValidResultRel() call and
the statement that sets ri_PartitionReadyForRouting inside the newly added
ExecInitRoutingInfo itself.  If ExecInitRoutingInfo does the last
necessary steps for a ResultRelInfo (and hence the partition) to be ready
to be used for routing, why not finish everything there.  So the changes
to ExecPrepareTupleRouting which look like this in the patch:

+if (!partrel->ri_PartitionReadyForRouting)
+{
+CheckValidResultRel(partrel, CMD_INSERT);
+
+/* Set up information needed for routing tuples to the partition */
+ExecInitRoutingInfo(mtstate, estate, proute, partrel, partidx);
+
+partrel->ri_PartitionReadyForRouting = true;
+}

will become:

+if (!partrel->ri_PartitionReadyForRouting)
+ExecInitRoutingInfo(mtstate, estate, proute, partrel, partidx);


Good idea!  Modified that way.


As I see no other issues, I will mark this as Ready for Committer.


Thanks!

Attached is an updated version of the patch set plus the patch in [1]. 
Patch 0003_foreign-routing-fdwapi-6.patch can be applied on top of patch 
0001_postgres-fdw-refactoring-6.patch and 
0002_copy-from-check-constraint-fix.patch.


Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5aba4074.1090...@lab.ntt.co.jp
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 376,387  static bool ec_member_matches_foreign(PlannerInfo *root, RelOptInfo *rel,
--- 376,396 
  static void create_cursor(ForeignScanState *node);
  static void fetch_more_data(ForeignScanState *node);
  static void close_cursor(PGconn *conn, unsigned int cursor_number);
+ static PgFdwModifyState *create_foreign_modify(EState *estate,
+ 	  ResultRelInfo *resultRelInfo,
+ 	  CmdType operation,
+ 	  Plan *subplan,
+ 	  char *query,
+ 	  List *target_attrs,
+ 	  bool has_returning,
+ 	  List *retrieved_attrs);
  static void prepare_foreign_modify(PgFdwModifyState *fmstate);
  static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
  		 ItemPointer tupleid,
  		 TupleTableSlot *slot);
  static void store_returning_result(PgFdwModifyState *fmstate,
  	   TupleTableSlot *slot, PGresult *res);
+ static void finish_foreign_modify(PgFdwModifyState *fmstate);
  static List *build_remote_returning(Index rtindex, Relation rel,
  	   List *returningList);
  static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist);
***
*** 1681,1698  postgresBeginForeignModify(ModifyTableState *mtstate,
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	EState	   *estate = mtstate->ps.state;
! 	CmdType		operation = mtstate->operation;
! 	Relation	rel = resultRelInfo->ri_RelationDesc;
! 	RangeTblEntry *rte;
! 	Oid			userid;
! 	ForeignTable *table;
! 	UserMapping *user;
! 	AttrNumber	n_params;
! 	Oid			typefnoid;
! 	bool		isvarlena;
! 	ListCell   *lc;
! 	TupleDesc	tupdesc = RelationGetDescr(rel);
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
--- 1690,1699 
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	char	   *query;
! 	List	   *target_attrs;
! 	bool		has_returning;
! 	List	   *retrieved_attrs;
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
***
*** 1701,1782  postgresBeginForeignModify(ModifyTableState *mtstate,
  	if (eflags & EXEC_FLAG_EXPLAIN_ONLY)
  		return;
  
- 	/* Begin constructing PgFdwModifyState. */
- 	fmstate = (PgFdwModifyState *) palloc0(sizeof(PgFdwModifyState));
- 	fmstate->rel = rel;
- 
- 	/*
- 	 * Identify which user to do the remote access as.  This should match what
- 	 * ExecCheckRTEPerms() does.
- 	 */
- 	rte = rt_fetch(resultRelInfo->ri_RangeTableIndex, estate->es_range_table);
- 	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- 
- 	/* Get info about foreign table. */
- 	table = GetForeignTable(RelationGetRelid(rel));
- 	user = GetUserMapping(userid, table->serverid);
- 
- 	/* Open connection; report that we'll create a prepared statement. */
- 	fmstate->conn = GetConnection(user, true);
- 	fmstate->p_name = NULL;		/* prepared statement not made yet */
- 
  	/* Deconstruct fdw_private data. */
! 	fmstate->query = strVal(list_nth(fdw_private,
! 	 FdwModifyPrivateUpdateSql));
! 	fmstate->target_attrs = (List *) list_nth(fdw_private,
! 			  FdwModifyPrivateTargetAttnums);
! 	fmstate->has_returning = intVal(list_nth(fdw_private,
! 			 FdwModifyPrivateHasReturning));
! 	fmstate->retrieved_attrs = (List *) list_nth(fdw_private,
!  

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

On 2018/04/05 16:31, Amit Langote wrote:
> Fuiita-san,

Oops, sorry about misspelling your name here, Fujita-san.

- Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fuiita-san,

On 2018/04/05 15:56, Etsuro Fujita wrote:
> (2018/04/05 15:37), Amit Langote wrote:
>> I noticed that the 2nd patch (foreign-routing-fdwapi-5.patch) fails to
>> apply to copy.c:
> 
> I forgot to mention this: the second patch is created on top of the first
> patch (postgres-fdw-refactoring-5.patch) and the patch in [1] as before.

Ah, sorry I hadn't noticed that in your previous email.

Might be a good idea to attach the bug-fix patch here as well, and perhaps
add numbers to the file names like:

0001_postgres-fdw-refactoring-5.patch
0002_BUGFIX-copy-from-check-constraint-fix.patch
0003_foreign-routing-fdwapi-5.patch


Just one minor comment:

I wonder why you decided not to have the CheckValidResultRel() call and
the statement that sets ri_PartitionReadyForRouting inside the newly added
ExecInitRoutingInfo itself.  If ExecInitRoutingInfo does the last
necessary steps for a ResultRelInfo (and hence the partition) to be ready
to be used for routing, why not finish everything there.  So the changes
to ExecPrepareTupleRouting which look like this in the patch:

+if (!partrel->ri_PartitionReadyForRouting)
+{
+CheckValidResultRel(partrel, CMD_INSERT);
+
+/* Set up information needed for routing tuples to the partition */
+ExecInitRoutingInfo(mtstate, estate, proute, partrel, partidx);
+
+partrel->ri_PartitionReadyForRouting = true;
+}

will become:

+if (!partrel->ri_PartitionReadyForRouting)
+ExecInitRoutingInfo(mtstate, estate, proute, partrel, partidx);


As I see no other issues, I will mark this as Ready for Committer.

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/05 15:37), Amit Langote wrote:

On 2018/04/05 15:02, Etsuro Fujita wrote:

(2018/04/04 19:31), Etsuro Fujita wrote:

Attached is an updated version of the patch set:
* As before, patch foreign-routing-fdwapi-4.patch is created on top of
patch postgres-fdw-refactoring-4.patch and the bug-fix patch [1].


I did a bit of cleanup and comment-rewording to patch
foreign-routing-fdwapi-4.patch.  Attached is a new version of the patch
set.  I renamed the postgres_fdw refactoring patch but no changes to that
patch.


I noticed that the 2nd patch (foreign-routing-fdwapi-5.patch) fails to
apply to copy.c:

Hunk #9 FAILED at 2719.
Hunk #10 succeeded at 2752 (offset -1 lines).
Hunk #11 succeeded at 2795 (offset -1 lines).
Hunk #12 succeeded at 2843 (offset -1 lines).
1 out of 12 hunks FAILED -- saving rejects to file
src/backend/commands/copy.c.rej

cat src/backend/commands/copy.c.rej
*** src/backend/commands/copy.c
--- src/backend/commands/copy.c
***
*** 2719,2727 
  
resultRelInfo->ri_TrigDesc->trig_insert_before_row))
check_partition_constr = false;

!   /* Check the constraints of the tuple */
!   if 
(resultRelInfo->ri_RelationDesc->rd_att->constr ||
!   check_partition_constr)
ExecConstraints(resultRelInfo, slot, 
estate, true);

if (useHeapMultiInsert)
--- 2730,2742 
  
resultRelInfo->ri_TrigDesc->trig_insert_before_row))
check_partition_constr = false;

!   /*
!* If the target is a plain table, check the 
constraints of
!* the tuple.
!*/
!   if (resultRelInfo->ri_FdwRoutine == NULL&&
!   
(resultRelInfo->ri_RelationDesc->rd_att->constr ||
!check_partition_constr))
ExecConstraints(resultRelInfo, slot, 
estate, true);

if (useHeapMultiInsert)

Can you check?


I forgot to mention this: the second patch is created on top of the 
first patch (postgres-fdw-refactoring-5.patch) and the patch in [1] as 
before.


Thanks for reviewing, Amit!

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5aba4074.1090...@lab.ntt.co.jp

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

On 2018/04/05 15:02, Etsuro Fujita wrote:
> (2018/04/04 19:31), Etsuro Fujita wrote:
>> Attached is an updated version of the patch set:
>> * As before, patch foreign-routing-fdwapi-4.patch is created on top of
>> patch postgres-fdw-refactoring-4.patch and the bug-fix patch [1].
> 
> I did a bit of cleanup and comment-rewording to patch
> foreign-routing-fdwapi-4.patch.  Attached is a new version of the patch
> set.  I renamed the postgres_fdw refactoring patch but no changes to that
> patch.

I noticed that the 2nd patch (foreign-routing-fdwapi-5.patch) fails to
apply to copy.c:

Hunk #9 FAILED at 2719.
Hunk #10 succeeded at 2752 (offset -1 lines).
Hunk #11 succeeded at 2795 (offset -1 lines).
Hunk #12 succeeded at 2843 (offset -1 lines).
1 out of 12 hunks FAILED -- saving rejects to file
src/backend/commands/copy.c.rej

cat src/backend/commands/copy.c.rej
*** src/backend/commands/copy.c
--- src/backend/commands/copy.c
***
*** 2719,2727 
  
resultRelInfo->ri_TrigDesc->trig_insert_before_row))
check_partition_constr = false;

!   /* Check the constraints of the tuple */
!   if 
(resultRelInfo->ri_RelationDesc->rd_att->constr ||
!   check_partition_constr)
ExecConstraints(resultRelInfo, slot, 
estate, true);

if (useHeapMultiInsert)
--- 2730,2742 
  
resultRelInfo->ri_TrigDesc->trig_insert_before_row))
check_partition_constr = false;

!   /*
!* If the target is a plain table, check the 
constraints of
!* the tuple.
!*/
!   if (resultRelInfo->ri_FdwRoutine == NULL &&
!   
(resultRelInfo->ri_RelationDesc->rd_att->constr ||
!check_partition_constr))
ExecConstraints(resultRelInfo, slot, 
estate, true);

if (useHeapMultiInsert)

Can you check?

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/04 19:31), Etsuro Fujita wrote:

Attached is an updated version of the patch set:
* As before, patch foreign-routing-fdwapi-4.patch is created on top of
patch postgres-fdw-refactoring-4.patch and the bug-fix patch [1].


I did a bit of cleanup and comment-rewording to patch 
foreign-routing-fdwapi-4.patch.  Attached is a new version of the patch 
set.  I renamed the postgres_fdw refactoring patch but no changes to 
that patch.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 376,387  static bool ec_member_matches_foreign(PlannerInfo *root, RelOptInfo *rel,
--- 376,396 
  static void create_cursor(ForeignScanState *node);
  static void fetch_more_data(ForeignScanState *node);
  static void close_cursor(PGconn *conn, unsigned int cursor_number);
+ static PgFdwModifyState *create_foreign_modify(EState *estate,
+ 	  ResultRelInfo *resultRelInfo,
+ 	  CmdType operation,
+ 	  Plan *subplan,
+ 	  char *query,
+ 	  List *target_attrs,
+ 	  bool has_returning,
+ 	  List *retrieved_attrs);
  static void prepare_foreign_modify(PgFdwModifyState *fmstate);
  static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
  		 ItemPointer tupleid,
  		 TupleTableSlot *slot);
  static void store_returning_result(PgFdwModifyState *fmstate,
  	   TupleTableSlot *slot, PGresult *res);
+ static void finish_foreign_modify(PgFdwModifyState *fmstate);
  static List *build_remote_returning(Index rtindex, Relation rel,
  	   List *returningList);
  static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist);
***
*** 1681,1698  postgresBeginForeignModify(ModifyTableState *mtstate,
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	EState	   *estate = mtstate->ps.state;
! 	CmdType		operation = mtstate->operation;
! 	Relation	rel = resultRelInfo->ri_RelationDesc;
! 	RangeTblEntry *rte;
! 	Oid			userid;
! 	ForeignTable *table;
! 	UserMapping *user;
! 	AttrNumber	n_params;
! 	Oid			typefnoid;
! 	bool		isvarlena;
! 	ListCell   *lc;
! 	TupleDesc	tupdesc = RelationGetDescr(rel);
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
--- 1690,1699 
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	char	   *query;
! 	List	   *target_attrs;
! 	bool		has_returning;
! 	List	   *retrieved_attrs;
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
***
*** 1701,1782  postgresBeginForeignModify(ModifyTableState *mtstate,
  	if (eflags & EXEC_FLAG_EXPLAIN_ONLY)
  		return;
  
- 	/* Begin constructing PgFdwModifyState. */
- 	fmstate = (PgFdwModifyState *) palloc0(sizeof(PgFdwModifyState));
- 	fmstate->rel = rel;
- 
- 	/*
- 	 * Identify which user to do the remote access as.  This should match what
- 	 * ExecCheckRTEPerms() does.
- 	 */
- 	rte = rt_fetch(resultRelInfo->ri_RangeTableIndex, estate->es_range_table);
- 	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- 
- 	/* Get info about foreign table. */
- 	table = GetForeignTable(RelationGetRelid(rel));
- 	user = GetUserMapping(userid, table->serverid);
- 
- 	/* Open connection; report that we'll create a prepared statement. */
- 	fmstate->conn = GetConnection(user, true);
- 	fmstate->p_name = NULL;		/* prepared statement not made yet */
- 
  	/* Deconstruct fdw_private data. */
! 	fmstate->query = strVal(list_nth(fdw_private,
! 	 FdwModifyPrivateUpdateSql));
! 	fmstate->target_attrs = (List *) list_nth(fdw_private,
! 			  FdwModifyPrivateTargetAttnums);
! 	fmstate->has_returning = intVal(list_nth(fdw_private,
! 			 FdwModifyPrivateHasReturning));
! 	fmstate->retrieved_attrs = (List *) list_nth(fdw_private,
!  FdwModifyPrivateRetrievedAttrs);
! 
! 	/* Create context for per-tuple temp workspace. */
! 	fmstate->temp_cxt = AllocSetContextCreate(estate->es_query_cxt,
! 			  "postgres_fdw temporary data",
! 			  ALLOCSET_SMALL_SIZES);
! 
! 	/* Prepare for input conversion of RETURNING results. */
! 	if (fmstate->has_returning)
! 		fmstate->attinmeta = TupleDescGetAttInMetadata(tupdesc);
! 
! 	/* Prepare for output conversion of parameters used in prepared stmt. */
! 	n_params = list_length(fmstate->target_attrs) + 1;
! 	fmstate->p_flinfo = (FmgrInfo *) palloc0(sizeof(FmgrInfo) * n_params);
! 	fmstate->p_nums = 0;
! 
! 	if (operation == CMD_UPDATE || operation == CMD_DELETE)
! 	{
! 		/* Find the ctid resjunk column in the subplan's result */
! 		Plan	   *subplan = mtstate->mt_plans[subplan_index]->plan;
! 
! 		fmstate->ctidAttno = ExecFindJunkAttributeInTlist(subplan->targetlist,
! 		  "ctid");
! 		if (!AttributeNumberIsValid(fmstate->ctidAttno))
! 			elog(ERROR, "could not find junk ctid column");
  
! 		/* First transmittable parameter will be ctid */
! 		getTypeOutputInfo(TIDOID, , );
! 		fmgr_info(typefnoid, >p_flinfo[fmstate->p_nums]);
! 		

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/03 22:01), Etsuro Fujita wrote:

Attached is an updated version of the patch. Patch
foreign-routing-fdwapi-3.patch is created on top of patch
postgres-fdw-refactoring-3.patch and the bug-fix patch [1].


One thing I noticed about patch foreign-routing-fdwapi-3.patch is this 
bug: the server will crash when copying data into a foreign table that 
doesn't support the proposed APIs (eg, file_fdw foreign tables).  The 
reason is that the patch doesn't perform CheckValidResultRel before that 
operation in that case.  So I modified the patch as such and added 
regression tests for that.


Attached is an updated version of the patch set:
* As before, patch foreign-routing-fdwapi-4.patch is created on top of 
patch postgres-fdw-refactoring-4.patch and the bug-fix patch [1].
* I revised comments, docs, and regression tests a bit further, but no 
code changes other than the bug fix.


Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5aba4074.1090...@lab.ntt.co.jp
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 376,387  static bool ec_member_matches_foreign(PlannerInfo *root, RelOptInfo *rel,
--- 376,396 
  static void create_cursor(ForeignScanState *node);
  static void fetch_more_data(ForeignScanState *node);
  static void close_cursor(PGconn *conn, unsigned int cursor_number);
+ static PgFdwModifyState *create_foreign_modify(EState *estate,
+ 	  ResultRelInfo *resultRelInfo,
+ 	  CmdType operation,
+ 	  Plan *subplan,
+ 	  char *query,
+ 	  List *target_attrs,
+ 	  bool has_returning,
+ 	  List *retrieved_attrs);
  static void prepare_foreign_modify(PgFdwModifyState *fmstate);
  static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
  		 ItemPointer tupleid,
  		 TupleTableSlot *slot);
  static void store_returning_result(PgFdwModifyState *fmstate,
  	   TupleTableSlot *slot, PGresult *res);
+ static void finish_foreign_modify(PgFdwModifyState *fmstate);
  static List *build_remote_returning(Index rtindex, Relation rel,
  	   List *returningList);
  static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist);
***
*** 1681,1698  postgresBeginForeignModify(ModifyTableState *mtstate,
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	EState	   *estate = mtstate->ps.state;
! 	CmdType		operation = mtstate->operation;
! 	Relation	rel = resultRelInfo->ri_RelationDesc;
! 	RangeTblEntry *rte;
! 	Oid			userid;
! 	ForeignTable *table;
! 	UserMapping *user;
! 	AttrNumber	n_params;
! 	Oid			typefnoid;
! 	bool		isvarlena;
! 	ListCell   *lc;
! 	TupleDesc	tupdesc = RelationGetDescr(rel);
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
--- 1690,1699 
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	char	   *query;
! 	List	   *target_attrs;
! 	bool		has_returning;
! 	List	   *retrieved_attrs;
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
***
*** 1701,1782  postgresBeginForeignModify(ModifyTableState *mtstate,
  	if (eflags & EXEC_FLAG_EXPLAIN_ONLY)
  		return;
  
- 	/* Begin constructing PgFdwModifyState. */
- 	fmstate = (PgFdwModifyState *) palloc0(sizeof(PgFdwModifyState));
- 	fmstate->rel = rel;
- 
- 	/*
- 	 * Identify which user to do the remote access as.  This should match what
- 	 * ExecCheckRTEPerms() does.
- 	 */
- 	rte = rt_fetch(resultRelInfo->ri_RangeTableIndex, estate->es_range_table);
- 	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- 
- 	/* Get info about foreign table. */
- 	table = GetForeignTable(RelationGetRelid(rel));
- 	user = GetUserMapping(userid, table->serverid);
- 
- 	/* Open connection; report that we'll create a prepared statement. */
- 	fmstate->conn = GetConnection(user, true);
- 	fmstate->p_name = NULL;		/* prepared statement not made yet */
- 
  	/* Deconstruct fdw_private data. */
! 	fmstate->query = strVal(list_nth(fdw_private,
! 	 FdwModifyPrivateUpdateSql));
! 	fmstate->target_attrs = (List *) list_nth(fdw_private,
! 			  FdwModifyPrivateTargetAttnums);
! 	fmstate->has_returning = intVal(list_nth(fdw_private,
! 			 FdwModifyPrivateHasReturning));
! 	fmstate->retrieved_attrs = (List *) list_nth(fdw_private,
!  FdwModifyPrivateRetrievedAttrs);
! 
! 	/* Create context for per-tuple temp workspace. */
! 	fmstate->temp_cxt = AllocSetContextCreate(estate->es_query_cxt,
! 			  "postgres_fdw temporary data",
! 			  ALLOCSET_SMALL_SIZES);
! 
! 	/* Prepare for input conversion of RETURNING results. */
! 	if (fmstate->has_returning)
! 		fmstate->attinmeta = TupleDescGetAttInMetadata(tupdesc);
! 
! 	/* Prepare for output conversion of parameters used in prepared stmt. */
! 	n_params = list_length(fmstate->target_attrs) + 1;
! 	fmstate->p_flinfo = (FmgrInfo *) palloc0(sizeof(FmgrInfo) * n_params);
! 	fmstate->p_nums = 0;
! 
! 	if (operation == CMD_UPDATE 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/03 13:59), Amit Langote wrote:

On 2018/04/02 21:29, Etsuro Fujita wrote:

(2018/04/02 18:49), Amit Langote wrote:

I looked at the new patch.  It looks good overall, although I have one
question -- IIUC, BeginForeignInsert() performs actions that are
equivalent of performing PlanForeignModify() + BeginForeignModify() for an
INSERT as if it was directly executed on a given foreign table/partition.
Did you face any problems in doing the latter itself in the core code?
Doing it that way would mean no changes to a FDW itself will be required
(and hence no need for additional APIs), but I may be missing something.


The biggest issue in performing PlanForeignModify() plus
BeginForeignModify() instead of the new FDW API would be: can the core
code create a valid-looking planner state passed to PlanForeignModify()
such as the ModifyTable plan node or the query tree stored in PlannerInfo?


Hmm, I can see the point.  Passing mostly-dummy (garbage) PlannerInfo and
query tree from the core code to FDW seems bad.  By defining the new API
with a clean interface (receiving fully valid ModifyTableState), we're not
required to do that across the interface, but only inside the FDW's
implementation.


I really think so too.


I was just slightly concerned that the new FDW function
would have to implement what would normally be carried out across multiple
special purpose callbacks, but maybe that's OK as long as it's clearly
documented what its job is.


OK


Noticed a couple of things in the patch:

+
+  When this is called by aCOPY FROM  command, the
+  plan-related global data inmtstate  is not provided
+  and theplanSlot  parameter of
+ExecForeignInsert  called for each inserted
tuple is

How about s/called/subsequently called/g?


Done.


+NULL, wether the foreign table is the partition
chosen

Typo: s/wether/whether/g


Done.

Thanks again, Amit!

Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/03 13:32), Amit Langote wrote:

On 2018/04/02 21:26, Etsuro Fujita wrote:

We wouldn't need that for foreign partitions (When DO NOTHING with an
inference specification or DO UPDATE on a partitioned table containing
foreign partitions, the planner would throw an error before we get to
ExecInitPartitionInfo).


Actually, as you might know, since it is not possible to create an index
on a partitioned table that has a foreign partition, there is no
possibility of supporting any form of ON CONFLICT that requires an
inference specification.


Right.


The reason why I updated the patch that way is
just to make the patch simple, but on reflection I don't think that's a
good idea; I think we should delay the map-creation step as well as the
FDW-initialization step for UPDATE subplan partitions as before for
improved efficiency for UPDATE-of-partition-key.  However, I don't think
it'd still be a good idea to delay those steps for partitions created by
ExecInitPartitionInfo the same way as for the subplan partitions, because
in that case it'd be better to perform CheckValidResultRel against a
partition right after we do InitResultRelInfo for the partition in
ExecInitPartitionInfo, as that would save cycles in cases when the
partition is considered nonvalid by that check.


It seems like a good idea to perform CheckValidResultRel right after the
InitResultRelInfo in ExecInitPartitionInfo.  However, if the partition is
considered non-valid, that results in an error afaik, so I don't
understand the point about saving cycles.


I think that we could abort the query without doing the remaining work 
after the check in ExecInitPartitionInfo in that case.



So, What I'm thinking is:
a) for the subplan partitions, we delay those steps as before, and b) for
the partitions created by ExecInitPartitionInfo, we do that check for a
partition right after InitResultRelInfo in that function, (and if valid,
we create a map and initialize the FDW for the partition in that function).


Sounds good to me.  I'm assuming that you're talking about delaying the
is-valid-for-insert-routing check (using CheckValidResultRel) and
parent-to-child map creation for a sub-plan result relation until
ExecPrepareTupleRouting().


That's exactly what I have in mind.  I modified the patch that way.


On a related note, I wonder if it'd be a good idea to rename the flag
ri_PartitionIsValid to something that signifies that we only need it to be
true if we want to do tuple routing (aka insert) using it.  Maybe,
ri_PartitionReadyForRouting or ri_PartitionReadyForInsert.  I'm afraid
that ri_PartitionIsValid is a bit ambiguous, although I'm not saying the
name options I'm suggesting are the best.


That's a good idea!  I like the first one, so I changed the name to that.

Thanks for the review!

Attached is an updated version of the patch.  Patch 
foreign-routing-fdwapi-3.patch is created on top of patch 
postgres-fdw-refactoring-3.patch and the bug-fix patch [1].


Other changes:
* Fixed/revised docs as you pointed out in another post.
* Added docs to update.sgml
* Revised some code/comments a little bit
* Revised regression tests
* Rebased against the latest HEAD

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5aba4074.1090...@lab.ntt.co.jp
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 376,387  static bool ec_member_matches_foreign(PlannerInfo *root, RelOptInfo *rel,
--- 376,396 
  static void create_cursor(ForeignScanState *node);
  static void fetch_more_data(ForeignScanState *node);
  static void close_cursor(PGconn *conn, unsigned int cursor_number);
+ static PgFdwModifyState *create_foreign_modify(EState *estate,
+ 	  ResultRelInfo *resultRelInfo,
+ 	  CmdType operation,
+ 	  Plan *subplan,
+ 	  char *query,
+ 	  List *target_attrs,
+ 	  bool has_returning,
+ 	  List *retrieved_attrs);
  static void prepare_foreign_modify(PgFdwModifyState *fmstate);
  static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
  		 ItemPointer tupleid,
  		 TupleTableSlot *slot);
  static void store_returning_result(PgFdwModifyState *fmstate,
  	   TupleTableSlot *slot, PGresult *res);
+ static void finish_foreign_modify(PgFdwModifyState *fmstate);
  static List *build_remote_returning(Index rtindex, Relation rel,
  	   List *returningList);
  static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist);
***
*** 1681,1698  postgresBeginForeignModify(ModifyTableState *mtstate,
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	EState	   *estate = mtstate->ps.state;
! 	CmdType		operation = mtstate->operation;
! 	Relation	rel = resultRelInfo->ri_RelationDesc;
! 	RangeTblEntry *rte;
! 	Oid			userid;
! 	ForeignTable *table;
! 	UserMapping *user;
! 	AttrNumber	n_params;
! 	Oid			typefnoid;
! 	bool		isvarlena;
! 	ListCell   *lc;
! 	TupleDesc	tupdesc = RelationGetDescr(rel);
  
  	/*
  	 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

On 2018/04/02 21:29, Etsuro Fujita wrote:
> (2018/04/02 18:49), Amit Langote wrote:
>> I looked at the new patch.  It looks good overall, although I have one
>> question -- IIUC, BeginForeignInsert() performs actions that are
>> equivalent of performing PlanForeignModify() + BeginForeignModify() for an
>> INSERT as if it was directly executed on a given foreign table/partition.
>> Did you face any problems in doing the latter itself in the core code?
>> Doing it that way would mean no changes to a FDW itself will be required
>> (and hence no need for additional APIs), but I may be missing something.
> 
> The biggest issue in performing PlanForeignModify() plus
> BeginForeignModify() instead of the new FDW API would be: can the core
> code create a valid-looking planner state passed to PlanForeignModify()
> such as the ModifyTable plan node or the query tree stored in PlannerInfo?

Hmm, I can see the point.  Passing mostly-dummy (garbage) PlannerInfo and
query tree from the core code to FDW seems bad.  By defining the new API
with a clean interface (receiving fully valid ModifyTableState), we're not
required to do that across the interface, but only inside the FDW's
implementation.  I was just slightly concerned that the new FDW function
would have to implement what would normally be carried out across multiple
special purpose callbacks, but maybe that's OK as long as it's clearly
documented what its job is.

Noticed a couple of things in the patch:

+ 
+  When this is called by a COPY FROM command, the
+  plan-related global data in mtstate is not provided
+  and the planSlot parameter of
+  ExecForeignInsert called for each inserted
tuple is

How about s/called/subsequently called/g?

+  NULL, wether the foreign table is the partition
chosen

Typo: s/wether/whether/g

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

On 2018/04/02 21:26, Etsuro Fujita wrote:
> (2018/03/30 22:28), Alvaro Herrera wrote:
>> Etsuro Fujita wrote:
>>
>>> Now we have ON CONFLICT for partitioned tables, which requires the
>>> conversion map to be computed in ExecInitPartitionInfo, so I updated the
>>> patch so that we keep the former step in ExecInitPartitionInfo and
>>> ExecSetupPartitionTupleRouting and so that we just init the FDW in
>>> ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I
>>> added
>>> comments to ExecInitForeignRouting and ExecPrepareTupleRouting.
>>
>> Hmm, I may be misreading what you mean here ... but as far as I
>> understand, ON CONFLICT does not support foreign tables, does it?
> 
> It doesn't, except for ON CONFLICT DO NOTHING without an inference
> specification.
> 
>> If
>> that is so, I'm not sure why the conversion map would be necessary, if
>> it is for ON CONFLICT alone.
> 
> We wouldn't need that for foreign partitions (When DO NOTHING with an
> inference specification or DO UPDATE on a partitioned table containing
> foreign partitions, the planner would throw an error before we get to
> ExecInitPartitionInfo).

Actually, as you might know, since it is not possible to create an index
on a partitioned table that has a foreign partition, there is no
possibility of supporting any form of ON CONFLICT that requires an
inference specification.

> The reason why I updated the patch that way is
> just to make the patch simple, but on reflection I don't think that's a
> good idea; I think we should delay the map-creation step as well as the
> FDW-initialization step for UPDATE subplan partitions as before for
> improved efficiency for UPDATE-of-partition-key.  However, I don't think
> it'd still be a good idea to delay those steps for partitions created by
> ExecInitPartitionInfo the same way as for the subplan partitions, because
> in that case it'd be better to perform CheckValidResultRel against a
> partition right after we do InitResultRelInfo for the partition in
> ExecInitPartitionInfo, as that would save cycles in cases when the
> partition is considered nonvalid by that check.

It seems like a good idea to perform CheckValidResultRel right after the
InitResultRelInfo in ExecInitPartitionInfo.  However, if the partition is
considered non-valid, that results in an error afaik, so I don't
understand the point about saving cycles.

> So, What I'm thinking is:
> a) for the subplan partitions, we delay those steps as before, and b) for
> the partitions created by ExecInitPartitionInfo, we do that check for a
> partition right after InitResultRelInfo in that function, (and if valid,
> we create a map and initialize the FDW for the partition in that function).

Sounds good to me.  I'm assuming that you're talking about delaying the
is-valid-for-insert-routing check (using CheckValidResultRel) and
parent-to-child map creation for a sub-plan result relation until
ExecPrepareTupleRouting().

On a related note, I wonder if it'd be a good idea to rename the flag
ri_PartitionIsValid to something that signifies that we only need it to be
true if we want to do tuple routing (aka insert) using it.  Maybe,
ri_PartitionReadyForRouting or ri_PartitionReadyForInsert.  I'm afraid
that ri_PartitionIsValid is a bit ambiguous, although I'm not saying the
name options I'm suggesting are the best.

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/04/02 18:49), Amit Langote wrote:

2. If I understand the description you provided in [1] correctly, the
point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
avoid possibly-redundantly performing following two steps in
ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
that may not be used for tuple routing after all:

   - create the parent_child_tupconv_maps[] entry for it
   - perform FDW tuple routing initialization.


Sorry, my explanation was not enough, but that was just one of the reasons
why I introduced those; another is to do CheckValidResultRel against a
partition after the partition was chosen for tuple routing rather than in
ExecSetupPartitionTupleRouting, to avoid aborting an UPDATE of a partition
key unnecessarily due to non-routable foreign-partitions that may not be
chosen for tuple routing after all.


I see.  So, currently in ExecSetupPartitionTupleRouting, we call
CheckValidResultRel() to check if resultRelInfos reused from those
initialized for UPDATE are valid for insertions (or rather for routing
inserted tuples into it).  But you're proposing to delay that check until
ExecPrepareTupleRouting is called for a given resultRelInfo, at which
point it's clear that we actually want to insert using that resultRelInfo.
  That seems reasonable.


That's exactly what I'm thinking.


Now we have ON CONFLICT for partitioned tables, which requires the
conversion map to be computed in ExecInitPartitionInfo, so I updated the
patch so that we keep the former step in ExecInitPartitionInfo and
ExecSetupPartitionTupleRouting and so that we just init the FDW in
ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I
added comments to ExecInitForeignRouting and ExecPrepareTupleRouting.


That looks good.


I revisited this.  Please see the reply to Alvaro I sent right now.


I looked at the new patch.  It looks good overall, although I have one
question -- IIUC, BeginForeignInsert() performs actions that are
equivalent of performing PlanForeignModify() + BeginForeignModify() for an
INSERT as if it was directly executed on a given foreign table/partition.
Did you face any problems in doing the latter itself in the core code?
Doing it that way would mean no changes to a FDW itself will be required
(and hence no need for additional APIs), but I may be missing something.


The biggest issue in performing PlanForeignModify() plus 
BeginForeignModify() instead of the new FDW API would be: can the core 
code create a valid-looking planner state passed to PlanForeignModify() 
such as the ModifyTable plan node or the query tree stored in PlannerInfo?



One thing that seems good about your approach is that newly added support
for COPY FROM on foreign tables/partitions takes minimal changes as
implemented by using the new API, whereas with the alternative approach it
would require duplicated code (same code would have to be present in both
copy.c and execPartition.c, but it could perhaps be avoided).


I agree.

Thanks for the review!

Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/03/30 22:28), Alvaro Herrera wrote:

Etsuro Fujita wrote:


Now we have ON CONFLICT for partitioned tables, which requires the
conversion map to be computed in ExecInitPartitionInfo, so I updated the
patch so that we keep the former step in ExecInitPartitionInfo and
ExecSetupPartitionTupleRouting and so that we just init the FDW in
ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I added
comments to ExecInitForeignRouting and ExecPrepareTupleRouting.


Hmm, I may be misreading what you mean here ... but as far as I
understand, ON CONFLICT does not support foreign tables, does it?


It doesn't, except for ON CONFLICT DO NOTHING without an inference 
specification.



If
that is so, I'm not sure why the conversion map would be necessary, if
it is for ON CONFLICT alone.


We wouldn't need that for foreign partitions (When DO NOTHING with an 
inference specification or DO UPDATE on a partitioned table containing 
foreign partitions, the planner would throw an error before we get to 
ExecInitPartitionInfo).  The reason why I updated the patch that way is 
just to make the patch simple, but on reflection I don't think that's a 
good idea; I think we should delay the map-creation step as well as the 
FDW-initialization step for UPDATE subplan partitions as before for 
improved efficiency for UPDATE-of-partition-key.  However, I don't think 
it'd still be a good idea to delay those steps for partitions created by 
ExecInitPartitionInfo the same way as for the subplan partitions, 
because in that case it'd be better to perform CheckValidResultRel 
against a partition right after we do InitResultRelInfo for the 
partition in ExecInitPartitionInfo, as that would save cycles in cases 
when the partition is considered nonvalid by that check.  So, What I'm 
thinking is: a) for the subplan partitions, we delay those steps as 
before, and b) for the partitions created by ExecInitPartitionInfo, we 
do that check for a partition right after InitResultRelInfo in that 
function, (and if valid, we create a map and initialize the FDW for the 
partition in that function).


Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

Thanks for updating the patch.

On 2018/03/30 21:56, Etsuro Fujita wrote:
> I modified the patch to use the existing API ExecForeignInsert instead of
> that API and removed that API including this doc.

OK.

>> 2. If I understand the description you provided in [1] correctly, the
>> point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
>> avoid possibly-redundantly performing following two steps in
>> ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
>> that may not be used for tuple routing after all:
>>
>>   - create the parent_child_tupconv_maps[] entry for it
>>   - perform FDW tuple routing initialization.
> 
> Sorry, my explanation was not enough, but that was just one of the reasons
> why I introduced those; another is to do CheckValidResultRel against a
> partition after the partition was chosen for tuple routing rather than in
> ExecSetupPartitionTupleRouting, to avoid aborting an UPDATE of a partition
> key unnecessarily due to non-routable foreign-partitions that may not be
> chosen for tuple routing after all.

I see.  So, currently in ExecSetupPartitionTupleRouting, we call
CheckValidResultRel() to check if resultRelInfos reused from those
initialized for UPDATE are valid for insertions (or rather for routing
inserted tuples into it).  But you're proposing to delay that check until
ExecPrepareTupleRouting is called for a given resultRelInfo, at which
point it's clear that we actually want to insert using that resultRelInfo.
 That seems reasonable.

> Now we have ON CONFLICT for partitioned tables, which requires the
> conversion map to be computed in ExecInitPartitionInfo, so I updated the
> patch so that we keep the former step in ExecInitPartitionInfo and
> ExecSetupPartitionTupleRouting and so that we just init the FDW in
> ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I
> added comments to ExecInitForeignRouting and ExecPrepareTupleRouting.

That looks good.


I looked at the new patch.  It looks good overall, although I have one
question -- IIUC, BeginForeignInsert() performs actions that are
equivalent of performing PlanForeignModify() + BeginForeignModify() for an
INSERT as if it was directly executed on a given foreign table/partition.
Did you face any problems in doing the latter itself in the core code?
Doing it that way would mean no changes to a FDW itself will be required
(and hence no need for additional APIs), but I may be missing something.

One thing that seems good about your approach is that newly added support
for COPY FROM on foreign tables/partitions takes minimal changes as
implemented by using the new API, whereas with the alternative approach it
would require duplicated code (same code would have to be present in both
copy.c and execPartition.c, but it could perhaps be avoided).

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Alvaro Herrera]

Etsuro Fujita wrote:

> Now we have ON CONFLICT for partitioned tables, which requires the
> conversion map to be computed in ExecInitPartitionInfo, so I updated the
> patch so that we keep the former step in ExecInitPartitionInfo and
> ExecSetupPartitionTupleRouting and so that we just init the FDW in
> ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I added
> comments to ExecInitForeignRouting and ExecPrepareTupleRouting.

Hmm, I may be misreading what you mean here ... but as far as I
understand, ON CONFLICT does not support foreign tables, does it?  If
that is so, I'm not sure why the conversion map would be necessary, if
it is for ON CONFLICT alone.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/03/20 21:31), Alvaro Herrera wrote:

Amit Langote wrote:


2. If I understand the description you provided in [1] correctly, the
point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
avoid possibly-redundantly performing following two steps in
ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
that may not be used for tuple routing after all:

  - create the parent_child_tupconv_maps[] entry for it
  - perform FDW tuple routing initialization.

If that's true, the following comment could be expanded just a little bit
to make that clearer:

/*
  * ExecInitPartitionExtraInfo
  *  Do the remaining initialization work for the given partition


Yeah, I think the comments on why this is a separate step should be more
extensive.  Probably add something to ExecInitPartitionInfo too.


Done.  Please see the reply to Amit that I sent just now.

Thank you for the review!

Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/03/23 20:55), Etsuro Fujita wrote:

(2018/03/23 4:09), Robert Haas wrote:

1. It still doesn't work for COPY, because COPY isn't going to have a
ModifyTableState. I really think it ought to be possible to come up
with an API that can handle both INSERT and COPY; I don't think it
should be necessary to have two different APIs for those two problems.
Amit managed to do it for regular tables, and I don't really see a
good reason why foreign tables need to be different.


Maybe I'm missing something, but I think the proposed FDW API could be
used for the COPY case as well with some modifications to core. If so,
my question is: should we support COPY into foreign tables as well? I
think that if we support COPY tuple routing for foreign partitions, it
would be better to support direct COPY into foreign partitions as well.


Done.


2. I previously asked why we couldn't use the existing APIs for this,
and you gave me some answer, but I can't help noticing that
postgresExecForeignRouting is an exact copy of
postgresExecForeignInsert. Apparently, some code reuse is indeed
possible here! Why not reuse the same function instead of calling a
new one? If the answer is that planSlot might be NULL in this case,
or something like that, then let's just document that. A needless
proliferation of FDW APIs is really undesirable, as it raises the bar
for every FDW author.


You've got a point! I'll change the patch that way.


Done.  I modified the patch so that the existing API 
postgresExecForeignInsert is called as-is (ie, with the planSlot 
parameter) in the INSERT case and is called with that parameter set to 
NULL in the COPY case.  So, I removed postgresExecForeignRouting and the 
postgres_fdw refactoring for that API.  Also, I changed the names of the 
remaining new APIs to postgresBeginForeignInsert and 
postgresEndForeignInsert, which I think would be better because these 
are used not only for tuple routing but for directly copying into 
foreign tables.  Also, I dropped partition_index from the parameter list 
for postgresBeginForeignInsert; I thought that it could be used for the 
FDW to access the partition info stored in mt_partition_tuple_routing 
for something in the case of tuple-routing, but I started to think that 
the FDW wouldn't need that in practice.



However, I think that getting INSERT
but not COPY, with bad performance, and with duplicated APIs, is
moving more in the wrong direction than the right one.


Will fix.


Done.

Attached is the new version of the patch.  Patch 
foreign-routing-fdwapi-2.patch is created on top of patch 
postgres-fdw-refactoring-2.patch.  (The former contains the bug-fix 
[1].)  Any feedback is welcome!


Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5aba4074.1090...@lab.ntt.co.jp
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 375,386  static bool ec_member_matches_foreign(PlannerInfo *root, RelOptInfo *rel,
--- 375,395 
  static void create_cursor(ForeignScanState *node);
  static void fetch_more_data(ForeignScanState *node);
  static void close_cursor(PGconn *conn, unsigned int cursor_number);
+ static PgFdwModifyState *create_foreign_modify(EState *estate,
+ 	  ResultRelInfo *resultRelInfo,
+ 	  CmdType operation,
+ 	  Plan *subplan,
+ 	  char *query,
+ 	  List *target_attrs,
+ 	  bool has_returning,
+ 	  List *retrieved_attrs);
  static void prepare_foreign_modify(PgFdwModifyState *fmstate);
  static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
  		 ItemPointer tupleid,
  		 TupleTableSlot *slot);
  static void store_returning_result(PgFdwModifyState *fmstate,
  	   TupleTableSlot *slot, PGresult *res);
+ static void finish_foreign_modify(PgFdwModifyState *fmstate);
  static List *build_remote_returning(Index rtindex, Relation rel,
  	   List *returningList);
  static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist);
***
*** 1678,1695  postgresBeginForeignModify(ModifyTableState *mtstate,
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	EState	   *estate = mtstate->ps.state;
! 	CmdType		operation = mtstate->operation;
! 	Relation	rel = resultRelInfo->ri_RelationDesc;
! 	RangeTblEntry *rte;
! 	Oid			userid;
! 	ForeignTable *table;
! 	UserMapping *user;
! 	AttrNumber	n_params;
! 	Oid			typefnoid;
! 	bool		isvarlena;
! 	ListCell   *lc;
! 	TupleDesc	tupdesc = RelationGetDescr(rel);
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
--- 1687,1696 
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	char	   *query;
! 	List	   *target_attrs;
! 	bool		has_returning;
! 	List	   *retrieved_attrs;
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
***
*** 1698,1779  postgresBeginForeignModify(ModifyTableState *mtstate,
  	if (eflags & EXEC_FLAG_EXPLAIN_ONLY)
  		return;
  
- 	/* Begin 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/03/19 20:25), Amit Langote wrote:

On 2018/02/27 21:01, Etsuro Fujita wrote:

Attached is a new version of the patch set.



* Comments postgres-fdw-refactoring-1.patch:

1. Just a minor nitpick: wouldn't it be better to call it
create_foreign_modify_state just like its "finish" counterpart that's
named finish_foreign_modify?


Good idea!  Done.


* Comments on foreign-routing-fdwapi-1.patch:

1. In the following sentence, s/rows/a tuple/g, to be consistent with
other text added by the patch

+
+ If theExecForeignRouting  pointer is set to
+NULL, attempts to route rows to the foreign table
will fail
+ with an error message.
+


I modified the patch to use the existing API ExecForeignInsert instead 
of that API and removed that API including this doc.



2. If I understand the description you provided in [1] correctly, the
point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
avoid possibly-redundantly performing following two steps in
ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
that may not be used for tuple routing after all:

  - create the parent_child_tupconv_maps[] entry for it
  - perform FDW tuple routing initialization.


Sorry, my explanation was not enough, but that was just one of the 
reasons why I introduced those; another is to do CheckValidResultRel 
against a partition after the partition was chosen for tuple routing 
rather than in ExecSetupPartitionTupleRouting, to avoid aborting an 
UPDATE of a partition key unnecessarily due to non-routable 
foreign-partitions that may not be chosen for tuple routing after all.


Now we have ON CONFLICT for partitioned tables, which requires the 
conversion map to be computed in ExecInitPartitionInfo, so I updated the 
patch so that we keep the former step in ExecInitPartitionInfo and 
ExecSetupPartitionTupleRouting and so that we just init the FDW in 
ExecInitExtraPartitionInfo (changed to ExecInitForeignRouting).  And I 
added comments to ExecInitForeignRouting and ExecPrepareTupleRouting.



3. You removed the following from ExecInitPartitionInfo:

  /*
- * Verify result relation is a valid target for an INSERT.  An UPDATE
of a
- * partition-key becomes a DELETE+INSERT operation, so this check is
still
- * required when the operation is CMD_UPDATE.
- */
-CheckValidResultRel(leaf_part_rri, CMD_INSERT);

but, only added back the following in ExecInsert:

+/*
+ * Verify the specified partition is a valid target for INSERT if we
+ * didn't yet.
+ */
+if (!resultRelInfo->ri_PartitionIsValid)
+{
+CheckValidResultRel(resultRelInfo, CMD_INSERT);

Maybe, the new comment should add a "Note: " line the comment saying
something about this code being invoked as part of an UPDATE.


Done.

Also, I fixed a bug reported from you the way you proposed [1], and 
added regression tests for that.  Good catch!  Thanks!


Thank you for the review!

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/2d72275d-3574-92c9-9241-5c9b456c87a2%40lab.ntt.co.jp

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Fri, Mar 23, 2018 at 8:22 AM, Etsuro Fujita
 wrote:
>> I think for bulk
>> inserts we'll need an API that says "here's a row, store it or buffer
>> it as you like" and then another API that says "flush any buffered
>> rows to the actual table and perform any necessary cleanup".  Or maybe
>> in postgres_fdw the first API could start a COPY if not already done
>> and send the row, and the second one could end the COPY.
> That's really what I have in mind.

So let's do it.

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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/03/23 21:02), Robert Haas wrote:

On Fri, Mar 23, 2018 at 7:55 AM, Etsuro Fujita
  wrote:

Maybe I'm missing something, but I think the proposed FDW API could be used
for the COPY case as well with some modifications to core.  If so, my
question is: should we support COPY into foreign tables as well?  I think
that if we support COPY tuple routing for foreign partitions, it would be
better to support direct COPY into foreign partitions as well.


Yes, I really, really want to be able to support COPY.  If you think
you can make that work with this API -- let's see it!


OK


3. It looks like we're just doing an INSERT for every row, which is
pretty much an anti-pattern for inserting data into a PostgreSQL
database.  COPY is way faster, and even multi-row inserts are
significantly faster.


I planed to work on new FDW API for using COPY for COPY tuple routing [1],
but I didn't have time for that in this development cycle, so I'm
re-planning to work on that for PG12.  I'm not sure we can optimize that
insertion using multi-row inserts because tuple routing works row by row, as
you know.  Anyway, I think these would be beyond the scope of the first
version.


My concern is that if we add APIs now that only support single-row
inserts, we'll have to rework them again in order to support multi-row
inserts.  I'd like to avoid that, if possible.


Yeah, but we would have this issue for normal inserts into foreign 
tables.  For the normal case, what I'm thinking to support multi-row 
inserts is to use DirectModify FDW API.  With this API, I think we could 
support pushing down INSERT with multiple VALUES sublists to the remote, 
but I'm not sure we could extend that to the tuple-routing case.



I think for bulk
inserts we'll need an API that says "here's a row, store it or buffer
it as you like" and then another API that says "flush any buffered
rows to the actual table and perform any necessary cleanup".  Or maybe
in postgres_fdw the first API could start a COPY if not already done
and send the row, and the second one could end the COPY.


That's really what I have in mind.

Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Fri, Mar 23, 2018 at 7:55 AM, Etsuro Fujita
 wrote:
> Maybe I'm missing something, but I think the proposed FDW API could be used
> for the COPY case as well with some modifications to core.  If so, my
> question is: should we support COPY into foreign tables as well?  I think
> that if we support COPY tuple routing for foreign partitions, it would be
> better to support direct COPY into foreign partitions as well.

Yes, I really, really want to be able to support COPY.  If you think
you can make that work with this API -- let's see it!

>> 3. It looks like we're just doing an INSERT for every row, which is
>> pretty much an anti-pattern for inserting data into a PostgreSQL
>> database.  COPY is way faster, and even multi-row inserts are
>> significantly faster.
>
> I planed to work on new FDW API for using COPY for COPY tuple routing [1],
> but I didn't have time for that in this development cycle, so I'm
> re-planning to work on that for PG12.  I'm not sure we can optimize that
> insertion using multi-row inserts because tuple routing works row by row, as
> you know.  Anyway, I think these would be beyond the scope of the first
> version.

My concern is that if we add APIs now that only support single-row
inserts, we'll have to rework them again in order to support multi-row
inserts.  I'd like to avoid that, if possible.  I think for bulk
inserts we'll need an API that says "here's a row, store it or buffer
it as you like" and then another API that says "flush any buffered
rows to the actual table and perform any necessary cleanup".  Or maybe
in postgres_fdw the first API could start a COPY if not already done
and send the row, and the second one could end the COPY.

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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/03/23 4:09), Robert Haas wrote:

On Tue, Feb 27, 2018 at 7:01 AM, Etsuro Fujita

Attached is a new version of the patch set.


I took a look at this patch set today but I really don't think we
should commit something so minimal.  It's got at least four issues
that I see:

1. It still doesn't work for COPY, because COPY isn't going to have a
ModifyTableState.  I really think it ought to be possible to come up
with an API that can handle both INSERT and COPY; I don't think it
should be necessary to have two different APIs for those two problems.
Amit managed to do it for regular tables, and I don't really see a
good reason why foreign tables need to be different.


Maybe I'm missing something, but I think the proposed FDW API could be 
used for the COPY case as well with some modifications to core.  If so, 
my question is: should we support COPY into foreign tables as well?  I 
think that if we support COPY tuple routing for foreign partitions, it 
would be better to support direct COPY into foreign partitions as well.



2. I previously asked why we couldn't use the existing APIs for this,
and you gave me some answer, but I can't help noticing that
postgresExecForeignRouting is an exact copy of
postgresExecForeignInsert.  Apparently, some code reuse is indeed
possible here!  Why not reuse the same function instead of calling a
new one?  If the answer is that planSlot might be NULL in this case,
or something like that, then let's just document that.  A needless
proliferation of FDW APIs is really undesirable, as it raises the bar
for every FDW author.


You've got a point!  I'll change the patch that way.


3. It looks like we're just doing an INSERT for every row, which is
pretty much an anti-pattern for inserting data into a PostgreSQL
database.  COPY is way faster, and even multi-row inserts are
significantly faster.


I planed to work on new FDW API for using COPY for COPY tuple routing 
[1], but I didn't have time for that in this development cycle, so I'm 
re-planning to work on that for PG12.  I'm not sure we can optimize that 
insertion using multi-row inserts because tuple routing works row by 
row, as you know.  Anyway, I think these would be beyond the scope of 
the first version.



4. It doesn't do anything about the UPDATE tuple routing problem
mentioned upthread.

I don't necessarily think that the first patch in this area has to
solve all of those problems, and #4 in particular seems like it might
be a pretty big can of worms.


OK


However, I think that getting INSERT
but not COPY, with bad performance, and with duplicated APIs, is
moving more in the wrong direction than the right one.


Will fix.

Thanks for reviewing the patch!

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/23990375-45a6-5823-b0aa-a6a7a6a957f0%40lab.ntt.co.jp

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Robert Haas]

On Tue, Feb 27, 2018 at 7:01 AM, Etsuro Fujita
 wrote:
> Changes other than that are:
>
> * Fixed typo and revised code/comments
> * Added more regression tests
> * Added docs
>
> Attached is a new version of the patch set.

I took a look at this patch set today but I really don't think we
should commit something so minimal.  It's got at least four issues
that I see:

1. It still doesn't work for COPY, because COPY isn't going to have a
ModifyTableState.  I really think it ought to be possible to come up
with an API that can handle both INSERT and COPY; I don't think it
should be necessary to have two different APIs for those two problems.
Amit managed to do it for regular tables, and I don't really see a
good reason why foreign tables need to be different.

2. I previously asked why we couldn't use the existing APIs for this,
and you gave me some answer, but I can't help noticing that
postgresExecForeignRouting is an exact copy of
postgresExecForeignInsert.  Apparently, some code reuse is indeed
possible here!  Why not reuse the same function instead of calling a
new one?  If the answer is that planSlot might be NULL in this case,
or something like that, then let's just document that.  A needless
proliferation of FDW APIs is really undesirable, as it raises the bar
for every FDW author.

3. It looks like we're just doing an INSERT for every row, which is
pretty much an anti-pattern for inserting data into a PostgreSQL
database.  COPY is way faster, and even multi-row inserts are
significantly faster.

4. It doesn't do anything about the UPDATE tuple routing problem
mentioned upthread.

I don't necessarily think that the first patch in this area has to
solve all of those problems, and #4 in particular seems like it might
be a pretty big can of worms.  However, I think that getting INSERT
but not COPY, with bad performance, and with duplicated APIs, is
moving more in the wrong direction than the right one.

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

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Alvaro Herrera]

Hi,

Amit Langote wrote:

> 2. If I understand the description you provided in [1] correctly, the
> point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
> avoid possibly-redundantly performing following two steps in
> ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
> that may not be used for tuple routing after all:
> 
>  - create the parent_child_tupconv_maps[] entry for it
>  - perform FDW tuple routing initialization.
> 
> If that's true, the following comment could be expanded just a little bit
> to make that clearer:
> 
> /*
>  * ExecInitPartitionExtraInfo
>  *  Do the remaining initialization work for the given partition

Yeah, I think the comments on why this is a separate step should be more
extensive.  Probably add something to ExecInitPartitionInfo too.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

Hi Amit,

(2018/03/20 15:57), Amit Langote wrote:

On 2018/03/19 20:25, Amit Langote wrote:

That's all I have for now.


Will reply to your previous email.


While testing this patch, I noticed a crash when performing EXPLAIN on
update of a partition tree containing foreign partitions.  Crash occurs in
postgresEndForeignRouting() due to the following Assert failing:

   Assert(fmstate != NULL);

It seems the problem is that ExecCleanupTupleRouting() invokes the
EndForeignRouting() function even if ri_PartitionIsValid is not set.  So I
suppose we need this:

  /*
- * If this is INSERT/UPDATE, allow any FDWs to shut down
+ * If this is INSERT/UPDATE, allow any FDWs to shut down if it has
+ * initialized tuple routing information at all.
   */
  if (node&&
+resultRelInfo->ri_PartitionIsValid&&
  resultRelInfo->ri_FdwRoutine != NULL&&
  resultRelInfo->ri_FdwRoutine->EndForeignRouting != NULL)
  resultRelInfo->ri_FdwRoutine->EndForeignRouting(node->ps.state,


Will look into this.


BTW,patch needs to be rebased because of two commits this morning:
ee49f [1] and ee0a1fc84 [2].


Will do.

Thanks for reviewing the patch!

Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

On 2018/03/19 20:25, Amit Langote wrote:
> That's all I have for now.

While testing this patch, I noticed a crash when performing EXPLAIN on
update of a partition tree containing foreign partitions.  Crash occurs in
postgresEndForeignRouting() due to the following Assert failing:

  Assert(fmstate != NULL);

It seems the problem is that ExecCleanupTupleRouting() invokes the
EndForeignRouting() function even if ri_PartitionIsValid is not set.  So I
suppose we need this:

 /*
- * If this is INSERT/UPDATE, allow any FDWs to shut down
+ * If this is INSERT/UPDATE, allow any FDWs to shut down if it has
+ * initialized tuple routing information at all.
  */
 if (node &&
+resultRelInfo->ri_PartitionIsValid &&
 resultRelInfo->ri_FdwRoutine != NULL &&
 resultRelInfo->ri_FdwRoutine->EndForeignRouting != NULL)
 resultRelInfo->ri_FdwRoutine->EndForeignRouting(node->ps.state,

BTW,patch needs to be rebased because of two commits this morning:
ee49f [1] and ee0a1fc84 [2].

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ee49f

[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ee0a1fc84

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

Thanks for sending the updated patches.

On 2018/02/27 21:01, Etsuro Fujita wrote:
> (2018/02/21 20:54), Etsuro Fujita wrote:
>> void
>> BeginForeignRouting();
>>
>> Prepare for a tuple-routing operation on a foreign table. This is called
>> from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.
> 
> I modified execPartition.c so that this callback routine is called from a
> single function that I added to execPartition.c and it is called the first
> time the foreign partition is chose as the target partition to route a
> tuple to.  That removes CheckValidResultRel, the tuple-conversion setup,
> and the FDW initialization for each UPDATE subplan from
> ExecSetupPartitionTupleRouting, so it would minimize the possibly-useless
> overhead in doing that function.
> 
> Changes other than that are:
> 
> * Fixed typo and revised code/comments
> * Added more regression tests
> * Added docs
> 
> Attached is a new version of the patch set.

Patches still seem to apply cleanly to HEAD, compile fine, tests pass.

* Comments postgres-fdw-refactoring-1.patch:

1. Just a minor nitpick: wouldn't it be better to call it
create_foreign_modify_state just like its "finish" counterpart that's
named finish_foreign_modify?

Other than that, it looks good to me.

* Comments on foreign-routing-fdwapi-1.patch:

1. In the following sentence, s/rows/a tuple/g, to be consistent with
other text added by the patch

+
+ If the ExecForeignRouting pointer is set to
+ NULL, attempts to route rows to the foreign table
will fail
+ with an error message.
+


2. If I understand the description you provided in [1] correctly, the
point of having ri_PartitionIsValid and ExecInitExtraPartitionInfo() is to
avoid possibly-redundantly performing following two steps in
ExecSetupPartitionTupleRouting() for *reused* partition ResultRelInfos
that may not be used for tuple routing after all:

 - create the parent_child_tupconv_maps[] entry for it
 - perform FDW tuple routing initialization.

If that's true, the following comment could be expanded just a little bit
to make that clearer:

/*
 * ExecInitPartitionExtraInfo
 *  Do the remaining initialization work for the given partition

3. You removed the following from ExecInitPartitionInfo:

 /*
- * Verify result relation is a valid target for an INSERT.  An UPDATE
of a
- * partition-key becomes a DELETE+INSERT operation, so this check is
still
- * required when the operation is CMD_UPDATE.
- */
-CheckValidResultRel(leaf_part_rri, CMD_INSERT);

but, only added back the following in ExecInsert:

+/*
+ * Verify the specified partition is a valid target for INSERT if we
+ * didn't yet.
+ */
+if (!resultRelInfo->ri_PartitionIsValid)
+{
+CheckValidResultRel(resultRelInfo, CMD_INSERT);

Maybe, the new comment should add a "Note: " line the comment saying
something about this code being invoked as part of an UPDATE.

Tests and documentation added by this patch look quite good.

That's all I have for now.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/5A95487E.9050808%40lab.ntt.co.jp

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/02/21 20:54), Etsuro Fujita wrote:

void
BeginForeignRouting();

Prepare for a tuple-routing operation on a foreign table. This is called
from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.


I modified execPartition.c so that this callback routine is called from 
a single function that I added to execPartition.c and it is called the 
first time the foreign partition is chose as the target partition to 
route a tuple to.  That removes CheckValidResultRel, the 
tuple-conversion setup, and the FDW initialization for each UPDATE 
subplan from ExecSetupPartitionTupleRouting, so it would minimize the 
possibly-useless overhead in doing that function.


Changes other than that are:

* Fixed typo and revised code/comments
* Added more regression tests
* Added docs

Attached is a new version of the patch set.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 375,386  static bool ec_member_matches_foreign(PlannerInfo *root, RelOptInfo *rel,
--- 375,398 
  static void create_cursor(ForeignScanState *node);
  static void fetch_more_data(ForeignScanState *node);
  static void close_cursor(PGconn *conn, unsigned int cursor_number);
+ static PgFdwModifyState *create_fdw_modify_state(ModifyTableState *mtstate,
+ 		ResultRelInfo *resultRelInfo,
+ 		CmdType operation,
+ 		int subplan_index,
+ 		char *query,
+ 		List *target_attrs,
+ 		bool has_returning,
+ 		List *retrieved_attrs);
  static void prepare_foreign_modify(PgFdwModifyState *fmstate);
  static const char **convert_prep_stmt_params(PgFdwModifyState *fmstate,
  		 ItemPointer tupleid,
  		 TupleTableSlot *slot);
+ static int execute_prep_stmt(PgFdwModifyState *fmstate,
+   const char **p_values,
+   TupleTableSlot *slot);
  static void store_returning_result(PgFdwModifyState *fmstate,
  	   TupleTableSlot *slot, PGresult *res);
+ static void finish_foreign_modify(PgFdwModifyState *fmstate);
  static List *build_remote_returning(Index rtindex, Relation rel,
  	   List *returningList);
  static void rebuild_fdw_scan_tlist(ForeignScan *fscan, List *tlist);
***
*** 1678,1695  postgresBeginForeignModify(ModifyTableState *mtstate,
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	EState	   *estate = mtstate->ps.state;
! 	CmdType		operation = mtstate->operation;
! 	Relation	rel = resultRelInfo->ri_RelationDesc;
! 	RangeTblEntry *rte;
! 	Oid			userid;
! 	ForeignTable *table;
! 	UserMapping *user;
! 	AttrNumber	n_params;
! 	Oid			typefnoid;
! 	bool		isvarlena;
! 	ListCell   *lc;
! 	TupleDesc	tupdesc = RelationGetDescr(rel);
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
--- 1690,1699 
  		   int eflags)
  {
  	PgFdwModifyState *fmstate;
! 	char	   *query;
! 	List	   *target_attrs;
! 	bool		has_returning;
! 	List	   *retrieved_attrs;
  
  	/*
  	 * Do nothing in EXPLAIN (no ANALYZE) case.  resultRelInfo->ri_FdwState
***
*** 1698,1779  postgresBeginForeignModify(ModifyTableState *mtstate,
  	if (eflags & EXEC_FLAG_EXPLAIN_ONLY)
  		return;
  
- 	/* Begin constructing PgFdwModifyState. */
- 	fmstate = (PgFdwModifyState *) palloc0(sizeof(PgFdwModifyState));
- 	fmstate->rel = rel;
- 
- 	/*
- 	 * Identify which user to do the remote access as.  This should match what
- 	 * ExecCheckRTEPerms() does.
- 	 */
- 	rte = rt_fetch(resultRelInfo->ri_RangeTableIndex, estate->es_range_table);
- 	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
- 
- 	/* Get info about foreign table. */
- 	table = GetForeignTable(RelationGetRelid(rel));
- 	user = GetUserMapping(userid, table->serverid);
- 
- 	/* Open connection; report that we'll create a prepared statement. */
- 	fmstate->conn = GetConnection(user, true);
- 	fmstate->p_name = NULL;		/* prepared statement not made yet */
- 
  	/* Deconstruct fdw_private data. */
! 	fmstate->query = strVal(list_nth(fdw_private,
! 	 FdwModifyPrivateUpdateSql));
! 	fmstate->target_attrs = (List *) list_nth(fdw_private,
! 			  FdwModifyPrivateTargetAttnums);
! 	fmstate->has_returning = intVal(list_nth(fdw_private,
! 			 FdwModifyPrivateHasReturning));
! 	fmstate->retrieved_attrs = (List *) list_nth(fdw_private,
!  FdwModifyPrivateRetrievedAttrs);
! 
! 	/* Create context for per-tuple temp workspace. */
! 	fmstate->temp_cxt = AllocSetContextCreate(estate->es_query_cxt,
! 			  "postgres_fdw temporary data",
! 			  ALLOCSET_SMALL_SIZES);
! 
! 	/* Prepare for input conversion of RETURNING results. */
! 	if (fmstate->has_returning)
! 		fmstate->attinmeta = TupleDescGetAttInMetadata(tupdesc);
! 
! 	/* Prepare for output conversion of parameters used in prepared stmt. */
! 	n_params = list_length(fmstate->target_attrs) + 1;
! 	fmstate->p_flinfo = (FmgrInfo *) palloc0(sizeof(FmgrInfo) * n_params);
! 	fmstate->p_nums = 0;
! 
! 	if (operation == CMD_UPDATE || 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/02/23 16:38), Amit Langote wrote:

On Thu, Feb 22, 2018 at 8:49 PM, Etsuro Fujita
  wrote:

This would introduce an asymmetry (we can move tuples from plain partitions
to foreign partitions, but the reverse is not true), but I am thinking that
it would be probably okay to document about that.



About just documenting the asymmetry you mentioned that's caused by
the fact that we don't enforce constraints on foreign tables, I
started wondering if we shouldn't change our stance on the matter wrt
"partition" constraints?


I'm not sure that it's a good idea to make an exception in that case. 
Another concern is triggers on the remote side; those might change the 
row so that the partition constraint of the containing partition is no 
longer satisfied.



But, admittedly, that's a topic for a
different thread.


OK, I'll leave that for another patch.

Will post a new version.  Thanks for the comments!

Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

On Thu, Feb 22, 2018 at 8:49 PM, Etsuro Fujita
 wrote:
> (2018/02/22 11:52), Amit Langote wrote:
>> I wonder why partition_index needs to be made part of this API?
>
> The reason for that is because I think the FDW might want to look at the
> partition info stored in mtstate->mt_partition_tuple_routing for some reason
> or other, such as parent_child_tupconv_maps and child_parent_tupconv_maps,
> which are accessed with the given partition index.

I see.  I guess that makes sense.

>> Perhaps an independent concern, but one thing I noticed is that it does
>> not seem to play well with the direct modification (update push-down)
>> feature.  Now because updates (at least local, let's say) support
>> re-routing, I thought we'd be able move rows across servers via the local
>> server, but with direct modification we'd never get the chance.  However,
>> since update tuple routing is triggered by partition constraint failure,
>> which we don't enforce for foreign tables/partitions anyway, I'm not sure
>> if we need to do anything about that, and even if we did, whether it
>> concerns this proposal.
>
> Good point!  Actually, update tuple routing we have in HEAD doesn't allow
> re-routing tuples from foreign partitions even without direct modification.
> Here is an example using postgres_fdw:
>
> postgres=# create table pt (a int, b text) partition by list (a);
> CREATE TABLE
> postgres=# create table loct (a int check (a in (1)), b text);
> CREATE TABLE
> postgres=# create foreign table remp partition of pt for values in (1)
> server loopback options (table_name 'loct');
> CREATE FOREIGN TABLE
> postgres=# create table locp partition of pt for values in (2);
> CREATE TABLE
> postgres=# insert into remp values (1, 'foo');
> INSERT 0 1
> postgres=# insert into locp values (2, 'bar');
> INSERT 0 1
> postgres=# select tableoid::regclass, * from pt;
>  tableoid | a |  b
> --+---+-
>  remp | 1 | foo
>  locp | 2 | bar
> (2 rows)
>
> postgres=# create function postgres_fdw_abs(int) returns int as $$begin
> return abs($1); end$$ language plpgsql immutable;
> CREATE FUNCTION
> postgres=# explain verbose update pt set a = postgres_fdw_abs(a) + 1 where b
> = 'foo';
>  QUERY PLAN
>
> 
> -
>  Update on public.pt  (cost=100.00..154.54 rows=12 width=42)
>Foreign Update on public.remp
>  Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1
>Update on public.locp
>->  Foreign Scan on public.remp  (cost=100.00..127.15 rows=6 width=42)
>  Output: (postgres_fdw_abs(remp.a) + 1), remp.b, remp.ctid
>  Remote SQL: SELECT a, b, ctid FROM public.loct WHERE ((b =
> 'foo'::text)) FOR UPDATE
>->  Seq Scan on public.locp  (cost=0.00..27.39 rows=6 width=42)
>  Output: (postgres_fdw_abs(locp.a) + 1), locp.b, locp.ctid
>  Filter: (locp.b = 'foo'::text)
> (10 rows)
>
> postgres=# update pt set a = postgres_fdw_abs(a) + 1 where b = 'foo';
> ERROR:  new row for relation "loct" violates check constraint "loct_a_check"
> DETAIL:  Failing row contains (2, foo).
> CONTEXT:  Remote SQL command: UPDATE public.loct SET a = $2 WHERE ctid = $1
>
> To be able to move tuples across foreign servers, I think we would first
> have to do something to allow re-routing tuples from foreign partitions.
> The patches I proposed hasn't done anything about that, so the patched
> version would behave the same way as HEAD with/without direct modification.

Yes.

As I said, since update re-routing is triggered by partition
constraint failure for the new row and we don't check (any)
constraints for foreign tables, that means re-routing won't occur for
foreign partitions.

>> That said, I saw in the changes to ExecSetupPartitionTupleRouting() that
>> BeginForeignRouting() is called for a foreign partition even if direct
>> modification might already have been set up.  If direct modification is
>> set up, then ExecForeignRouting() will never get called, because we'd
>> never call ExecUpdate() or ExecInsert().
>
> Yeah, but I am thinking to leave the support for re-routing tuples across
> foreign servers for another patch.
>
> One difference between HEAD and the patched version would be: we can
> re-route tuples from a plain partition to a foreign partition if the foreign
> partition supports this tuple-routing API.  Here is an example using the
> above data:
>
> postgres=# select tableoid::regclass, * from pt;
>  tableoid | a |  b
> --+---+-
>  remp | 1 | foo
>  locp | 2 | bar
> (2 rows)
>
> postgres=# update pt set a = 1 where b = 'bar';
> UPDATE 1
> postgres=# select tableoid::regclass, * from pt;
>  tableoid | a |  b
> --+---+-
>  remp | 1 | foo
>  remp | 1 | bar
> (2 rows)
>
> This would introduce an asymmetry (we can move tuples from plain partitions
> to foreign partitions, but the 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/02/22 11:52), Amit Langote wrote:

On 2018/02/21 20:54, Etsuro Fujita wrote:

void
BeginForeignRouting(ModifyTableState *mtstate,
 ResultRelInfo *resultRelInfo,
 int partition_index);

Prepare for a tuple-routing operation on a foreign table.  This is called
from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.


I wonder why partition_index needs to be made part of this API?


The reason for that is because I think the FDW might want to look at the 
partition info stored in mtstate->mt_partition_tuple_routing for some 
reason or other, such as parent_child_tupconv_maps and 
child_parent_tupconv_maps, which are accessed with the given partition 
index.



Patch foreign-routing-fdwapi-WIP.patch: main patch to add new FDW APIs,
which is created on top of patch postgres-fdw-refactoring-WIP.patch and
the lazy-initialization-of-partition-info patch [1].


Noticed a typo in the patch (s/parition/partition/g):

+* Also let the FDW init itself if this 
parition is foreign.

+* Also let the FDW init itself if this parition is foreign.


Good catch!  Will fix.


Perhaps an independent concern, but one thing I noticed is that it does
not seem to play well with the direct modification (update push-down)
feature.  Now because updates (at least local, let's say) support
re-routing, I thought we'd be able move rows across servers via the local
server, but with direct modification we'd never get the chance.  However,
since update tuple routing is triggered by partition constraint failure,
which we don't enforce for foreign tables/partitions anyway, I'm not sure
if we need to do anything about that, and even if we did, whether it
concerns this proposal.


Good point!  Actually, update tuple routing we have in HEAD doesn't 
allow re-routing tuples from foreign partitions even without direct 
modification.  Here is an example using postgres_fdw:


postgres=# create table pt (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table loct (a int check (a in (1)), b text);
CREATE TABLE
postgres=# create foreign table remp partition of pt for values in (1) 
server loopback options (table_name 'loct');

CREATE FOREIGN TABLE
postgres=# create table locp partition of pt for values in (2);
CREATE TABLE
postgres=# insert into remp values (1, 'foo');
INSERT 0 1
postgres=# insert into locp values (2, 'bar');
INSERT 0 1
postgres=# select tableoid::regclass, * from pt;
 tableoid | a |  b
--+---+-
 remp | 1 | foo
 locp | 2 | bar
(2 rows)

postgres=# create function postgres_fdw_abs(int) returns int as $$begin 
return abs($1); end$$ language plpgsql immutable;

CREATE FUNCTION
postgres=# explain verbose update pt set a = postgres_fdw_abs(a) + 1 
where b = 'foo';

 QUERY PLAN


-
 Update on public.pt  (cost=100.00..154.54 rows=12 width=42)
   Foreign Update on public.remp
 Remote SQL: UPDATE public.loct SET a = $2 WHERE ctid = $1
   Update on public.locp
   ->  Foreign Scan on public.remp  (cost=100.00..127.15 rows=6 width=42)
 Output: (postgres_fdw_abs(remp.a) + 1), remp.b, remp.ctid
 Remote SQL: SELECT a, b, ctid FROM public.loct WHERE ((b = 
'foo'::text)) FOR UPDATE

   ->  Seq Scan on public.locp  (cost=0.00..27.39 rows=6 width=42)
 Output: (postgres_fdw_abs(locp.a) + 1), locp.b, locp.ctid
 Filter: (locp.b = 'foo'::text)
(10 rows)

postgres=# update pt set a = postgres_fdw_abs(a) + 1 where b = 'foo';
ERROR:  new row for relation "loct" violates check constraint "loct_a_check"
DETAIL:  Failing row contains (2, foo).
CONTEXT:  Remote SQL command: UPDATE public.loct SET a = $2 WHERE ctid = $1

To be able to move tuples across foreign servers, I think we would first 
have to do something to allow re-routing tuples from foreign partitions. 
 The patches I proposed hasn't done anything about that, so the patched 
version would behave the same way as HEAD with/without direct modification.



That said, I saw in the changes to ExecSetupPartitionTupleRouting() that
BeginForeignRouting() is called for a foreign partition even if direct
modification might already have been set up.  If direct modification is
set up, then ExecForeignRouting() will never get called, because we'd
never call ExecUpdate() or ExecInsert().


Yeah, but I am thinking to leave the support for re-routing tuples 
across foreign servers for another patch.


One difference between HEAD and the patched version would be: we can 
re-route tuples from a plain partition to a foreign partition if the 
foreign partition supports this tuple-routing API.  Here is an example 
using the above data:


postgres=# select tableoid::regclass, * from pt;
 tableoid | a |  b
--+---+-
 remp | 1 | foo
 locp | 2 | bar
(2 rows)

postgres=# update pt set a = 1 where b = 'bar';
UPDATE 1

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

Fujita-san,

On 2018/02/21 20:54, Etsuro Fujita wrote:
> (2018/02/02 19:33), Etsuro Fujita wrote:
>> (2018/01/25 23:33), Stephen Frost wrote:
>>> I'm afraid a good bit of this patch is now failing to apply. I don't
>>> have much else to say except to echo the performance concern that Amit
>>> Langote raised about expanding the inheritence tree twice.
>>
>> To address that concern, I'm thinking to redesign the patch so that it
>> wouldn't expand the tree at planning time anymore. I don't have any
>> clear solution for that yet, but what I have in mind now is to add new
>> FDW APIs to the executor, instead, so that the FDW could 1) create stuff
>> such as a query for remote INSERT as PlanForeignModify and 2)
>> initialize/end the remote INSERT operation as BeginForeignModify and
>> EndForeignModify, somewhere in the executor.
> 
> New FDW APIs I would like to propose for that are:

Thanks for updating the proposal.

> void
> BeginForeignRouting(ModifyTableState *mtstate,
>     ResultRelInfo *resultRelInfo,
>     int partition_index);
>
> Prepare for a tuple-routing operation on a foreign table.  This is called
> from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.

I wonder why partition_index needs to be made part of this API?

> TupleTableSlot *
> ExecForeignRouting(EState *estate,
>    ResultRelInfo *resultRelInfo,
>    TupleTableSlot *slot);
> 
> Route one tuple to the foreign table.  This is called from ExecInsert.
> 
> void
> EndForeignRouting(EState *estate,
>   ResultRelInfo *resultRelInfo);
> 
> End the operation and release resources.  This is called from
> ExecCleanupTupleRouting.
> 
> Attached are WIP patches for that:
> 
> Patch postgres-fdw-refactoring-WIP.patch: refactoring patch for
> postgres_fdw.c to reduce duplicate code.
> 
> Patch foreign-routing-fdwapi-WIP.patch: main patch to add new FDW APIs,
> which is created on top of patch postgres-fdw-refactoring-WIP.patch and
> the lazy-initialization-of-partition-info patch [1].

Noticed a typo in the patch (s/parition/partition/g):

+* Also let the FDW init itself if this 
parition is foreign.

+* Also let the FDW init itself if this parition is foreign.

> By this change we don't need to expand the inheritance tree at planning
> time, so no need to worry about the performance concern.  Maybe I'm
> missing something, though.  Early feedback would be greatly appreciated.

Perhaps an independent concern, but one thing I noticed is that it does
not seem to play well with the direct modification (update push-down)
feature.  Now because updates (at least local, let's say) support
re-routing, I thought we'd be able move rows across servers via the local
server, but with direct modification we'd never get the chance.  However,
since update tuple routing is triggered by partition constraint failure,
which we don't enforce for foreign tables/partitions anyway, I'm not sure
if we need to do anything about that, and even if we did, whether it
concerns this proposal.

That said, I saw in the changes to ExecSetupPartitionTupleRouting() that
BeginForeignRouting() is called for a foreign partition even if direct
modification might already have been set up.  If direct modification is
set up, then ExecForeignRouting() will never get called, because we'd
never call ExecUpdate() or ExecInsert().

Thanks,
Amit

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/02/02 19:33), Etsuro Fujita wrote:

(2018/01/25 23:33), Stephen Frost wrote:

I'm afraid a good bit of this patch is now failing to apply. I don't
have much else to say except to echo the performance concern that Amit
Langote raised about expanding the inheritence tree twice.


To address that concern, I'm thinking to redesign the patch so that it
wouldn't expand the tree at planning time anymore. I don't have any
clear solution for that yet, but what I have in mind now is to add new
FDW APIs to the executor, instead, so that the FDW could 1) create stuff
such as a query for remote INSERT as PlanForeignModify and 2)
initialize/end the remote INSERT operation as BeginForeignModify and
EndForeignModify, somewhere in the executor.


New FDW APIs I would like to propose for that are:

void
BeginForeignRouting(ModifyTableState *mtstate,
ResultRelInfo *resultRelInfo,
int partition_index);

Prepare for a tuple-routing operation on a foreign table.  This is 
called from ExecSetupPartitionTupleRouting and ExecInitPartitionInfo.


TupleTableSlot *
ExecForeignRouting(EState *estate,
   ResultRelInfo *resultRelInfo,
   TupleTableSlot *slot);

Route one tuple to the foreign table.  This is called from ExecInsert.

void
EndForeignRouting(EState *estate,
  ResultRelInfo *resultRelInfo);

End the operation and release resources.  This is called from 
ExecCleanupTupleRouting.


Attached are WIP patches for that:

Patch postgres-fdw-refactoring-WIP.patch: refactoring patch for 
postgres_fdw.c to reduce duplicate code.


Patch foreign-routing-fdwapi-WIP.patch: main patch to add new FDW APIs, 
which is created on top of patch postgres-fdw-refactoring-WIP.patch and 
the lazy-initialization-of-partition-info patch [1].


By this change we don't need to expand the inheritance tree at planning 
time, so no need to worry about the performance concern.  Maybe I'm 
missing something, though.  Early feedback would be greatly appreciated.


Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5a8bfb31.6030...@lab.ntt.co.jp
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 315,321  SELECT tableoid::regclass, * FROM p2;
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
--- 315,321 
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR
! ERROR:  cannot route copied tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
***
*** 342,351  SELECT tableoid::regclass, * FROM p2;
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  INSERT INTO pt VALUES (2, 'xyzzy');
  UPDATE pt set a = 1 where a = 2; -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
  --+---+---
--- 342,351 
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to foreign table "p1"
  INSERT INTO pt VALUES (2, 'xyzzy');
  UPDATE pt set a = 1 where a = 2; -- ERROR
! ERROR:  cannot route inserted tuples to foreign table "p1"
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
  --+---+---
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 7364,7369  NOTICE:  drop cascades to foreign table bar2
--- 7364,7448 
  drop table loct1;
  drop table loct2;
  -- ===
+ -- test tuple routing for foreign-table partitions
+ -- ===
+ create table pt (a int, b int, c text) partition by list (a);
+ create table loct1 (a int check (a in (1)), b int, c text, constraint locp1_pkey primary key (b));
+ create table loct2 (b int, c text, a int check (a in (2)), constraint locp2_pkey primary key (b));
+ create foreign table ptp1 partition of pt for values in (1) server loopback options (table_name 'loct1');
+ create foreign table ptp2 partition of pt for values in (2) server loopback options (table_name 'loct2');
+ insert into pt values (1, 1, 'foo');
+ insert into pt values (1, 2, 'bar') returning *;
+  a | b |  c  
+ ---+---+-
+  1 | 2 | bar
+ (1 row)
+ 
+ insert into pt values (2, 1, 'baz') returning *;
+  a | b |  c  
+ ---+---+-
+  2 | 1 | baz
+ (1 row)
+ 
+ select tableoid::regclass, * FROM pt;
+  tableoid | a | 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/01/25 23:33), Stephen Frost wrote:

I'm afraid a good bit of this patch is now failing to apply.  I don't
have much else to say except to echo the performance concern that Amit
Langote raised about expanding the inheritence tree twice.


To address that concern, I'm thinking to redesign the patch so that it 
wouldn't expand the tree at planning time anymore.  I don't have any 
clear solution for that yet, but what I have in mind now is to add new 
FDW APIs to the executor, instead, so that the FDW could 1) create stuff 
such as a query for remote INSERT as PlanForeignModify and 2) 
initialize/end the remote INSERT operation as BeginForeignModify and 
EndForeignModify, somewhere in the executor.  (For #1, I'm thinking to 
add an API for that to ExecSetupPartitionTupleRouting or 
ExecInitPartitionResultRelInfo proposed by the patch by Amit Langote 
[1].)  Anyway, I'll work on this after reviewing that patch, so I'll 
mark this as Returned with feedback.


Thanks for the comment!

Best regards,
Etsuro Fujita

[1] https://commitfest.postgresql.org/16/1415/

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2018/02/02 19:33), Etsuro Fujita wrote:

(2018/01/25 23:33), Stephen Frost wrote:

I'm afraid a good bit of this patch is now failing to apply. I don't
have much else to say except to echo the performance concern that Amit
Langote raised about expanding the inheritence tree twice.


To address that concern, I'm thinking to redesign the patch so that it
wouldn't expand the tree at planning time anymore. I don't have any
clear solution for that yet, but what I have in mind now is to add new
FDW APIs to the executor, instead, so that the FDW could 1) create stuff
such as a query for remote INSERT as PlanForeignModify and 2)
initialize/end the remote INSERT operation as BeginForeignModify and
EndForeignModify, somewhere in the executor. (For #1, I'm thinking to
add an API for that to ExecSetupPartitionTupleRouting or
ExecInitPartitionResultRelInfo proposed by the patch by Amit Langote
[1].) Anyway, I'll work on this after reviewing that patch, so I'll mark
this as Returned with feedback.


CORRECTION: I'm planning to submit a new version to the March CF, so I 
set the status to "Moved to next CF".


Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Stephen Frost]

Etsuro,

* Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote:
> (2017/12/18 23:25), Alvaro Herrera wrote:
> >InitResultRelInfo becomes unintelligible after this patch -- it was
> >straightforward but adding partition_root makes things shaky.  Please
> >add a proper comment indicating what each argument is.
> 
> I was thiking that the comment I added to the definition of the
> ResultRelInfo struct in execnodes.h would make that function intelligible,
> but I agree on that point.  Please fined attached a new version of the patch
> adding such comments.

I'm afraid a good bit of this patch is now failing to apply.  I don't
have much else to say except to echo the performance concern that Amit
Langote raised about expanding the inheritence tree twice.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2017/12/18 23:25), Alvaro Herrera wrote:

InitResultRelInfo becomes unintelligible after this patch -- it was
straightforward but adding partition_root makes things shaky.  Please
add a proper comment indicating what each argument is.


I was thiking that the comment I added to the definition of the 
ResultRelInfo struct in execnodes.h would make that function 
intelligible, but I agree on that point.  Please fined attached a new 
version of the patch adding such comments.


Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 176,182  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
--- 176,188 
  SELECT tableoid::regclass, * FROM pt;
  SELECT tableoid::regclass, * FROM p1;
  SELECT tableoid::regclass, * FROM p2;
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy');
+ \t off
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy');
+ \t off
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
  SELECT tableoid::regclass, * FROM p1;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 315,321  SELECT tableoid::regclass, * FROM p2;
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
--- 315,321 
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR
! ERROR:  cannot route copied tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
***
*** 341,348  SELECT tableoid::regclass, * FROM p2;
   p2   | 2 | qux
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
--- 341,366 
   p2   | 2 | qux
  (2 rows)
  
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy');
+  Insert on public.pt
+Foreign Insert on public.p1
+Insert on public.p2
+->  Result
+  Output: 1, 'xyzzy'::text
+ 
+ \t off
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to foreign table "p1"
! \t on
! EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy');
!  Insert on public.pt
!Foreign Insert on public.p1
!Insert on public.p2
!->  Result
!  Output: 2, 'xyzzy'::text
! 
! \t off
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 7088,7093  NOTICE:  drop cascades to foreign table bar2
--- 7088,7295 
  drop table loct1;
  drop table loct2;
  -- ===
+ -- test tuple routing for foreign-table partitions
+ -- ===
+ create table pt (a int, b int) partition by list (a);
+ create table locp partition of pt for values in (1);
+ create table locfoo (a int check (a in (2)), b int);
+ create foreign table remp partition of pt for values in (2) server loopback options (table_name 'locfoo');
+ explain (verbose, costs off)
+ insert into pt values (1, 1), (2, 1);
+QUERY PLAN
+ -
+  Insert on public.pt
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2)
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (6 rows)
+ 
+ insert into pt values (1, 1), (2, 1);
+ select tableoid::regclass, * FROM pt;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+  remp | 2 | 1
+ (2 rows)
+ 
+ select tableoid::regclass, * FROM locp;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+ (1 row)
+ 
+ select tableoid::regclass, * FROM remp;
+  tableoid | a | b 
+ --+---+---
+  remp | 2 | 1
+ (1 row)
+ 
+ explain (verbose, costs off)
+ insert into pt values (1, 2), (2, 2) returning *;
+QUERY PLAN   
+ 
+  Insert on public.pt
+Output: pt.a, pt.b
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Amit Langote]

On 2017/12/18 23:25, Alvaro Herrera wrote:
> (I wonder why
> this function needs a local variable "partition_check" -- seems
> pointless).

Before 15ce775faa4 [1], there were more than one line where
partition_check was being set, but maybe it still didn't have to be a
separate variable.

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=15ce775faa4

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Alvaro Herrera]

InitResultRelInfo becomes unintelligible after this patch -- it was
straightforward but adding partition_root makes things shaky.  Please
add a proper comment indicating what each argument is.  (I wonder why
this function needs a local variable "partition_check" -- seems
pointless).

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

(2017/12/18 18:14), Amit Langote wrote:

I noticed that this patch introduces a partition_rels field in
ModifyTable, which contains the RT indexes of *all* leaf partitions in the
partition tree.  That means we now expand the partition inheritance tree
in the planner even in the INSERT case, simply because some of the leaf
partitions might be foreign tables which must be looked at by the planner.


Yeah, the patch expands the inheritance tree at planning time as well to 
build an RTE for each partition so that the FDW can use that RTE eg, 
when called from PlanForeignModify.



  I'm somewhat concerned about the performance implications of that.  Now,
to insert even a single row into a partitioned table, which may not even
be routed to any of its foreign partitions, we are going to have to expand
the inheritance twice -- once by the planner to handle foreign partitions
and then by the executor when setting up the tuple routing information.

Is there any reason why we have to to things this way, beside the fact
that the PlanForeignModify() API appears to be callable from only within a
valid planner context?


Another reason for that is for set_plan_references to get such RTEs to 
record plan dependencies so that plancache.c invalidates cached plans 
for foreign partitions.


I suspect that we could avoid the planning-time expansion by doing some 
optimization when inserting a single row into a partitioned table.


Best regards,
Etsuro Fujita

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Etsuro Fujita]

Hi Maksim,

(2017/12/12 17:57), Maksim Milyutin wrote:

Your patch already is not applied on master. Please rebase it.


Done.  Please find attached an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 176,182  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
--- 176,188 
  SELECT tableoid::regclass, * FROM pt;
  SELECT tableoid::regclass, * FROM p1;
  SELECT tableoid::regclass, * FROM p2;
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy');
+ \t off
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy');
+ \t off
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
  SELECT tableoid::regclass, * FROM p1;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 315,321  SELECT tableoid::regclass, * FROM p2;
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
--- 315,321 
  (0 rows)
  
  COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR
! ERROR:  cannot route copied tuples to a foreign table
  CONTEXT:  COPY pt, line 2: "1,qux"
  COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ',');
  SELECT tableoid::regclass, * FROM pt;
***
*** 341,348  SELECT tableoid::regclass, * FROM p2;
   p2   | 2 | qux
  (2 rows)
  
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to a foreign table
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
--- 341,366 
   p2   | 2 | qux
  (2 rows)
  
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy');
+  Insert on public.pt
+Foreign Insert on public.p1
+Insert on public.p2
+->  Result
+  Output: 1, 'xyzzy'::text
+ 
+ \t off
  INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR
! ERROR:  cannot route inserted tuples to foreign table "p1"
! \t on
! EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy');
!  Insert on public.pt
!Foreign Insert on public.p1
!Insert on public.p2
!->  Result
!  Output: 2, 'xyzzy'::text
! 
! \t off
  INSERT INTO pt VALUES (2, 'xyzzy');
  SELECT tableoid::regclass, * FROM pt;
   tableoid | a |   b   
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 7088,7093  NOTICE:  drop cascades to foreign table bar2
--- 7088,7295 
  drop table loct1;
  drop table loct2;
  -- ===
+ -- test tuple routing for foreign-table partitions
+ -- ===
+ create table pt (a int, b int) partition by list (a);
+ create table locp partition of pt for values in (1);
+ create table locfoo (a int check (a in (2)), b int);
+ create foreign table remp partition of pt for values in (2) server loopback options (table_name 'locfoo');
+ explain (verbose, costs off)
+ insert into pt values (1, 1), (2, 1);
+QUERY PLAN
+ -
+  Insert on public.pt
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2)
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (6 rows)
+ 
+ insert into pt values (1, 1), (2, 1);
+ select tableoid::regclass, * FROM pt;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+  remp | 2 | 1
+ (2 rows)
+ 
+ select tableoid::regclass, * FROM locp;
+  tableoid | a | b 
+ --+---+---
+  locp | 1 | 1
+ (1 row)
+ 
+ select tableoid::regclass, * FROM remp;
+  tableoid | a | b 
+ --+---+---
+  remp | 2 | 1
+ (1 row)
+ 
+ explain (verbose, costs off)
+ insert into pt values (1, 2), (2, 2) returning *;
+QUERY PLAN   
+ 
+  Insert on public.pt
+Output: pt.a, pt.b
+Insert on public.locp
+Foreign Insert on public.remp
+  Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2) RETURNING a, b
+->  Values Scan on "*VALUES*"
+  Output: "*VALUES*".column1, "*VALUES*".column2
+ (7 rows)
+ 
+ insert into pt values (1, 2), (2, 2) returning *;
+  a | b 
+ ---+---
+  1 | 2
+  2 | 2
+ (2 rows)
+ 
+ select 

Re: [HACKERS] Add support for tuple routing to foreign partitions

[Maksim Milyutin]

Hi, Fujita-san!


On 24.11.2017 16:03, Etsuro Fujita wrote:

(2017/10/27 20:00), Etsuro Fujita wrote:

Please find attached an updated version of the patch.


Amit rebased this patch and sent me the rebased version off list. 
Thanks for that, Amit!


One thing I noticed I overlooked is about this change I added to 
make_modifytable to make a valid-looking plan node to pass to 
PlanForeignModify to plan remote insert to each foreign partition:


+   /*
+    * The column list of the child might have a different 
column
+    * order and/or a different set of dropped columns 
than that

+    * of its parent, so adjust the subplan's tlist as well.
+    */
+   tlist = preprocess_targetlist(root,
+ child_parse->targetList);

This would be needed because the FDW might reference the tlist. Since 
preprocess_targetlist references root->parse, it's needed to replace 
that with the child query before calling that function, but I forgot 
to do that.  So I fixed that.  Attached is an updated version of the 
patch.


Your patch already is not applied on master. Please rebase it.

--
Regards,
Maksim Milyutin