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.
(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.)
> 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.
> 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, but if we
expose an option we're going to be stuck with supporting it forever.
regards, tom lane