Re: [HACKERS] create_unique_path and GEQO

[Tom Lane]

Ashutosh Bapat  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


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

Re: [HACKERS] create_unique_path and GEQO

[Ashutosh Bapat]

On Fri, Mar 24, 2017 at 11:52 AM, Rushabh Lathia
 wrote:
>
>
> On Wed, Mar 22, 2017 at 3:43 PM, Ashutosh Bapat
>  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


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

Re: [HACKERS] create_unique_path and GEQO

[Rushabh Lathia]

On Wed, Mar 22, 2017 at 3:43 PM, Ashutosh Bapat <
ashutosh.ba...@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?


> --
> 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
>



-- 
Rushabh Lathia

Re: [HACKERS] create_unique_path and GEQO

[Ashutosh Bapat]

On Wed, Mar 22, 2017 at 3:43 PM, Ashutosh Bapat
 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


pg_upath_geqo.patch
Description: Binary data

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

[HACKERS] create_unique_path and GEQO

[Ashutosh Bapat]

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


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