Re: EvalPlanQual behaves oddly for FDW queries involving system columns

On 2015/04/08 7:44, Tom Lane wrote:
> Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>> To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like
>> to propose the following FDW APIs:
> 
>> RowMarkType
>> GetForeignRowMarkType(Oid relid,
>>                         LockClauseStrength strength);
> 
>> Decide which rowmark type to use for a foreign table (that has strength
>> = LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY.  (For now, the
>> second argument takes LCS_NONE only, but is intended to be used for the
>> possible extension to the other cases.)  This is called during
>> select_rowmark_type() in the planner.
> 
> Why would you restrict that to LCS_NONE?  Seems like the point is to give
> the FDW control of the rowmark behavior, not only partial control.

The reason is because I think it's comparatively more promissing to
customize the ROW_MARK type for LCS_NONE than other cases since in many
workloads no re-fetch is needed, and because I think other cases would
need more discussions.  So, as a first step, I restricted that to
LCS_NONE.  But I've got the point, and agree that we should give the
full control to the FDW.

> (For the same reason I disagree with the error check in the patch that
> restricts which ROW_MARK options this function can return.  If the FDW has
> TIDs, seems like it could reasonably use whatever options tables can use.)

Will fix.

>> void
>> BeginForeignFetch(EState *estate,
>>                     ExecRowMark *erm,
>>                     List *fdw_private,
>>                     int eflags);
> 
>> Begin a remote fetch. This is called during InitPlan() in the executor.
> 
> The begin/end functions seem like useless extra mechanism.  Why wouldn't
> the FDW just handle this during its regular scan setup?  It could look to
> see whether the foreign table is referenced by any ExecRowMarks (possibly
> there's room to add some convenience functions to help with that).  What's
> more, that design would make it simpler if the basic row fetch needs to be
> modified.

The reason is just because it's easy to understand the structure at
least to me since the begin/exec/end are all done in a higher level of
the executor.  What's more, the begin/end can be done once per foreign
scan node for the multi-table update case.  But I feel inclined to agree
with you on this point also.

>> And I'd also like to propose to add a table/server option,
>> row_mark_reference, to postgres_fdw.  When a user sets the option to
>> true for eg a foreign table, ROW_MARK_REFERENCE will be used for the
>> table, not ROW_MARK_COPY.
> 
> Why would we leave that in the hands of the user?  Hardly anybody is going
> to know what to do with the option, or even that they should do something
> with it.  It's basically only of value for debugging AFAICS,

Agreed.  (When begining to update postgres_fdw docs, I also started to
think so.)

I'll update the patch, which will contain only an infrastructure for
this in the PG core, and will not contain any postgres_fdw change.

Thank you for taking the time to review the patch!

Best regards,
Etsuro Fujita