On Tue, Mar 14, 2017 at 8:04 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: >> In 0001, you've removed a comment about how GEQO needs special >> handling, but it doesn't look as if you've made any compensating >> change elsewhere. That seems unlikely to be correct. If GEQO needs >> some paths to survive longer than others, how can it be right for this >> code to create them all in the same context? > > Thanks for pointing that out. I have replaced the code and the > comments back. There was another issue that the temporary paths > created by geqo will not be freed when geqo moves one genetic string > to next genetic string (or what it calls as tour: an list of relation > to be joined in a given order). To fix this, we need to set the path > context to the temporary context of geqo inside geqo_eval() before > calling gimme_tree() and reset it later. That way the temporary paths > are also created in the temporary memory context of geqo. Fixed in the > patch.
Yeah, that looks better. >> Incidentally, >> geqo_eval() seems to be an existing precedent for the idea of throwing >> away paths and RelOptInfos, so we might want to use similar code for >> partitionwise join. > > There are some differences in what geqo does and what partition-wise > needs to do. geqo tries many joining orders each one in a separate > temporary context. The way geqo slices the work, every slice produces > a full plan. For partition-wise join I do not see a way to slice the > work such that the whole path and corresponding RelOptInfos come from > the same slice. So, we can't use the same method as GEQO. What I was thinking about was the use of this technique for getting rid of joinrels: root->join_rel_list = list_truncate(root->join_rel_list, savelength); root->join_rel_hash = savehash; makePathNode() serves to segregate paths into a separate memory context that can then be destroyed, but as you point out, the path lists are still hanging around, and so are the RelOptInfo nodes. It seems to me we could do a lot better using this technique. Suppose we jigger things so that the List objects created by add_path go into path_cxt, and so that RelOptInfo nodes also go into path_cxt. Then when we blow up path_cxt we won't have dangling pointers in the RelOptInfo objects any more because the RelOptInfos themselves will be gone. The only problem is that the join_rel_list (and join_rel_hash if it exists) will be corrupt, but we can fix that using the technique demonstrated above. Of course, that supposes that 0009 can manage to postpone creating non-sampled child joinrels until create_partition_join_plan(), which it currently doesn't. In fact, unless I'm missing something, 0009 hasn't been even slightly adapted to take advantage of the infrastructure in 0001; it doesn't seem to reset the path_cxt or anything. That seems like a fairly major omission. Incidentally, I committed 0002, 0003, and 0005 as a single commit with a few tweaks; I think you may need to do a bit of rebasing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers