Re: EvalPlanQual behaves oddly for FDW queries involving system columns - Mailing list pgsql-hackers

Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
> On 2015/03/12 13:35, Tom Lane wrote:
>> I don't like the execMain.c changes much at all.  They look somewhat
>> like they're intended to allow foreign tables to adopt a different
>> locking strategy,

> I didn't intend such a thing.  My intention is, foreign tables have
> markType = ROW_MARK_COPY as ever, but I might not have correctly
> understood what you pointed out.  Could you elaborate on that?

I think the real fix as far as postgres_fdw is concerned is in fact
to let it adopt a different ROW_MARK strategy, since it has meaningful
ctid values.  However, that is not a one-size-fits-all answer.  The
fundamental problem I've got with this patch is that it's trying to
impose some one-size-fits-all assumptions on all FDWs about whether
ctids are meaningful; which is wrong, not to mention not backwards
compatible.

>> I don't see the need for the change in nodeForeignscan.c.  If the FDW has
>> returned a physical tuple, it can fix that for itself, while if it has
>> returned a virtual tuple, the ctid will be garbage in any case.

> If you leave nodeForeignscan.c unchanged, file_fdw would cause the
> problem that I pointed out upthread.  Here is an example.

But that's self-inflicted damage, because in fact ctid correctly shows
as invalid in this case in HEAD, without this patch.

The tableoid problem can be fixed much less invasively as per the attached
patch.  I think that we should continue to assume that ctid is not
meaningful (and hence should read as (4294967295,0)) in FDWs that use
ROW_MARK_COPY, and press forward on fixing the locking issues for
postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to
that.  That would also cause ctid to read properly for rows from
postgres_fdw.

            regards, tom lane

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 33b172b..5a1c3b3 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** EvalPlanQualFetchRowMarks(EPQState *epqs
*** 2438,2444 ****
              /* build a temporary HeapTuple control structure */
              tuple.t_len = HeapTupleHeaderGetDatumLength(td);
              ItemPointerSetInvalid(&(tuple.t_self));
!             tuple.t_tableOid = InvalidOid;
              tuple.t_data = td;

              /* copy and store tuple */
--- 2438,2445 ----
              /* build a temporary HeapTuple control structure */
              tuple.t_len = HeapTupleHeaderGetDatumLength(td);
              ItemPointerSetInvalid(&(tuple.t_self));
!             tuple.t_tableOid = getrelid(erm->rti,
!                                         epqstate->estate->es_range_table);
              tuple.t_data = td;

              /* copy and store tuple */