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
<fujita.ets...@lab.ntt.co.jp> 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 (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/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 $$
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***************
*** 308,313 **** ExecInsert(ModifyTableState *mtstate,
--- 308,319 ----
/* 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,566 **** ExecDelete(ItemPointer tupleid,
--- 567,574 ----
}
else if (resultRelInfo->ri_FdwRoutine)
{
+ HeapTuple tuple;
+
/*
* delete from foreign table: let the FDW do it
*
***************
*** 579,584 **** ExecDelete(ItemPointer tupleid,
--- 587,601 ----
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,843 **** ExecUpdate(ItemPointer tupleid,
--- 855,866 ----
/* 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
