Re: EvalPlanQual behaves oddly for FDW queries involving system columns

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;
> default:
> elog(ERROR, "unrecognized markType: %d", rc->markType);
> + relid = InvalidOid;
> relation = NULL; /* keep compiler quiet */
> 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;
>
> /* copy and store tuple */

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? 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.

I didn't look at anything else in the patch so I can't comment more on
it, but the change to ExecRowMark caught my attention.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services