Re: Optimization for updating foreign tables in Postgres FDW

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.

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
(usingSELECT 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
thendo nothing at ModifyTable.  Room for future optimization ...
 

> 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.

> 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.  OTOH, maybe the trigger issue is really
> the only thing that could be shared, not sure.  Stuff like "is there a
> LIMIT" probably has to be in the FDW since some FDWs could support pushing
> down LIMIT and others not.

You are right, the gain would probably be limited.

Yours,
Laurenz Albe