On Fri, Mar 10, 2017 at 5:43 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> PFA the zip containing all the patches rebased on
> 56018bf26eec1a0b4bf20303c98065a8eb1b0c5d and contain the patch to free
> memory consumed by paths using a separate path context.
Some very high-level thoughts based on a look through these patches:
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? 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.
0002 and 0003 look OK.
Probably 0004 is OK too, although that seems to be adding some
overhead to existing callers for the benefit of new ones. Might be
insignificant, though.
0005 looks OK, except that add_join_rel's definition is missing a
"static" qualifier. That's not just cosmetic; based on previous
expereince, this will break the BF.
0006 seems to be unnecessary; the new function isn't used in later patches.
Haven't looked at 0007 yet.
0008 is, as previously mentioned, more than we probably want to commit.
Haven't looked at 0009 yet.
0010 - 0012 seem to be various fixes which would need to be done
before or along with 0009, rather than afterward, so I am confused
about the ordering of those patches in the patch series.
The commit message for 0013 is a bit unclear about what it's doing,
although I can guess, a bit, based on the commit message for 0007.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
