On Wed, Jan 20, 2016 at 4:50 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> 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 (9999,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: 9999, 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 (9999,999,'foo') RETURNING tableoid::regclass;
+ tableoid
+----------
+ ft2
+(1 row)
+
+EXPLAIN (verbose, costs off)
+UPDATE ft2 SET c3 = 'bar' WHERE c1 = 9999 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" = 9999)) FOR UPDATE
+(6 rows)
+
+UPDATE ft2 SET c3 = 'bar' WHERE c1 = 9999 RETURNING tableoid::regclass;
+ tableoid
+----------
+ ft2
+(1 row)
+
+EXPLAIN (verbose, costs off)
+DELETE FROM ft2 WHERE c1 = 9999 RETURNING tableoid::regclass;
+ QUERY PLAN
+------------------------------------------------------------------------------------
+ Delete on public.ft2
+ Output: (tableoid)::regclass
+ Remote SQL: DELETE FROM "S 1"."T 1" WHERE ctid = $1
+ -> Foreign Scan on public.ft2
+ Output: ctid
+ Remote SQL: SELECT ctid FROM "S 1"."T 1" WHERE (("C 1" = 9999)) FOR UPDATE
+(6 rows)
+
+DELETE FROM ft2 WHERE c1 = 9999 RETURNING tableoid::regclass;
+ tableoid
+----------
+ ft2
+(1 row)
+
-- Test that trigger on remote table works as expected
CREATE OR REPLACE FUNCTION "S 1".F_BRTRIG() RETURNS trigger AS $$
BEGIN
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 73fa9f6..4b13654 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -413,6 +413,15 @@ EXPLAIN (verbose, costs off)
DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1;
+EXPLAIN (verbose, costs off)
+INSERT INTO ft2 (c1,c2,c3) VALUES (9999,999,'foo') RETURNING tableoid::regclass;
+INSERT INTO ft2 (c1,c2,c3) VALUES (9999,999,'foo') RETURNING tableoid::regclass;
+EXPLAIN (verbose, costs off)
+UPDATE ft2 SET c3 = 'bar' WHERE c1 = 9999 RETURNING tableoid::regclass;
+UPDATE ft2 SET c3 = 'bar' WHERE c1 = 9999 RETURNING tableoid::regclass;
+EXPLAIN (verbose, costs off)
+DELETE FROM ft2 WHERE c1 = 9999 RETURNING tableoid::regclass;
+DELETE FROM ft2 WHERE c1 = 9999 RETURNING tableoid::regclass;
-- Test that trigger on remote table works as expected
CREATE OR REPLACE FUNCTION "S 1".F_BRTRIG() RETURNS trigger AS $$
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 46299fc..27051e8 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -308,6 +308,12 @@ ExecInsert(ModifyTableState *mtstate,
/* FDW might have changed tuple */
tuple = ExecMaterializeSlot(slot);
+ /*
+ * AFTER ROW Triggers or RETURNING expressions might reference the
+ * tableoid column, so initialize t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
+
newId = InvalidOid;
}
else
@@ -561,6 +567,8 @@ ExecDelete(ItemPointer tupleid,
}
else if (resultRelInfo->ri_FdwRoutine)
{
+ HeapTuple tuple;
+
/*
* delete from foreign table: let the FDW do it
*
@@ -579,6 +587,15 @@ ExecDelete(ItemPointer tupleid,
if (slot == NULL) /* "do nothing" */
return NULL;
+
+ /*
+ * RETURNING expressions might reference the tableoid column, so
+ * initialize t_tableOid before evaluating them.
+ */
+ if (slot->tts_isempty)
+ ExecStoreAllNullTuple(slot);
+ tuple = ExecMaterializeSlot(slot);
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
}
else
{
@@ -838,6 +855,12 @@ ExecUpdate(ItemPointer tupleid,
/* FDW might have changed tuple */
tuple = ExecMaterializeSlot(slot);
+
+ /*
+ * AFTER ROW Triggers or RETURNING expressions might reference the
+ * tableoid column, so initialize t_tableOid before evaluating them.
+ */
+ tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
}
else
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
