Re: Optimization for updating foreign tables in Postgres FDW

(2014/09/12 16:03), Albe Laurenz wrote:
> Tom Lane wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>> I have to admit that, while I applaud the effort made to have this
>>> change only be to postgres_fdw, I'm not sure that having the
>>> update/delete happening during the Scan phase and then essentially
>>> no-op'ing the ExecForeignUpdate/ExecForeignDelete is entirely in-line
>>> with the defined API.
>>
>> Yeah, I've started looking at this patch and that seemed like not
>> necessarily such a wise choice.  I think it'd be better if the generated
>> plan tree had a different structure in this case.  The existing approach
>> is an impressive hack but it's hard to call it anything but a hack.

Thank you for the review, Tom and Stephen!

> I guess that the idea is inspired by this comment on postgres_fdw.c:
>
>   * Note: currently, the plan tree generated for UPDATE/DELETE will always
>   * include a ForeignScan that retrieves ctids (using SELECT FOR UPDATE)
>   * and then the ModifyTable node will have to execute individual remote
>   * UPDATE/DELETE commands.  If there are no local conditions or joins
>   * needed, it'd be better to let the scan node do UPDATE/DELETE RETURNING
>   * and then do nothing at ModifyTable.  Room for future optimization ...

That's right.  Thanks, Laurenz!

And in addition to that, I've created the patch with the conscious aim 
of not getting the core code involved, because I was thinking this is 
just an optimization.  But I have to admit I was conscious of that too much.

>> I'm not sure offhand what the new plan tree ought to look like.  We could
>> just generate a ForeignScan node, but that seems like rather a misnomer.
>> Is it worth inventing a new ForeignUpdate node type?  Or maybe it'd still
>> be ForeignScan but with a flag field saying "hey this is really an update
>> (or a delete)".  The main benefit I can think of right now is that the
>> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly
>> the only thing that ever looks at plan trees, so having an outright
>> misleading plan structure is likely to burn us down the line.
>
> I can understand these qualms.
> I wonder if "ForeignUpdate" is such a good name though, since it would
> surprise the uninitiate that in the regular (no push-down) case the
> actual modification is *not* performed by ForeignUpdate.
> So it should rather be a "ForeignModifyingScan", but I personally would
> prefer a "has_side_effects" flag on ForeignScan.

+1 for improving the EXPLAIN output by inventing a plan tree with a 
different structure in this case, in general.

>> One advantage of getting the core code involved in the decision about
>> whether an update/delete can be pushed down is that FDW-independent checks
>> like whether there are relevant triggers could be implemented in the core
>> code, rather than having to duplicate them (and later maintain them) in
>> every FDW that wants to do this.

Good idea!

>> Stuff like "is there a
>> LIMIT" probably has to be in the FDW since some FDWs could support pushing
>> down LIMIT and others not.

That's what I have in mind.  I'll work on that at the next CF.

Thanks,

Best regards,
Etsuro Fujita