Re: Optimization for updating foreign tables in Postgres FDW

Fujita-san, I am attaching update version of the patch, which added
the documentation update.

Once we finalize this, I feel good with the patch and think that we
could mark this as ready for committer.

On Fri, Feb 5, 2016 at 5:33 PM, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
wrote:

>
>
> On Fri, Feb 5, 2016 at 4:46 PM, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
> wrote:
>
>>
>>
>> On Wed, Feb 3, 2016 at 3:31 PM, Etsuro Fujita <
>> fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>
>>> On 2016/01/28 15:20, Rushabh Lathia wrote:
>>>
>>>> On Thu, Jan 28, 2016 at 11:33 AM, Etsuro Fujita
>>>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>>
>>>> wrote:
>>>>
>>>> On 2016/01/27 21:23, Rushabh Lathia wrote:
>>>>
>>>> If I understood correctly, above documentation means, that if
>>>> FDW have
>>>> DMLPushdown APIs that is enough. But in reality thats not the
>>>> case, we
>>>> need ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete
>>>> in case
>>>> DML is not pushable.
>>>>
>>>> And here fact is DMLPushdown APIs are optional for FDW, so that
>>>> if FDW
>>>> don't have DMLPushdown APIs they can still very well perform
>>>> the DML
>>>> operations using ExecForeignInsert, ExecForeignUpdate, or
>>>> ExecForeignDelete.
>>>>
>>>
>>> So documentation should be like:
>>>>
>>>> If the IsForeignRelUpdatable pointer is set to NULL, foreign
>>>> tables are
>>>> assumed to be insertable, updatable, or deletable if the FDW
>>>> provides
>>>> ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete
>>>> respectively,
>>>>
>>>> If FDW provides DMLPushdown APIs and the DML are pushable to the
>>>> foreign
>>>> server, then FDW still needs ExecForeignInsert,
>>>> ExecForeignUpdate, or
>>>> ExecForeignDelete for the non-pushable DML operation.
>>>>
>>>> What's your opinion ?
>>>>
>>>
>>> I agree that we should add this to the documentation, too.
>>>>
>>>
>>> I added docs to the IsForeignRelUpdatable documentation. Also, a brief
>>> introductory remark has been added at the beginning of the DML pushdown
>>> APIs' documentation.
>>>
>>> BTW, if I understand correctly, I think we should also modify
>>>> relation_is_updatabale() accordingly. Am I right?
>>>>
>>>
>>> Yep, we need to modify relation_is_updatable().
>>>>
>>>
>>> I thought I'd modify that function in the same way as
>>> CheckValidResultRel(), but I noticed that we cannot do that, because we
>>> don't have any information on whether each update is pushed down to the
>>> remote server by PlanDMLPushdown, during relation_is_updatabale().
>>
>>
>> Sorry I didn't get you here. Can't resultRelInfo->ri_FdwPushdown gives
>> information update whether update is pushed down safe or not ? What my
>> concern here is, lets say resultRelInfo->ri_FdwPushdown marked as true
>> (PlanDMLPushdown return true), but later into CheckValidResultRel() it
>> found out that missing BeginDMLPushdown, IterateDMLPushdown and
>> EndDMLPushdown APIs and it will end up with error.
>>
>> What I think CheckValidResultRel() should do is, if
>> resultRelInfo->ri_FdwPushdown is true then check for the DMLPushdown API
>> and if it doesn't find those API then check for traditional APIs
>> (ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete). And when it
>> doesn't find both then it should return an error.
>>
>> I changed CheckValidResultRel(), where
>>
>> 1) Don't throw an error if resultRelInfo->ri_FdwPushdown is true and
>> DMLPushdown APIs are missing as query can still perform operation with
>> traditional ExecForeign APIs. So in this situation just marked
>> resultRelInfo->ri_FdwPushdown to false.
>>
>> (Wondering can we add the checks for DMLPushdown APIs into
>> PlanDMLPushdown as additional check? Means PlanDMLPushdown should return
>> true only if FDW provides the BeginDMLPushdown & IterateDMLPushdown &
>> EndDMLPushdown APIs ? What you say ?)
>>
>>
> On another thought, we should not give responsibility to check for the
> APIs to the FDW. So may be we should call PlanDMLPushdown only if
> BeginDMLPushdown & IterateDMLPushdown & EndDMLPushdown APIs are present
> into FDW. That means prepare DMLPushdown plan only when all the required
> APIs are available with FDW. This will also reduce the changes into
> CheckValidResultRel().
>
> Thanks Ashutosh Bapat for healthy discussion.
>
> PFA patch.
>
>
>
>> 2) Don't throw an error if resultRelInfo->ri_FdwPushdown is true and
>> DMLPushdown APIs is present but ExecForeign APIs are missing.
>> 3) Throw an error if resultRelInfo->ri_FdwPushdown is false and
>> ExecForeign APIs are missing.
>>
>> Attaching is the WIP patch here, do share your thought.
>> (need to apply on top of V6 patch)
>>
>>
>> So, I left that function as-is. relation_is_updatabale() is just used
>>> for display in the information_schema views, so ISTM that that function is
>>> fine as-is. (As for CheckValidResultRel(), I revised it so as to check the
>>> presence of DML pushdown APIs after checking the existing APIs if the given
>>> command will be pushed down. The reason is because we assume the presence
>>> of the existing APIs, anyway.)
>>>
>>>
>> I revised other docs and some comments, mostly for consistency.
>>>
>>> Attached is an updated version of the patch, which has been created on
>>> top of the updated version of the bugfix patch posted by Robert in [1]
>>> (attached).
>>>
>>>
>>> Best regards,
>>> Etsuro Fujita
>>>
>>> [1]
>>> http://www.postgresql.org/message-id/CA+TgmoZ40j2uC5aC1NXu03oj4CrVOLkS15XX+PTFP-1U-8zR1Q@mail.gmail.com
>>>
>>
>>
>>
>> --
>> Rushabh Lathia
>>
>
>
>
> --
> Rushabh Lathia
>

--
Rushabh Lathia