Re: EvalPlanQual behaves oddly for FDW queries involving system columns

On 2015/01/16 1:24, Alvaro Herrera wrote:
> Etsuro Fujita wrote:
>> *** 817,826 **** InitPlan(QueryDesc *queryDesc, int eflags)
>> --- 818,833 ----
>>                    break;
>>                case ROW_MARK_COPY:
>>                    /* there's no real table here ... */
>> +                 relkind = rt_fetch(rc->rti, rangeTable)->relkind;
>> +                 if (relkind == RELKIND_FOREIGN_TABLE)
>> +                     relid = getrelid(rc->rti, rangeTable);
>> +                 else
>> +                     relid = InvalidOid;
>>                    relation = NULL;
>>                    break;

>> --- 2326,2342 ----
>>
>>                /* build a temporary HeapTuple control structure */
>>                tuple.t_len = HeapTupleHeaderGetDatumLength(td);
>> !             /* if relid is valid, rel is a foreign table; set system columns */
>> !             if (OidIsValid(erm->relid))
>> !             {
>> !                 tuple.t_self = td->t_ctid;
>> !                 tuple.t_tableOid = erm->relid;
>> !             }
>> !             else
>> !             {
>> !                 ItemPointerSetInvalid(&(tuple.t_self));
>> !                 tuple.t_tableOid = InvalidOid;
>> !             }
>>                tuple.t_data = td;

> I find this arrangement confusing and unnecessary -- surely if you have
> access to the ExecRowMark here, you could just obtain the relid with
> RelationGetRelid instead of saving the OID beforehand?

I don't think so because we don't have the Relation (ie, erm->relation =
NULL) here since InitPlan don't open/lock relations having markType =
ROW_MARK_COPY as shown above, which include foreign tables selected for
update/share.  But I noticed we should open/lock such foreign tables as
well in InitPlan because I think ExecOpenScanRelation is assuming that,
IIUC.

 > And if you have
> the Relation, you could just consult the relkind at that point instead
> of relying on the relid being set or not as a flag to indicate whether
> the rel is foreign.

Followed your revision.  Attached is an updated version of the patch.

Thanks for the comment!

Best regards,
Etsuro Fujita