Thread: [HACKERS] create_unique_path and GEQO

Hi,
In create_unique_path() there's comment   /*    * We must ensure path struct and subsidiary data are allocated in main
 * planning context; otherwise GEQO memory management causes trouble.    */   oldcontext =
MemoryContextSwitchTo(root->planner_cxt);
   pathnode = makeNode(UniquePath);

This means that when GEQO resets the memory context, the RelOptInfo
for which this path is created and may be set to cheapest_unique_path
goes away, the unique path lingers on in the planner context.
Shouldn't we instead allocate the path in the same context as the
RelOptInfo similar to mark_dummy_rel()?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Wed, Mar 22, 2017 at 3:43 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Hi,
> In create_unique_path() there's comment
>     /*
>      * We must ensure path struct and subsidiary data are allocated in main
>      * planning context; otherwise GEQO memory management causes trouble.
>      */
>     oldcontext = MemoryContextSwitchTo(root->planner_cxt);
>
>     pathnode = makeNode(UniquePath);
>
> This means that when GEQO resets the memory context, the RelOptInfo
> for which this path is created and may be set to cheapest_unique_path
> goes away, the unique path lingers on in the planner context.
> Shouldn't we instead allocate the path in the same context as the
> RelOptInfo similar to mark_dummy_rel()?
>

tried this change as attached patch. I ran make installcheck with
geqo_threshold = 2. Only join.sql failed several plans changed, which
is expected. There was one difference related to changed output order
but that's because of the changed plan.

Adding this to the commitfest.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Mar 24, 2017 at 11:52 AM, Rushabh Lathia
<rushabh.lathia@gmail.com> wrote:
>
>
> On Wed, Mar 22, 2017 at 3:43 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> Hi,
>> In create_unique_path() there's comment
>>     /*
>>      * We must ensure path struct and subsidiary data are allocated in
>> main
>>      * planning context; otherwise GEQO memory management causes trouble.
>>      */
>>     oldcontext = MemoryContextSwitchTo(root->planner_cxt);
>>
>>     pathnode = makeNode(UniquePath);
>>
>> This means that when GEQO resets the memory context, the RelOptInfo
>> for which this path is created and may be set to cheapest_unique_path
>> goes away, the unique path lingers on in the planner context.
>> Shouldn't we instead allocate the path in the same context as the
>> RelOptInfo similar to mark_dummy_rel()?
>>
>
> Do you have test case, which can reproduce the issue you
> explained above?
>

No. It would require some surgery in standard_planner() to measure the
memory consumed in the planner context OR build the code with
SHOW_MEMORY_STATS defined and dump memory context statistics and check
planner context memory usage. I don't think I can produce a testcase
quickly right now. But then, I think the problem is quite apparent
from the code inspection alone, esp. comparing what mark_dummy_rel()
does with what create_unique_path() is doing.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> Do you have test case, which can reproduce the issue you
>> explained above?

> No. It would require some surgery in standard_planner() to measure the
> memory consumed in the planner context OR build the code with
> SHOW_MEMORY_STATS defined and dump memory context statistics and check
> planner context memory usage. I don't think I can produce a testcase
> quickly right now. But then, I think the problem is quite apparent
> from the code inspection alone, esp. comparing what mark_dummy_rel()
> does with what create_unique_path() is doing.

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.
        regards, tom lane

On Fri, Mar 24, 2017 at 10:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>>> Do you have test case, which can reproduce the issue you
>>> explained above?
>
>> No. It would require some surgery in standard_planner() to measure the
>> memory consumed in the planner context OR build the code with
>> SHOW_MEMORY_STATS defined and dump memory context statistics and check
>> planner context memory usage. I don't think I can produce a testcase
>> quickly right now. But then, I think the problem is quite apparent
>> from the code inspection alone, esp. comparing what mark_dummy_rel()
>> does with what create_unique_path() is doing.
>
> 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.

This has remained unanswered for more than 8 months, so I am marking
this patch as returned with feedback.
-- 
Michael

On Thu, Nov 30, 2017 at 8:50 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Mar 24, 2017 at 10:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>>>> Do you have test case, which can reproduce the issue you
>>>> explained above?
>>
>>> No. It would require some surgery in standard_planner() to measure the
>>> memory consumed in the planner context OR build the code with
>>> SHOW_MEMORY_STATS defined and dump memory context statistics and check
>>> planner context memory usage. I don't think I can produce a testcase
>>> quickly right now. But then, I think the problem is quite apparent
>>> from the code inspection alone, esp. comparing what mark_dummy_rel()
>>> does with what create_unique_path() is doing.
>>
>> 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.
>
> This has remained unanswered for more than 8 months, so I am marking
> this patch as returned with feedback.

I am not sure what's there to answer in Tom's reply. He seems to be
agreeing with my analysis. Correct me if I am wrong. If that's the
case, I am expecting somebody to review the patch. If there are no
review comments, some committer may commit it.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Thu, Nov 30, 2017 at 9:00 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Thu, Nov 30, 2017 at 8:50 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Mar 24, 2017 at 10:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>>>>> Do you have test case, which can reproduce the issue you
>>>>> explained above?
>>>
>>>> No. It would require some surgery in standard_planner() to measure the
>>>> memory consumed in the planner context OR build the code with
>>>> SHOW_MEMORY_STATS defined and dump memory context statistics and check
>>>> planner context memory usage. I don't think I can produce a testcase
>>>> quickly right now. But then, I think the problem is quite apparent
>>>> from the code inspection alone, esp. comparing what mark_dummy_rel()
>>>> does with what create_unique_path() is doing.
>>>
>>> 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.
>>
>> This has remained unanswered for more than 8 months, so I am marking
>> this patch as returned with feedback.
>
> I am not sure what's there to answer in Tom's reply. He seems to be
> agreeing with my analysis. Correct me if I am wrong. If that's the
> case, I am expecting somebody to review the patch. If there are no
> review comments, some committer may commit it.
>

For now I have marked it as need reviewer and moved to the next commitfest.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

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