Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> On 2015/05/13 6:22, Tom Lane wrote:
>> Of course, if we don't do that then we still have your original gripe
>> about ctid not being correct in EvalPlanQual results. I poked at this
>> a bit and it seems like we could arrange to pass ctid through even in
>> the ROW_MARK_COPY case, if we define the t_ctid field of a composite
>> Datum as being the thing to use.

> I did the same thing when creating the first version of the patch [1].
> In addition, I made another change to ForeignNext so that the FDWs to
> get ctid coming out as the same value (0, 0) instead of (4294967295,0)
> before and after an EvalPlanQual recheck. But IIRC, I think both of
> them were rejected by you ...

Ah, right. AFAIR, people objected to the other things you'd done in
that patch, and I'd still say that the change in nodeForeignscan.c
is unnecessary. But the idea of using t_ctid to solve the problem
for the ROW_MARK_COPY code path seems reasonable enough.

>> We could fix that by
>> adding an ItemPointerSetInvalid(&(td->t_ctid)) call to heap_form_tuple,
>> but I dunno if we want to add even a small number of cycles for this
>> purpose to such a core function.

> I thought so too when creating the first version.

On the other hand, that would only be three additional instructions
on typical machines, which is surely in the noise compared to the rest of
heap_form_tuple, and you could argue that it's a bug fix: leaving the
t_ctid field with an apparently valid value is not very appropriate.
So after thinking about it I think we ought to do that.

regards, tom lane