Re: EvalPlanQual behaves oddly for FDW queries involving system columns

On 2015/04/08 7:44, Tom Lane wrote:
> Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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