(2014/12/12 11:33), Etsuro Fujita wrote:
> (2014/12/12 11:19), Tom Lane wrote:
>> Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> writes:
>>> (2014/12/12 10:37), Tom Lane wrote:
>>>> Yeah, this is clearly a thinko: really, nothing in the planner should
>>>> be using get_parse_rowmark(). I looked around for other errors of the
>>>> same type and found that postgresGetForeignPlan() is also using
>>>> get_parse_rowmark(). While that's harmless at the moment because we
>>>> don't support foreign tables as children, it's still wrong. Will
>>>> fix that too.
>>
>>> I don't think we need to fix that too. In order to support that, I'm
>>> proposing to modify postgresGetForeignPlan() in the following way [1]
>>> (see fdw-inh-5.patch).
>>
>> My goodness, that's ugly. And it's still wrong, because this is planner
>> code so it shouldn't be using get_parse_rowmark at all. The whole point
>> here is that the rowmark info has been transformed into something
>> appropriate for the planner to use. While that transformation is
>> relatively trivial today, it might not always be so.
>
> OK, I'll update the inheritance patch on top of the revison you'll make.
Thanks for your speedy work.
While updating the inheritance patch, I noticed that the fix for
postgresGetForeignPlan() is not right. Since PlanRowMarks for foreign
tables get the ROW_MARK_COPY markType during preprocess_rowmarks(), so
we can't get the locking strength from the PlanRowMarks, IIUC. In order
to get the locking strength, I think we need to see the RowMarkClauses
and thus still need to use get_parse_rowmark() in
postgresGetForeignPlan(), though I agree with you that that is ugly.
Thanks,
Best regards,
Etsuro Fujita
