On 2016/01/28 12:58, Robert Haas wrote:
On Thu, Jan 21, 2016 at 4:05 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> 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/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 (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
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 1614,1619 **** postgresExecForeignInsert(EState *estate,
--- 1614,1620 ----
const char **p_values;
PGresult *res;
int n_rows;
+ HeapTuple tuple;
/* Set up the prepared statement on the remote server, if we didn't yet */
if (!fmstate->p_name)
***************
*** 1654,1659 **** postgresExecForeignInsert(EState *estate,
--- 1655,1667 ----
MemoryContextReset(fmstate->temp_cxt);
+ /*
+ * AFTER ROW Triggers or RETURNING expressions might reference the tableoid
+ * column, so initialize t_tableOid before evaluating them.
+ */
+ tuple = ExecMaterializeSlot(slot);
+ tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
+
/* Return NULL if nothing was inserted on the remote end */
return (n_rows > 0) ? slot : NULL;
}
***************
*** 1674,1679 **** postgresExecForeignUpdate(EState *estate,
--- 1682,1688 ----
const char **p_values;
PGresult *res;
int n_rows;
+ HeapTuple tuple;
/* Set up the prepared statement on the remote server, if we didn't yet */
if (!fmstate->p_name)
***************
*** 1724,1729 **** postgresExecForeignUpdate(EState *estate,
--- 1733,1745 ----
MemoryContextReset(fmstate->temp_cxt);
+ /*
+ * AFTER ROW Triggers or RETURNING expressions might reference the tableoid
+ * column, so initialize t_tableOid before evaluating them.
+ */
+ tuple = ExecMaterializeSlot(slot);
+ tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
+
/* Return NULL if nothing was updated on the remote end */
return (n_rows > 0) ? slot : NULL;
}
***************
*** 1744,1749 **** postgresExecForeignDelete(EState *estate,
--- 1760,1766 ----
const char **p_values;
PGresult *res;
int n_rows;
+ HeapTuple tuple;
/* Set up the prepared statement on the remote server, if we didn't yet */
if (!fmstate->p_name)
***************
*** 1794,1799 **** postgresExecForeignDelete(EState *estate,
--- 1811,1825 ----
MemoryContextReset(fmstate->temp_cxt);
+ /*
+ * 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(resultRelInfo->ri_RelationDesc);
+
/* Return NULL if nothing was deleted on the remote end */
return (n_rows > 0) ? slot : NULL;
}
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 413,418 **** EXPLAIN (verbose, costs off)
--- 413,427 ----
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 $$
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
