On Fri, Mar 24, 2017 at 9:50 AM, Tom Lane <tgl@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
