Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)
On 2016/02/05 12:28, Robert Haas wrote: 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. Thank you! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)
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)
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 --- b/contrib/postgres_fdw/expected/pos
Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)
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)
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)
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)
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, NULL::integer, 'foo'::text, NULL::times
Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)
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)
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)
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)
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)
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
Re: Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)
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
Odd behavior in foreign table modification (Was: Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW)
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. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/deparse.c --- b/contrib/postgres_fdw/deparse.c *** *** 1003,1008 deparseReturningList(StringInfo buf, PlannerInfo *root, --- 1003,1009 List **retrieved_attrs) { Bitmapset *attrs_used = NULL; + bool has_returning = false; if (trig_after_row) { *** *** 1021,1031 deparseReturningList(StringInfo buf, PlannerInfo *root, --- 1022,1072 &attrs_used); } + /* + * Check to see whether the remote query has a RETURNING clause. + * + * XXX be careful to keep this in sync with deparseTargetList. + */ if (attrs_used != NULL) { + if (bms_is_member(SelfItemPointerAttributeNumber - FirstLowInvalidHeapAttributeNumber, + attrs_used)) + has_returning = true; + else + { + TupleDesc tupdesc = RelationGetDescr(rel); + bool have_wholerow; + int i; + + /* If there's a whole-row reference, we'll need all the columns. */ + have_wholerow = bms_is_member(0 - FirstLowInvalidHeapAttributeNumber, + attrs_used); + + for (i = 1; i <= tupdesc->natts; i++) + { + Form_pg_attribute attr = tupdesc->attrs[i - 1]; + + /* Ignore dropped attributes. */ + if (attr->attisdropped) + continue; + + if (have_wholerow || + bms_is_member(i - FirstLowInvalidHeapAttributeNumber, + attrs_used)) + { + has_returning = true; + break; + } + } + } + } + + if (has_returning != false) + { appendStringInfoString(buf, " RETURNING "); deparseTargetList(buf, root, rtindex, rel, attrs_used, retrieved_attrs); + Assert(*retrieved_attrs != NIL); } else *retrieved_attrs = NIL; *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 2408,2413 SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1; --- 2408,2466 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, NULL::integer, 'foo'::text, NULL::timestamp with time zone, NULL::timestamp without time zone, NULL::character varying, 'ft2 '::character(10), NULL::user_enum + (5 rows) + + INSERT INTO ft2 (c1,c2,c3) VALUES (,999,'foo') RETURNING tableoid::regclass; + tableoid + -- + ft2 + (1 row) + + EXPLAIN (verbose, costs off) + UPDATE ft2 SET c3 = 'bar' WHERE c1 = RETURNING tableoid::regclass; + QUERY PLAN + --- + Update on public.ft2 +Output: (tableoid)::regclass +Remote SQL: UPDATE "S 1"."T 1" SET c3 = $2 WHERE ctid = $1 +-> Foreign Scan on public.ft2 + Output: c1, c2, NULL::integer, 'bar'::text, c4, c5, c6, c7, c8, ctid + Remote SQL: SELECT "C 1", c2, c4, c5, c6, c7, c8, ctid FROM "S 1"."T 1" WHERE (("C 1" = )) FOR UPDATE + (6 rows) + + UPDATE ft2 SET c3 = 'bar' WHERE c1 = RETURNING tableoid::regclass; + tableoid + -