Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Robert Haas]

On Thu, Jan 28, 2016 at 7:36 AM, Etsuro Fujita
 wrote:
> Attached is that version of the patch.
>
> I think that postgres_fdw might be able to insert a tableoid value in the
> returned slot in e.g., postgresExecForeignInsert if AFTER ROW Triggers or
> RETURNING expressions reference that value, but I didn't do anything about
> that.

Thanks.  I went with the earlier version, but split it into two commits.

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


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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/28 12:58, Robert Haas wrote:

On Thu, Jan 21, 2016 at 4:05 AM, Etsuro Fujita
 wrote:

By the way, I'm not too sure I understand the need for the core
changes that are part of this patch, and I think that point merits
some discussion.  Whenever you change core like this, you're changing
the contract between the FDW and core; it's not just postgres_fdw that
needs updating, but every FDW.  So we'd better be pretty sure we need
these changes and they are adequately justified before we think about
putting them into the tree.  Are these core changes really needed
here, or can we fix this whole issue in postgres_fdw and leave the
core code alone?



Well, if we think it is the FDW's responsibility to insert a valid value for
tableoid in the returned slot during ExecForeignInsert, ExecForeignUpdate or
ExecForeignDelete, we don't need those core changes.  However, I think it
would be better that that's done by ModifyTable in the same way as
ForeignScan does in ForeignNext, IMO. That eliminates the need for
postgres_fdw or any other FDW to do that business in the callback routines.



I'm not necessarily opposed to the core changes, but I want to
understand better what complexity they are avoiding.  Can you send a
version of this patch that only touches postgres_fdw, so I can
compare?


Attached is that version of the patch.

I think that postgres_fdw might be able to insert a tableoid value in 
the returned slot in e.g., postgresExecForeignInsert if AFTER ROW 
Triggers or RETURNING expressions reference that value, but I didn't do 
anything about that.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 110,115  static void deparseTargetList(StringInfo buf,
--- 110,116 
    PlannerInfo *root,
    Index rtindex,
    Relation rel,
+   bool is_returning,
    Bitmapset *attrs_used,
    List **retrieved_attrs);
  static void deparseReturningList(StringInfo buf, PlannerInfo *root,
***
*** 724,730  deparseSelectSql(StringInfo buf,
  	 * Construct SELECT list
  	 */
  	appendStringInfoString(buf, "SELECT ");
! 	deparseTargetList(buf, root, baserel->relid, rel, attrs_used,
  	  retrieved_attrs);
  
  	/*
--- 725,731 
  	 * Construct SELECT list
  	 */
  	appendStringInfoString(buf, "SELECT ");
! 	deparseTargetList(buf, root, baserel->relid, rel, false, attrs_used,
  	  retrieved_attrs);
  
  	/*
***
*** 738,744  deparseSelectSql(StringInfo buf,
  
  /*
   * Emit a target list that retrieves the columns specified in attrs_used.
!  * This is used for both SELECT and RETURNING targetlists.
   *
   * The tlist text is appended to buf, and we also create an integer List
   * of the columns being retrieved, which is returned to *retrieved_attrs.
--- 739,746 
  
  /*
   * Emit a target list that retrieves the columns specified in attrs_used.
!  * This is used for both SELECT and RETURNING targetlists; the is_returning
!  * parameter is true only for a RETURNING targetlist.
   *
   * The tlist text is appended to buf, and we also create an integer List
   * of the columns being retrieved, which is returned to *retrieved_attrs.
***
*** 748,753  deparseTargetList(StringInfo buf,
--- 750,756 
    PlannerInfo *root,
    Index rtindex,
    Relation rel,
+   bool is_returning,
    Bitmapset *attrs_used,
    List **retrieved_attrs)
  {
***
*** 777,782  deparseTargetList(StringInfo buf,
--- 780,787 
  		{
  			if (!first)
  appendStringInfoString(buf, ", ");
+ 			else if (is_returning)
+ appendStringInfoString(buf, " RETURNING ");
  			first = false;
  
  			deparseColumnRef(buf, rtindex, i, root);
***
*** 794,799  deparseTargetList(StringInfo buf,
--- 799,806 
  	{
  		if (!first)
  			appendStringInfoString(buf, ", ");
+ 		else if (is_returning)
+ 			appendStringInfoString(buf, " RETURNING ");
  		first = false;
  
  		appendStringInfoString(buf, "ctid");
***
*** 803,809  deparseTargetList(StringInfo buf,
  	}
  
  	/* Don't generate bad syntax if no undropped columns */
! 	if (first)
  		appendStringInfoString(buf, "NULL");
  }
  
--- 810,816 
  	}
  
  	/* Don't generate bad syntax if no undropped columns */
! 	if (first && !is_returning)
  		appendStringInfoString(buf, "NULL");
  }
  
***
*** 1022,1032  deparseReturningList(StringInfo buf, PlannerInfo *root,
  	}
  
  	if (attrs_used != NULL)
! 	{
! 		appendStringInfoString(buf, " RETURNING ");
! 		deparseTargetList(buf, root, rtindex, rel, attrs_used,
  		  retrieved_attrs);
- 	}
  	else
  		*retrieved_attrs = NIL;
  }
--- 1029,1036 
  	}
  
  	if (attrs_used != NULL)
! 		deparseTargetList(buf, root, rtindex, rel, true, attrs_used,
  		  retrieved_attrs);
  	else
  		*retrieved_attrs = NIL;
  }
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- 

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Robert Haas]

On Thu, Jan 21, 2016 at 4:05 AM, Etsuro Fujita
 wrote:
>> By the way, I'm not too sure I understand the need for the core
>> changes that are part of this patch, and I think that point merits
>> some discussion.  Whenever you change core like this, you're changing
>> the contract between the FDW and core; it's not just postgres_fdw that
>> needs updating, but every FDW.  So we'd better be pretty sure we need
>> these changes and they are adequately justified before we think about
>> putting them into the tree.  Are these core changes really needed
>> here, or can we fix this whole issue in postgres_fdw and leave the
>> core code alone?
>
> Well, if we think it is the FDW's responsibility to insert a valid value for
> tableoid in the returned slot during ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete, we don't need those core changes.  However, I think it
> would be better that that's done by ModifyTable in the same way as
> ForeignScan does in ForeignNext, IMO. That eliminates the need for
> postgres_fdw or any other FDW to do that business in the callback routines.

I'm not necessarily opposed to the core changes, but I want to
understand better what complexity they are avoiding.  Can you send a
version of this patch that only touches postgres_fdw, so I can
compare?

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


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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/21 5:06, Robert Haas wrote:

On Wed, Jan 20, 2016 at 4:50 AM, Etsuro Fujita
 wrote:

My concern about that is that would make the code in deparseTargetList()
complicated.

Essentially, I think your propossal needs a two-pass algorithm for
deparseTargetList; (1) create an integer List of the columns being retrieved
from the given attrs_used (getRetrievedAttrs()), and (2) print those columns
(printRetrievedAttrs()).  How about sharing those two functions between
deparseTargetList and deparseReturningList?:



I don't see why we'd need that.  I adjusted the code in postgres_fdw
along the lines I had in mind and am attaching the result.  It doesn't
look complicated to me, and it passes the regression test you wrote.


Thanks for the patch!  From the patch, I correctly understand what you 
proposed.  Good idea!



By the way, I'm not too sure I understand the need for the core
changes that are part of this patch, and I think that point merits
some discussion.  Whenever you change core like this, you're changing
the contract between the FDW and core; it's not just postgres_fdw that
needs updating, but every FDW.  So we'd better be pretty sure we need
these changes and they are adequately justified before we think about
putting them into the tree.  Are these core changes really needed
here, or can we fix this whole issue in postgres_fdw and leave the
core code alone?


Well, if we think it is the FDW's responsibility to insert a valid value 
for tableoid in the returned slot during ExecForeignInsert, 
ExecForeignUpdate or ExecForeignDelete, we don't need those core 
changes.  However, I think it would be better that that's done by 
ModifyTable in the same way as ForeignScan does in ForeignNext, IMO. 
That eliminates the need for postgres_fdw or any other FDW to do that 
business in the callback routines.


Best regards,
Etsuro Fujita




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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/19 19:04, Thom Brown wrote:

On 12 January 2016 at 11:49, Etsuro Fujita  wrote:

On 2016/01/12 20:36, Thom Brown wrote:



On 8 January 2016 at 05:08, Etsuro Fujita 
wrote:



On 2016/01/06 20:37, Thom Brown wrote:


I've run into an issue:

*# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
tableoid::regclass;
ERROR:
CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
WHERE ((id = 16)) RETURNING NULL



While working on this, I noticed that the existing postgres_fdw system
shows
similar behavior, so I changed the subject.

IIUC, the reason for that is when the local query specifies "RETURNING
tableoid::regclass", the FDW has fmstate->has_returning=false while the
remote query executed at ModifyTable has "RETURNING NULL", as shown in
the
above example; that would cause an abnormal exit in executing the remote
query in postgresExecForeignUpdate, since that the FDW would get
PGRES_TUPLES_OK as a result of the query while the FDW would think that
the
right result to get should be PGRES_COMMAND_OK, from the flag
fmstate->has_returning=false.



Attached is a patch to fix that.



I can't apply this patch in tandem with FDW DML pushdown patch (either
v2 or v3).



That patch is for fixing the similar issue in the existing postgres_fdw
system.  So, please apply that patch without the DML pushdown patch.  If
that patch is reasonable as a fix for the issue, I'll update the DML
pushdown patch (v3) on top of that patch.



The patch seems to work for me:

Before:

*# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
tableoid::regclass;
ERROR:
CONTEXT:  Remote SQL command: UPDATE public.customers SET id = $2
WHERE ctid = $1 RETURNING NULL

After:

*# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
tableoid::regclass;
  tableoid
--
  remote.customers
(1 row)

UPDATE 1


Thanks for the testing!

I updated the DML pushdown patch on top of Robert's version of this 
bugfix patch.  Please see


http://www.postgresql.org/message-id/56a0a9f0.9090...@lab.ntt.co.jp

Best regards,
Etsuro Fujita




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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Robert Haas]

On Wed, Jan 20, 2016 at 4:50 AM, Etsuro Fujita
 wrote:
> My concern about that is that would make the code in deparseTargetList()
> complicated.
>
> Essentially, I think your propossal needs a two-pass algorithm for
> deparseTargetList; (1) create an integer List of the columns being retrieved
> from the given attrs_used (getRetrievedAttrs()), and (2) print those columns
> (printRetrievedAttrs()).  How about sharing those two functions between
> deparseTargetList and deparseReturningList?:

I don't see why we'd need that.  I adjusted the code in postgres_fdw
along the lines I had in mind and am attaching the result.  It doesn't
look complicated to me, and it passes the regression test you wrote.

By the way, I'm not too sure I understand the need for the core
changes that are part of this patch, and I think that point merits
some discussion.  Whenever you change core like this, you're changing
the contract between the FDW and core; it's not just postgres_fdw that
needs updating, but every FDW.  So we'd better be pretty sure we need
these changes and they are adequately justified before we think about
putting them into the tree.  Are these core changes really needed
here, or can we fix this whole issue in postgres_fdw and leave the
core code alone?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index e59af2c..12a1031 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -110,6 +110,7 @@ static void deparseTargetList(StringInfo buf,
   PlannerInfo *root,
   Index rtindex,
   Relation rel,
+  bool is_returning,
   Bitmapset *attrs_used,
   List **retrieved_attrs);
 static void deparseReturningList(StringInfo buf, PlannerInfo *root,
@@ -724,7 +725,7 @@ deparseSelectSql(StringInfo buf,
 	 * Construct SELECT list
 	 */
 	appendStringInfoString(buf, "SELECT ");
-	deparseTargetList(buf, root, baserel->relid, rel, attrs_used,
+	deparseTargetList(buf, root, baserel->relid, rel, false, attrs_used,
 	  retrieved_attrs);
 
 	/*
@@ -738,7 +739,8 @@ deparseSelectSql(StringInfo buf,
 
 /*
  * Emit a target list that retrieves the columns specified in attrs_used.
- * This is used for both SELECT and RETURNING targetlists.
+ * This is used for both SELECT and RETURNING targetlists; the is_returning
+ * parameter is true only for a RETURNING targetlist.
  *
  * The tlist text is appended to buf, and we also create an integer List
  * of the columns being retrieved, which is returned to *retrieved_attrs.
@@ -748,6 +750,7 @@ deparseTargetList(StringInfo buf,
   PlannerInfo *root,
   Index rtindex,
   Relation rel,
+  bool is_returning,
   Bitmapset *attrs_used,
   List **retrieved_attrs)
 {
@@ -777,6 +780,8 @@ deparseTargetList(StringInfo buf,
 		{
 			if (!first)
 appendStringInfoString(buf, ", ");
+			else if (is_returning)
+appendStringInfoString(buf, " RETURNING ");
 			first = false;
 
 			deparseColumnRef(buf, rtindex, i, root);
@@ -794,6 +799,8 @@ deparseTargetList(StringInfo buf,
 	{
 		if (!first)
 			appendStringInfoString(buf, ", ");
+		else if (is_returning)
+			appendStringInfoString(buf, " RETURNING ");
 		first = false;
 
 		appendStringInfoString(buf, "ctid");
@@ -803,7 +810,7 @@ deparseTargetList(StringInfo buf,
 	}
 
 	/* Don't generate bad syntax if no undropped columns */
-	if (first)
+	if (first && !is_returning)
 		appendStringInfoString(buf, "NULL");
 }
 
@@ -1022,11 +1029,8 @@ deparseReturningList(StringInfo buf, PlannerInfo *root,
 	}
 
 	if (attrs_used != NULL)
-	{
-		appendStringInfoString(buf, " RETURNING ");
-		deparseTargetList(buf, root, rtindex, rel, attrs_used,
+		deparseTargetList(buf, root, rtindex, rel, true, attrs_used,
 		  retrieved_attrs);
-	}
 	else
 		*retrieved_attrs = NIL;
 }
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index b471c67..b0d12ac 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2408,6 +2408,59 @@ SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1;
  1104 | 204 | ddd| 
 (819 rows)
 
+EXPLAIN (verbose, costs off)
+INSERT INTO ft2 (c1,c2,c3) VALUES (,999,'foo') RETURNING tableoid::regclass;
+   QUERY PLAN
+-
+ Insert on public.ft2
+   Output: (tableoid)::regclass
+   Remote SQL: INSERT INTO "S 1"."T 1"("C 1", c2, c3, c4, c5, c6, c7, c8) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+   ->  Result
+ Output: , 999, 

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/20 3:42, Robert Haas wrote:

On Tue, Jan 19, 2016 at 1:59 AM, Etsuro Fujita
 wrote:

I've run into an issue:

*# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
tableoid::regclass;
ERROR:
CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
WHERE ((id = 16)) RETURNING NULL



While working on this, I noticed that the existing postgres_fdw system
shows similar behavior, so I changed the subject.

IIUC, the reason for that is when the local query specifies "RETURNING
tableoid::regclass", the FDW has fmstate->has_returning=false while the
remote query executed at ModifyTable has "RETURNING NULL", as shown in
the above example; that would cause an abnormal exit in executing the
remote query in postgresExecForeignUpdate, since that the FDW would get
PGRES_TUPLES_OK as a result of the query while the FDW would think that
the right result to get should be PGRES_COMMAND_OK, from the flag
fmstate->has_returning=false.

Attached is a patch to fix that.



I added this to the next CF.

https://commitfest.postgresql.org/9/483/



Uggh, what a mess.  How about passing an additional boolean
"is_returning" to deparseTargetList()?   If false, then
deparseTargetList() behaves as now.  If false, then
deparseTargetList() doesn't append anything at all if there are no
columns to return, instead of (as at present) adding NULL.  On the
other hand, if there are columns to return, then it appends "
RETURNING " before the first column.  Then, deparseReturningList could
skip adding RETURNING itself, and just pass true to
deparseTargetList().  The advantage of this approach is that we don't
end up with two copies of the code that have to stay synchronized -


Thanks for the review!  I think that is important.


the decision is made inside deparseTargetList(), and
deparseReturningList() accepts the results.


My concern about that is that would make the code in deparseTargetList() 
complicated.


Essentially, I think your propossal needs a two-pass algorithm for 
deparseTargetList; (1) create an integer List of the columns being 
retrieved from the given attrs_used (getRetrievedAttrs()), and (2) print 
those columns (printRetrievedAttrs()).  How about sharing those two 
functions between deparseTargetList and deparseReturningList?:


* In deparseTargetList, perform getRetrievedAttrs().  If 
getRetrievedAttrs()!=NIL, perform printRetrievedAttrs().  Otherwise, 
print NULL.
* In deparseReturningList, perform getRetrievedAttrs() before adding 
RETURNING.  If getRetrievedAttrs()!=NIL, print RETURNING and perform 
printRetrievedAttrs().  Otherwise, do nothing.


Best regards,
Etsuro Fujita




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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Thom Brown]

On 12 January 2016 at 11:49, Etsuro Fujita  wrote:
> On 2016/01/12 20:36, Thom Brown wrote:
>>
>> On 8 January 2016 at 05:08, Etsuro Fujita 
>> wrote:
>
>
 On 2016/01/06 20:37, Thom Brown wrote:
>
> I've run into an issue:
>
> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
> tableoid::regclass;
> ERROR:
> CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
> WHERE ((id = 16)) RETURNING NULL
>
>
>>> While working on this, I noticed that the existing postgres_fdw system
>>> shows
>>> similar behavior, so I changed the subject.
>>>
>>> IIUC, the reason for that is when the local query specifies "RETURNING
>>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>>> the
>>> above example; that would cause an abnormal exit in executing the remote
>>> query in postgresExecForeignUpdate, since that the FDW would get
>>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>>> the
>>> right result to get should be PGRES_COMMAND_OK, from the flag
>>> fmstate->has_returning=false.
>
>
>>> Attached is a patch to fix that.
>
>
>> I can't apply this patch in tandem with FDW DML pushdown patch (either
>> v2 or v3).
>
>
> That patch is for fixing the similar issue in the existing postgres_fdw
> system.  So, please apply that patch without the DML pushdown patch.  If
> that patch is reasonable as a fix for the issue, I'll update the DML
> pushdown patch (v3) on top of that patch.

The patch seems to work for me:

Before:

*# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
tableoid::regclass;
ERROR:
CONTEXT:  Remote SQL command: UPDATE public.customers SET id = $2
WHERE ctid = $1 RETURNING NULL

After:

*# UPDATE master_customers SET id = 22 WHERE id = 1 RETURNING
tableoid::regclass;
 tableoid
--
 remote.customers
(1 row)

UPDATE 1

Thom


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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Robert Haas]

On Tue, Jan 19, 2016 at 1:59 AM, Etsuro Fujita
 wrote:
 I've run into an issue:

 *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
 tableoid::regclass;
 ERROR:
 CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
 WHERE ((id = 16)) RETURNING NULL
>
>> While working on this, I noticed that the existing postgres_fdw system
>> shows similar behavior, so I changed the subject.
>>
>> IIUC, the reason for that is when the local query specifies "RETURNING
>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>> the above example; that would cause an abnormal exit in executing the
>> remote query in postgresExecForeignUpdate, since that the FDW would get
>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>> the right result to get should be PGRES_COMMAND_OK, from the flag
>> fmstate->has_returning=false.
>>
>> Attached is a patch to fix that.
>
> I added this to the next CF.
>
> https://commitfest.postgresql.org/9/483/

Uggh, what a mess.  How about passing an additional boolean
"is_returning" to deparseTargetList()?   If false, then
deparseTargetList() behaves as now.  If false, then
deparseTargetList() doesn't append anything at all if there are no
columns to return, instead of (as at present) adding NULL.  On the
other hand, if there are columns to return, then it appends "
RETURNING " before the first column.  Then, deparseReturningList could
skip adding RETURNING itself, and just pass true to
deparseTargetList().  The advantage of this approach is that we don't
end up with two copies of the code that have to stay synchronized -
the decision is made inside deparseTargetList(), and
deparseReturningList() accepts the results.

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


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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/08 14:08, Etsuro Fujita wrote:

On 2016/01/07 21:50, Etsuro Fujita wrote:

On 2016/01/06 20:37, Thom Brown wrote:



I've run into an issue:

*# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
tableoid::regclass;
ERROR:
CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
WHERE ((id = 16)) RETURNING NULL



While working on this, I noticed that the existing postgres_fdw system
shows similar behavior, so I changed the subject.

IIUC, the reason for that is when the local query specifies "RETURNING
tableoid::regclass", the FDW has fmstate->has_returning=false while the
remote query executed at ModifyTable has "RETURNING NULL", as shown in
the above example; that would cause an abnormal exit in executing the
remote query in postgresExecForeignUpdate, since that the FDW would get
PGRES_TUPLES_OK as a result of the query while the FDW would think that
the right result to get should be PGRES_COMMAND_OK, from the flag
fmstate->has_returning=false.

Attached is a patch to fix that.


I added this to the next CF.

https://commitfest.postgresql.org/9/483/

Best regards,
Etsuro Fujita




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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Thom Brown]

On 8 January 2016 at 05:08, Etsuro Fujita  wrote:
> On 2016/01/07 21:50, Etsuro Fujita wrote:
>>
>> On 2016/01/06 20:37, Thom Brown wrote:
>>>
>>> On 25 December 2015 at 10:00, Etsuro Fujita
>>>  wrote:

 Attached is an updated version of the patch, which is
 still WIP, but I'd be happy if I could get any feedback.
>
>
>>> I've run into an issue:
>>>
>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>> tableoid::regclass;
>>> ERROR:
>>> CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
>>> WHERE ((id = 16)) RETURNING NULL
>
>
>> Will fix.
>
>
> While working on this, I noticed that the existing postgres_fdw system shows
> similar behavior, so I changed the subject.
>
> IIUC, the reason for that is when the local query specifies "RETURNING
> tableoid::regclass", the FDW has fmstate->has_returning=false while the
> remote query executed at ModifyTable has "RETURNING NULL", as shown in the
> above example; that would cause an abnormal exit in executing the remote
> query in postgresExecForeignUpdate, since that the FDW would get
> PGRES_TUPLES_OK as a result of the query while the FDW would think that the
> right result to get should be PGRES_COMMAND_OK, from the flag
> fmstate->has_returning=false.
>
> Attached is a patch to fix that.

I can't apply this patch in tandem with FDW DML pushdown patch (either
v2 or v3).

Thom


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

Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)

[Etsuro Fujita]

On 2016/01/12 20:36, Thom Brown wrote:

On 8 January 2016 at 05:08, Etsuro Fujita  wrote:



On 2016/01/06 20:37, Thom Brown wrote:

I've run into an issue:

*# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
tableoid::regclass;
ERROR:
CONTEXT:  Remote SQL command: UPDATE public.customers SET id = 22
WHERE ((id = 16)) RETURNING NULL



While working on this, I noticed that the existing postgres_fdw system shows
similar behavior, so I changed the subject.

IIUC, the reason for that is when the local query specifies "RETURNING
tableoid::regclass", the FDW has fmstate->has_returning=false while the
remote query executed at ModifyTable has "RETURNING NULL", as shown in the
above example; that would cause an abnormal exit in executing the remote
query in postgresExecForeignUpdate, since that the FDW would get
PGRES_TUPLES_OK as a result of the query while the FDW would think that the
right result to get should be PGRES_COMMAND_OK, from the flag
fmstate->has_returning=false.



Attached is a patch to fix that.



I can't apply this patch in tandem with FDW DML pushdown patch (either
v2 or v3).


That patch is for fixing the similar issue in the existing postgres_fdw 
system.  So, please apply that patch without the DML pushdown patch.  If 
that patch is reasonable as a fix for the issue, I'll update the DML 
pushdown patch (v3) on top of that patch.


Best regards,
Etsuro Fujita




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