On Fri, Mar 24, 2017 at 9:50 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Yeah. I think the code in mark_dummy_rel is newer and better-thought-out > than what's in create_unique_path. It probably makes sense to change over.
I did a bit of archaeology here. create_unique_path() first appears in commit bdfbfde1b168b3332c4cdac34ac86a80aaf4d442 (vintage 2003), where it used GetMemoryChunkContext(rel). Commit f41803bb39bc2949db200116a609fd242d0ec221 (vintage 2007) changed it to use root->planner_cxt, but neither the comment changes in that patch nor the commit message give any hint as to the motivation for the change. The comments do mention that it should be kept in sync with best_inner_indexscan(), which was also switched to use root->planner_cxt by that commit, again without any real explanation as to why, and best_inner_indexscan() continued to use root->planner_cxt until its demise in commit e2fa76d80ba571d4de8992de6386536867250474 (vintage 2012). Meanwhile, mark_dummy_rel() didn't switch contexts at all until commit eca75a12a27d28b972fc269c1c8813cd8eb15441 (vintage 2011) at which point it began using GetMemoryChunkContext(rel). So, the coding that Ashutosh is proposing here is just changing things back to the way they were before 2007, but with the better comment that Tom wrote in 2011 for mark_dummy_rel(). Since the 2007 change lacks any justification and the new code has one, I'm inclined to agree with Tom that changing this make sense. If it turns out to be wrong, then mark_dummy_rel() also needs fixing, and the comments need to explain things better. My guess, though, is that it's right, because the rationale offered in mark_dummy_rel() seems to make a lot of sense. One reason why we might want to care about this, other than a laudable consistency, is that at one point Ashutosh had some patches to reduce the memory usage of partition-wise join that involved doing more incremental discarding of RelOptInfos akin to what GEQO does today. If we ever do anything like that, it will be good if we're consistent (and correct) about which contexts end up containing which data structures. All of which, I think, is a long-winded way of saying that I'm going to go commit this patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
