(2014/12/12 11:33), Etsuro Fujita wrote: > (2014/12/12 11:19), Tom Lane wrote: >> Etsuro Fujita <fujita.ets...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
