Hi,
I noticed returning a modified record in a row-level BEFORE UPDATE trigger
on postgres_fdw foreign tables do not work. Attached patch fixes this issue.
Below are scenarios similar to postgres_fdw test to reproduce the issue.
postgres=# CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(dbname 'postgres',port '5432');
postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
postgres=# create table loc1 (f1 serial, f2 text);
postgres=# create foreign table rem1 (f1 serial, f2 text)
postgres-# server loopback options(table_name 'loc1');
postgres=# CREATE FUNCTION trig_row_before_insupdate() RETURNS TRIGGER AS $$
postgres$# BEGIN
postgres$# NEW.f2 := NEW.f2 || ' triggered !';
postgres$# RETURN NEW;
postgres$# END
postgres$# $$ language plpgsql;
postgres=# CREATE TRIGGER trig_row_before_insupd BEFORE INSERT OR UPDATE ON rem1
postgres-# FOR EACH ROW EXECUTE PROCEDURE trig_row_before_insupdate();
-- insert trigger is OK
postgres=# INSERT INTO rem1 values(1, 'insert');
postgres=# SELECT * FROM rem1;
f1 | f2
----+--------------------
1 | insert triggered !
(1 row)
-- update trigger is OK if we update f2
postgres=# UPDATE rem1 set f2 = 'update';
postgres=# SELECT * FROM rem1;
f1 | f2
----+--------------------
1 | update triggered !
Without attached patch:
postgres=# UPDATE rem1 set f1 = 10;
postgres=# SELECT * FROM rem1;
f1 | f2
----+--------------------
10 | update triggered !
(1 row)
f2 should be updated by trigger, but not.
This is because current fdw code adds only columns to RemoteSQL that were
explicitly targets of the UPDATE as follows.
postgres=# EXPLAIN (verbose, costs off)
UPDATE rem1 set f1 = 10;
QUERY PLAN
---------------------------------------------------------------------
Update on public.rem1
Remote SQL: UPDATE public.loc1 SET f1 = $2 WHERE ctid = $1 <--- not set f2
-> Foreign Scan on public.rem1
Output: 10, f2, ctid, rem1.*
Remote SQL: SELECT f1, f2, ctid FROM public.loc1 FOR UPDATE
(5 rows)
With attached patch, f2 is updated by a trigger and "f2 = $3" is added to
remote SQL
as follows.
postgres=# UPDATE rem1 set f1 = 10;
postgres=# select * from rem1;
f1 | f2
----+--------------------------------
10 | update triggered ! triggered !
(1 row)
postgres=# EXPLAIN (verbose, costs off)
postgres-# UPDATE rem1 set f1 = 10;
QUERY PLAN
-----------------------------------------------------------------------
Update on public.rem1
Remote SQL: UPDATE public.loc1 SET f1 = $2, f2 = $3 WHERE ctid = $1
-> Foreign Scan on public.rem1
Output: 10, f2, ctid, rem1.*
Remote SQL: SELECT f1, f2, ctid FROM public.loc1 FOR UPDATE
(5 rows)
My patch adds all columns to a target list of remote update query
as in INSERT case if a before update trigger exists.
I tried to add only columns modified in trigger to the target list of
a remote update query, but I cannot find simple way to do that because
update query is built during planning phase at postgresPlanForeignModify
while it is difficult to decide which columns are modified by a trigger
until query execution.
Regards,
--
Shohei Mochizuki
TOSHIBA CORPORATION
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index e034b03..546d97b 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -6558,6 +6558,14 @@ SELECT * from loc1;
2 | skidoo triggered !
(2 rows)
+UPDATE rem1 set f1 = 10;
+SELECT * from loc1;
+ f1 | f2
+----+--------------------------------
+ 10 | skidoo triggered ! triggered !
+ 10 | skidoo triggered ! triggered !
+(2 rows)
+
DELETE FROM rem1;
-- Add a second trigger, to check that the changes are propagated correctly
-- from trigger to trigger
@@ -6670,7 +6678,7 @@ NOTICE: trig_row_after(23, skidoo) AFTER ROW INSERT ON
rem1
NOTICE: NEW: (13,"test triggered !")
ctid
--------
- (0,27)
+ (0,29)
(1 row)
-- cleanup
@@ -6774,10 +6782,10 @@ BEFORE UPDATE ON rem1
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
EXPLAIN (verbose, costs off)
UPDATE rem1 set f2 = ''; -- can't be pushed down
- QUERY PLAN
----------------------------------------------------------------------
+ QUERY PLAN
+-----------------------------------------------------------------------
Update on public.rem1
- Remote SQL: UPDATE public.loc1 SET f2 = $2 WHERE ctid = $1
+ Remote SQL: UPDATE public.loc1 SET f1 = $2, f2 = $3 WHERE ctid = $1
-> Foreign Scan on public.rem1
Output: f1, ''::text, ctid, rem1.*
Remote SQL: SELECT f1, f2, ctid FROM public.loc1 FOR UPDATE
@@ -7404,12 +7412,12 @@ AFTER UPDATE OR DELETE ON bar2
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
explain (verbose, costs off)
update bar set f2 = f2 + 100;
- QUERY PLAN
---------------------------------------------------------------------------------------
+ QUERY PLAN
+--------------------------------------------------------------------------------------------------------
Update on public.bar
Update on public.bar
Foreign Update on public.bar2
- Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1,
f2, f3
+ Remote SQL: UPDATE public.loct2 SET f1 = $2, f2 = $3, f3 = $4 WHERE ctid
= $1 RETURNING f1, f2, f3
-> Seq Scan on public.bar
Output: bar.f1, (bar.f2 + 100), bar.ctid
-> Foreign Scan on public.bar2
diff --git a/contrib/postgres_fdw/postgres_fdw.c
b/contrib/postgres_fdw/postgres_fdw.c
index 2b6d885..6a1c7a4 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -1685,13 +1685,19 @@ postgresPlanForeignModify(PlannerInfo *root,
rel = table_open(rte->relid, NoLock);
/*
- * In an INSERT, we transmit all columns that are defined in the foreign
- * table. In an UPDATE, we transmit only columns that were explicitly
- * targets of the UPDATE, so as to avoid unnecessary data transmission.
- * (We can't do that for INSERT since we would miss sending default
values
- * for columns not listed in the source statement.)
- */
- if (operation == CMD_INSERT)
+ * In an INSERT and an UPDATE of a table with a before update trigger,
we
+ * transmit all columns that are defined in the foreign table. In other
+ * UPDATE, we transmit only columns that were explicitly targets of the
+ * UPDATE, so as to avoid unnecessary data transmission. (We can't do
that
+ * for INSERT since we would miss sending default values for columns not
+ * listed in the source statement and for an UPDATE of a table with a
+ * before update trigger since we would miss updating colums modified
in a
+ * trigger but not listed in the souce statement. )
+ */
+ if (operation == CMD_INSERT ||
+ (operation == CMD_UPDATE &&
+ rel->trigdesc &&
+ rel->trigdesc->trig_update_before_row))
{
TupleDesc tupdesc = RelationGetDescr(rel);
int attnum;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 73852f1..3da0293 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1552,6 +1552,8 @@ UPDATE rem1 set f2 = '';
SELECT * from loc1;
UPDATE rem1 set f2 = 'skidoo' RETURNING f2;
SELECT * from loc1;
+UPDATE rem1 set f1 = 10;
+SELECT * from loc1;
DELETE FROM rem1;
