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