Re: BEFORE UPDATE trigger on postgres_fdw table not work

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