Re: Optimization for updating foreign tables in Postgres FDW

Hi Fujita-san,

Issues addressed by Eitoku-san were fixed properly, but he found a bug
and a possible enhancement in the v2 patch.

* push-down check misses delete triggers
update_is_pushdown_safe() seems to have a bug that it misses the
existence of row-level delete trigger. DELETE statement executed
against a foreign table which has row-level delete trigger is pushed
down to remote, and consequently no row-level delete trigger is fired.

* further optimization
Is there any chance to consider further optimization by passing the
operation type (UPDATE|DELETE) of undergoing statement to
update_is_pushdown_safe()? It seems safe to push down UPDATE
statement when the target foreign table has no update trigger even it
has a delete trigger (of course the opposite combination would be also
fine).

* Documentation
The requirement of pushing down UPDATE/DELETE statements would not be
easy to understand for non-expert users, so it seems that there is a
room to enhance documentation. An idea is to define which expression
is safe to send to remote first (it might need to mention the
difference of semantics), and refer the definition from the place
describing the requirement of pushing-down for SELECT, UPDATE and
DELETE.

2014-08-04 20:30 GMT+09:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> (2014/07/30 17:22), Etsuro Fujita wrote:
>>
>> (2014/07/29 0:58), Robert Haas wrote:
>>>
>>> On Fri, Jul 25, 2014 at 3:39 AM, Albe Laurenz
>>> <laurenz(dot)albe(at)wien(dot)gv(dot)at> wrote:
>>>>
>>>> Shigeru Hanada wrote:
>>>>>
>>>>> * Naming of new behavior
>>>>> You named this optimization "Direct Update", but I'm not sure that
>>>>> this is intuitive enough to express this behavior. I would like to
>>>>> hear opinions of native speakers.
>>>>
>>>>
>>>> How about "batch foreign update" or "batch foreign modification"?
>>>> (Disclaimer: I'm not a native speaker either.)
>>>
>>>
>>> I think direct update sounds pretty good. "Batch" does not sound as
>>> good to me, since it doesn't clearly describe what makes this patch
>>> special as opposed to some other grouping of updates that happens to
>>> produce a speedup.
>>
>>
>> I agree with Robert on that point.
>>
>>> Another term that might be used is "update pushdown", since we are
>>> pushing the whole update to the remote server instead of having the
>>> local server participate. Without looking at the patch, I don't have
>>> a strong opinion on whether that's better than "direct update" in this
>>> context.
>>
>>
>> "Update Pushdown" is fine with me.
>>
>> If there are no objections of others, I'll change the name from "Direct
>> Update" to "Update Pushdown".
>
>
> Done. (I've left deparseDirectUpdateSql/deparseDirectDeleteSql as-is,
> though.)
>
> Other changes:
>
> * Address the comments from Eitoku-san.
> * Add regression tests.
> * Fix a bug, which fails to show the actual row counts in EXPLAIN ANALYZE
> for UPDATE/DELETE without a RETURNING clause.
> * Rebase to HEAD.
>
> Please find attached an updated version of the patch.
>
>
> Thanks,
>
> Best regards,
> Etsuro Fujita

--
Shigeru HANADA