On Thu, Aug 2, 2018 at 7:20 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: > The new approach seems to me more localized than what Ashutosh had proposed. > One obvious advantage of the new approach is that we no longer need changes > to postgres_fdw for it to work for partitionwise joins, which would apply > for any other FDWs, making the FDW authors' life easy.
Well, I don't know what to say here expect that I don't agree. I think Ashutosh's approach is a pretty natural extension of the system that we have now. It involves needing to handle converted whole row vars in some new places, most of which were handled in the original patch, but postgres_fdw was missed. Your approach involves changing the meaning of the target list, but only in narrow corner cases. I completely disagree that we can say it's less invasive. It may indeed be less work for extension authors, though, though perhaps at the expense of moving the conversion from the remote server to the local one. >> But in general, with your approach, any code that >> looks at the tlist for a child-join has to know that the tlist is to >> be used as-is *except that* ConvertRowtypeExpr may need to be >> inserted. I think that special case could be easy to overlook, and it >> seems pretty arbitrary. > > I think we can check to see whether the conversion is needed, from the flags > added to RelOptInfo. Sure, I don't dispute that it can be made to work. I just think it's an ugly kludge. > I have to admit that it's weird to adjust the child's targetlist in that > case when putting the child under the parent's Append/MergeAppend, but IMHO > I think that would be much localized. Yeah, I just don't agree at all. Does anyone else want to weigh in on this? It seems to me that there are several people who are quite willing to complain about the fact that this hasn't been fixed, but not willing to express an opinion about the shape of the fix. Either the RMT needs to take executive action, or we need some more votes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company