Mochizuki-san, On 2019/05/27 10:52, Shohei Mochizuki wrote: > 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. > > 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.
Indeed. That seems like a bug to me. > This is because current fdw code adds only columns to RemoteSQL that were > explicitly targets of the UPDATE as follows. Yeah. So, the trigger execution correctly modifies the existing tuple fetched from the remote server, but those changes are then essentially discarded by postgres_fdw, that is, postgresExecForeignModify(). > 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. Thanks for the patch. It seems to fix the problem as far as I can see. > 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. I think that the approach in your patch may be fine, but others may disagree. We don't require row triggers' definition to declare which columns of the input row it intends to modify. Without that information, the planner can't determine the exact set of changed columns to transmit to the remote server. So it's too early, for example, for PlanForeignModify() to construct an optimal update query which transmits only the columns that are changed, including those that may be modified by triggers. If the FDW had delayed the construction of the exact update query to ExecForeignUpdate(), we could build a more optimal update query, because by then we will know *all* columns that have changed, including those that are changed by BEFORE UPDATE row triggers if any. Maybe other FDWs beside postgres_fdw do that already, so it's possible to rejigger postgres_fdw to do that too. But considering that such rejiggering is only necessary for efficiency, I'm not sure if others will agree to pursuing it, especially if it requires too much code change. Also, in the worst case, we'll end up generating new query for every row being changed, because the trigger may change different columns for different rows based on some condition. Thanks, Amit
