Menu
▾
▴
postgres-xc-developers
|
From: Nikhil S. <ni...@st...> - 2013-08-10 17:19:09
|
Hi, Have a Query about ExecRemoteQuery. The logic seems to have been modeled after ExecMaterial. ISTM, that it should have been modeled after ExecScan because we fetch tuples, and those which match the qual should be sent up. ExecMaterial is for materializing and collecting and storing tuples. Can anyone explain? The reason for asking this is I am suspecting a big memory leak in this code path. We are not using any expression context nor we are freeing up tuples as we scan for the one which qualifies. Regards, Nikhils -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Ashutosh B. <ash...@en...> - 2013-08-12 04:24:32
|
Welcome to the mess ;) and enjoy junk food. Sometime back, I have changed ExecRemoteQuery to be called in the same fashion as other Scan nodes. So, you will see ExecRemoteQuery calling ExecScan with RemoteQueryNext as the iterator. So, I assume your comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! This code needs a lot of rework, removing duplications, using proper way of materialisation, central response handler and error handler etc. If we clean up this code, some improvements in planner (like using MergeAppend plan) for Sort, will be possible. Regarding materialisation, the code uses a linked list for materialising the rows from datanodes (in case the same connection needs to be given to other remote query node), which must be eating a lot of performance. Instead we should be using some kind of tuplestore there. We actually use tuplestore (as well) in the RemoteQuery node; the same method can be used. On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st...>wrote: > Hi, > > Have a Query about ExecRemoteQuery. > > The logic seems to have been modeled after ExecMaterial. ISTM, that it > should have been modeled after ExecScan because we fetch tuples, and those > which match the qual should be sent up. ExecMaterial is for materializing > and collecting and storing tuples. > > Can anyone explain? The reason for asking this is I am suspecting a big > memory leak in this code path. We are not using any expression context nor > we are freeing up tuples as we scan for the one which qualifies. > > Regards, > Nikhils > -- > StormDB - http://www.stormdb.com > The Database Cloud > > ------------------------------------------------------------------------------ > Get 100% visibility into Java/.NET code with AppDynamics Lite! > It's a free troubleshooting tool designed for production. > Get down to code-level detail for bottlenecks, with <2% overhead. > Download for free and get started troubleshooting in minutes. > http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk > _______________________________________________ > Postgres-xc-developers mailing list > Pos...@li... > https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers > > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Pavan D. <pav...@gm...> - 2013-08-12 09:17:23
|
On Mon, Aug 12, 2013 at 9:54 AM, Ashutosh Bapat < ash...@en...> wrote: > > Welcome to the mess ;) and enjoy junk food. > > Sometime back, I have changed ExecRemoteQuery to be called in the same > fashion as other Scan nodes. So, you will see ExecRemoteQuery calling > ExecScan with RemoteQueryNext as the iterator. So, I assume your comment > pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! > > This code needs a lot of rework, removing duplications, using proper way > of materialisation, central response handler and error handler etc. If we > clean up this code, some improvements in planner (like using MergeAppend > plan) for Sort, will be possible. Regarding materialisation, the code uses > a linked list for materialising the rows from datanodes (in case the same > connection needs to be given to other remote query node), which must be > eating a lot of performance. Instead we should be using some kind of > tuplestore there. We actually use tuplestore (as well) in the RemoteQuery > node; the same method can be used. > > If we at all want to rework that, IMHO we should just use ForeignScan with appropriate enhancements to the node and/or addition of a new node to effectively combine results from multiple ForeignScan nodes. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee |
|
From: Michael P. <mic...@gm...> - 2013-08-12 23:51:32
|
On Mon, Aug 12, 2013 at 6:16 PM, Pavan Deolasee <pav...@gm...> wrote: > If we at all want to rework that, IMHO we should just use ForeignScan with > appropriate enhancements to the node and/or addition of a new node to > effectively combine results from multiple ForeignScan nodes. Completely agreed with that for SELECT queries with FQS planner and clause pushdowns plugged on top of it. This would make XC more closely implemented with vanilla PG. -- Michael |
|
From: Nikhil S. <ni...@st...> - 2013-08-13 01:47:51
|
Hi Ashutosh, I guess you have changed it in pgxc head? I was looking at 103 and 11 branches and saw this. In that even ExecRemoteQuery seems to have an issue wherein it's not using the appropriate context. Regards, Nikhils Sent from my iPhone On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat <ash...@en...> wrote: Welcome to the mess ;) and enjoy junk food. Sometime back, I have changed ExecRemoteQuery to be called in the same fashion as other Scan nodes. So, you will see ExecRemoteQuery calling ExecScan with RemoteQueryNext as the iterator. So, I assume your comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! This code needs a lot of rework, removing duplications, using proper way of materialisation, central response handler and error handler etc. If we clean up this code, some improvements in planner (like using MergeAppend plan) for Sort, will be possible. Regarding materialisation, the code uses a linked list for materialising the rows from datanodes (in case the same connection needs to be given to other remote query node), which must be eating a lot of performance. Instead we should be using some kind of tuplestore there. We actually use tuplestore (as well) in the RemoteQuery node; the same method can be used. On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st...>wrote: > Hi, > > Have a Query about ExecRemoteQuery. > > The logic seems to have been modeled after ExecMaterial. ISTM, that it > should have been modeled after ExecScan because we fetch tuples, and those > which match the qual should be sent up. ExecMaterial is for materializing > and collecting and storing tuples. > > Can anyone explain? The reason for asking this is I am suspecting a big > memory leak in this code path. We are not using any expression context nor > we are freeing up tuples as we scan for the one which qualifies. > > Regards, > Nikhils > -- > StormDB - http://www.stormdb.com > The Database Cloud > > ------------------------------------------------------------------------------ > Get 100% visibility into Java/.NET code with AppDynamics Lite! > It's a free troubleshooting tool designed for production. > Get down to code-level detail for bottlenecks, with <2% overhead. > Download for free and get started troubleshooting in minutes. > http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk > _______________________________________________ > Postgres-xc-developers mailing list > Pos...@li... > https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers > > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Ashutosh B. <ash...@en...> - 2013-08-13 04:22:41
|
It should be part of 1.1 as well. It was done to support projection out of RemoteQuery node. On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <ni...@st...>wrote: > Hi Ashutosh, > > I guess you have changed it in pgxc head? I was looking at 103 and 11 > branches and saw this. In that even ExecRemoteQuery seems to have an issue > wherein it's not using the appropriate context. > > Regards, > Nikhils > > Sent from my iPhone > > On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < > ash...@en...> wrote: > > > Welcome to the mess ;) and enjoy junk food. > > Sometime back, I have changed ExecRemoteQuery to be called in the same > fashion as other Scan nodes. So, you will see ExecRemoteQuery calling > ExecScan with RemoteQueryNext as the iterator. So, I assume your comment > pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! > > This code needs a lot of rework, removing duplications, using proper way > of materialisation, central response handler and error handler etc. If we > clean up this code, some improvements in planner (like using MergeAppend > plan) for Sort, will be possible. Regarding materialisation, the code uses > a linked list for materialising the rows from datanodes (in case the same > connection needs to be given to other remote query node), which must be > eating a lot of performance. Instead we should be using some kind of > tuplestore there. We actually use tuplestore (as well) in the RemoteQuery > node; the same method can be used. > > > On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st...>wrote: > >> Hi, >> >> Have a Query about ExecRemoteQuery. >> >> The logic seems to have been modeled after ExecMaterial. ISTM, that it >> should have been modeled after ExecScan because we fetch tuples, and those >> which match the qual should be sent up. ExecMaterial is for materializing >> and collecting and storing tuples. >> >> Can anyone explain? The reason for asking this is I am suspecting a big >> memory leak in this code path. We are not using any expression context nor >> we are freeing up tuples as we scan for the one which qualifies. >> >> Regards, >> Nikhils >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> >> ------------------------------------------------------------------------------ >> Get 100% visibility into Java/.NET code with AppDynamics Lite! >> It's a free troubleshooting tool designed for production. >> Get down to code-level detail for bottlenecks, with <2% overhead. >> Download for free and get started troubleshooting in minutes. >> >> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >> _______________________________________________ >> Postgres-xc-developers mailing list >> Pos...@li... >> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >> >> > > > -- > Best Wishes, > Ashutosh Bapat > EntepriseDB Corporation > The Postgres Database Company > > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Nikhil S. <ni...@st...> - 2013-08-14 05:52:15
|
Ah, I see. I was looking at REL_1_0 sources. There are people out there using REL_1_0 as well. Regards, Nikhils On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < ash...@en...> wrote: > It should be part of 1.1 as well. It was done to support projection out of > RemoteQuery node. > > > On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <ni...@st...>wrote: > >> Hi Ashutosh, >> >> I guess you have changed it in pgxc head? I was looking at 103 and 11 >> branches and saw this. In that even ExecRemoteQuery seems to have an issue >> wherein it's not using the appropriate context. >> >> Regards, >> Nikhils >> >> Sent from my iPhone >> >> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >> ash...@en...> wrote: >> >> >> Welcome to the mess ;) and enjoy junk food. >> >> Sometime back, I have changed ExecRemoteQuery to be called in the same >> fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >> >> This code needs a lot of rework, removing duplications, using proper way >> of materialisation, central response handler and error handler etc. If we >> clean up this code, some improvements in planner (like using MergeAppend >> plan) for Sort, will be possible. Regarding materialisation, the code uses >> a linked list for materialising the rows from datanodes (in case the same >> connection needs to be given to other remote query node), which must be >> eating a lot of performance. Instead we should be using some kind of >> tuplestore there. We actually use tuplestore (as well) in the RemoteQuery >> node; the same method can be used. >> >> >> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st...>wrote: >> >>> Hi, >>> >>> Have a Query about ExecRemoteQuery. >>> >>> The logic seems to have been modeled after ExecMaterial. ISTM, that it >>> should have been modeled after ExecScan because we fetch tuples, and those >>> which match the qual should be sent up. ExecMaterial is for materializing >>> and collecting and storing tuples. >>> >>> Can anyone explain? The reason for asking this is I am suspecting a big >>> memory leak in this code path. We are not using any expression context nor >>> we are freeing up tuples as we scan for the one which qualifies. >>> >>> Regards, >>> Nikhils >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >>> ------------------------------------------------------------------------------ >>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>> It's a free troubleshooting tool designed for production. >>> Get down to code-level detail for bottlenecks, with <2% overhead. >>> Download for free and get started troubleshooting in minutes. >>> >>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>> _______________________________________________ >>> Postgres-xc-developers mailing list >>> Pos...@li... >>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>> >>> >> >> >> -- >> Best Wishes, >> Ashutosh Bapat >> EntepriseDB Corporation >> The Postgres Database Company >> >> > > > -- > Best Wishes, > Ashutosh Bapat > EntepriseDB Corporation > The Postgres Database Company > -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Ashutosh B. <ash...@en...> - 2013-08-14 06:03:07
|
There has been an overhaul in the planner (and corresponding parts of executor) in 1.1, so it would be better if they move to 1.1 after GA. On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke <ni...@st...>wrote: > Ah, I see. > > I was looking at REL_1_0 sources. > > There are people out there using REL_1_0 as well. > > Regards, > Nikhils > > > On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < > ash...@en...> wrote: > >> It should be part of 1.1 as well. It was done to support projection out >> of RemoteQuery node. >> >> >> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <ni...@st...>wrote: >> >>> Hi Ashutosh, >>> >>> I guess you have changed it in pgxc head? I was looking at 103 and 11 >>> branches and saw this. In that even ExecRemoteQuery seems to have an issue >>> wherein it's not using the appropriate context. >>> >>> Regards, >>> Nikhils >>> >>> Sent from my iPhone >>> >>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>> ash...@en...> wrote: >>> >>> >>> Welcome to the mess ;) and enjoy junk food. >>> >>> Sometime back, I have changed ExecRemoteQuery to be called in the same >>> fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >>> >>> This code needs a lot of rework, removing duplications, using proper way >>> of materialisation, central response handler and error handler etc. If we >>> clean up this code, some improvements in planner (like using MergeAppend >>> plan) for Sort, will be possible. Regarding materialisation, the code uses >>> a linked list for materialising the rows from datanodes (in case the same >>> connection needs to be given to other remote query node), which must be >>> eating a lot of performance. Instead we should be using some kind of >>> tuplestore there. We actually use tuplestore (as well) in the RemoteQuery >>> node; the same method can be used. >>> >>> >>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> Hi, >>>> >>>> Have a Query about ExecRemoteQuery. >>>> >>>> The logic seems to have been modeled after ExecMaterial. ISTM, that it >>>> should have been modeled after ExecScan because we fetch tuples, and those >>>> which match the qual should be sent up. ExecMaterial is for materializing >>>> and collecting and storing tuples. >>>> >>>> Can anyone explain? The reason for asking this is I am suspecting a big >>>> memory leak in this code path. We are not using any expression context nor >>>> we are freeing up tuples as we scan for the one which qualifies. >>>> >>>> Regards, >>>> Nikhils >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>>> ------------------------------------------------------------------------------ >>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>> It's a free troubleshooting tool designed for production. >>>> Get down to code-level detail for bottlenecks, with <2% overhead. >>>> Download for free and get started troubleshooting in minutes. >>>> >>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>> _______________________________________________ >>>> Postgres-xc-developers mailing list >>>> Pos...@li... >>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>> >>>> >>> >>> >>> -- >>> Best Wishes, >>> Ashutosh Bapat >>> EntepriseDB Corporation >>> The Postgres Database Company >>> >>> >> >> >> -- >> Best Wishes, >> Ashutosh Bapat >> EntepriseDB Corporation >> The Postgres Database Company >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Nikhil S. <ni...@st...> - 2013-08-14 09:34:17
|
Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it.
Here's the snippet from RemoteQueryNext:
if (tuplestorestate && !TupIsNull(scanslot))
tuplestore_puttupleslot(tuplestorestate, scanslot);
I printed the current memory context inside this function, it is
""ExecutorState". This means that the tuple will stay around till the query
is executing in its entirety! For large COPY queries this is bound to cause
issues as is also reported by Hitoshi san on another thread.
I propose that in RemoteQueryNext, before calling the
tuplestore_puttupleslot we switch into
scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It will get
reset, when the next tuple has to be returned to the caller and the leak
will be curtailed. Thoughts?
Regards,
Nikhils
On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat <
ash...@en...> wrote:
> There has been an overhaul in the planner (and corresponding parts of
> executor) in 1.1, so it would be better if they move to 1.1 after GA.
>
>
> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke <ni...@st...>wrote:
>
>> Ah, I see.
>>
>> I was looking at REL_1_0 sources.
>>
>> There are people out there using REL_1_0 as well.
>>
>> Regards,
>> Nikhils
>>
>>
>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat <
>> ash...@en...> wrote:
>>
>>> It should be part of 1.1 as well. It was done to support projection out
>>> of RemoteQuery node.
>>>
>>>
>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <ni...@st...>wrote:
>>>
>>>> Hi Ashutosh,
>>>>
>>>> I guess you have changed it in pgxc head? I was looking at 103 and 11
>>>> branches and saw this. In that even ExecRemoteQuery seems to have an issue
>>>> wherein it's not using the appropriate context.
>>>>
>>>> Regards,
>>>> Nikhils
>>>>
>>>> Sent from my iPhone
>>>>
>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat <
>>>> ash...@en...> wrote:
>>>>
>>>>
>>>> Welcome to the mess ;) and enjoy junk food.
>>>>
>>>> Sometime back, I have changed ExecRemoteQuery to be called in the same
>>>> fashion as other Scan nodes. So, you will see ExecRemoteQuery calling
>>>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment
>>>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say!
>>>>
>>>> This code needs a lot of rework, removing duplications, using proper
>>>> way of materialisation, central response handler and error handler etc. If
>>>> we clean up this code, some improvements in planner (like using MergeAppend
>>>> plan) for Sort, will be possible. Regarding materialisation, the code uses
>>>> a linked list for materialising the rows from datanodes (in case the same
>>>> connection needs to be given to other remote query node), which must be
>>>> eating a lot of performance. Instead we should be using some kind of
>>>> tuplestore there. We actually use tuplestore (as well) in the RemoteQuery
>>>> node; the same method can be used.
>>>>
>>>>
>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st...>wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Have a Query about ExecRemoteQuery.
>>>>>
>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, that it
>>>>> should have been modeled after ExecScan because we fetch tuples, and those
>>>>> which match the qual should be sent up. ExecMaterial is for materializing
>>>>> and collecting and storing tuples.
>>>>>
>>>>> Can anyone explain? The reason for asking this is I am suspecting a
>>>>> big memory leak in this code path. We are not using any expression context
>>>>> nor we are freeing up tuples as we scan for the one which qualifies.
>>>>>
>>>>> Regards,
>>>>> Nikhils
>>>>> --
>>>>> StormDB - http://www.stormdb.com
>>>>> The Database Cloud
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite!
>>>>> It's a free troubleshooting tool designed for production.
>>>>> Get down to code-level detail for bottlenecks, with <2% overhead.
>>>>> Download for free and get started troubleshooting in minutes.
>>>>>
>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
>>>>> _______________________________________________
>>>>> Postgres-xc-developers mailing list
>>>>> Pos...@li...
>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers
>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Best Wishes,
>>>> Ashutosh Bapat
>>>> EntepriseDB Corporation
>>>> The Postgres Database Company
>>>>
>>>>
>>>
>>>
>>> --
>>> Best Wishes,
>>> Ashutosh Bapat
>>> EntepriseDB Corporation
>>> The Postgres Database Company
>>>
>>
>>
>>
>> --
>> StormDB - http://www.stormdb.com
>> The Database Cloud
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EntepriseDB Corporation
> The Postgres Database Company
>
--
StormDB - http://www.stormdb.com
The Database Cloud
|
|
From: Ashutosh B. <ash...@en...> - 2013-08-14 09:37:16
|
Yes, that's correct. My patch was not intended to fix this. This was added while fixing a bug for parameterised quals on RemoteQuery I think. Check commits by Amit in this area. On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke <ni...@st...>wrote: > Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. > > Here's the snippet from RemoteQueryNext: > > if (tuplestorestate && !TupIsNull(scanslot)) > tuplestore_puttupleslot(tuplestorestate, scanslot); > > I printed the current memory context inside this function, it is > ""ExecutorState". This means that the tuple will stay around till the query > is executing in its entirety! For large COPY queries this is bound to cause > issues as is also reported by Hitoshi san on another thread. > > I propose that in RemoteQueryNext, before calling the > tuplestore_puttupleslot we switch into > scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It will get > reset, when the next tuple has to be returned to the caller and the leak > will be curtailed. Thoughts? > > Regards, > Nikhils > > > > > On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < > ash...@en...> wrote: > >> There has been an overhaul in the planner (and corresponding parts of >> executor) in 1.1, so it would be better if they move to 1.1 after GA. >> >> >> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke <ni...@st...>wrote: >> >>> Ah, I see. >>> >>> I was looking at REL_1_0 sources. >>> >>> There are people out there using REL_1_0 as well. >>> >>> Regards, >>> Nikhils >>> >>> >>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>> ash...@en...> wrote: >>> >>>> It should be part of 1.1 as well. It was done to support projection out >>>> of RemoteQuery node. >>>> >>>> >>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <ni...@st...>wrote: >>>> >>>>> Hi Ashutosh, >>>>> >>>>> I guess you have changed it in pgxc head? I was looking at 103 and 11 >>>>> branches and saw this. In that even ExecRemoteQuery seems to have an issue >>>>> wherein it's not using the appropriate context. >>>>> >>>>> Regards, >>>>> Nikhils >>>>> >>>>> Sent from my iPhone >>>>> >>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>> ash...@en...> wrote: >>>>> >>>>> >>>>> Welcome to the mess ;) and enjoy junk food. >>>>> >>>>> Sometime back, I have changed ExecRemoteQuery to be called in the same >>>>> fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >>>>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >>>>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >>>>> >>>>> This code needs a lot of rework, removing duplications, using proper >>>>> way of materialisation, central response handler and error handler etc. If >>>>> we clean up this code, some improvements in planner (like using MergeAppend >>>>> plan) for Sort, will be possible. Regarding materialisation, the code uses >>>>> a linked list for materialising the rows from datanodes (in case the same >>>>> connection needs to be given to other remote query node), which must be >>>>> eating a lot of performance. Instead we should be using some kind of >>>>> tuplestore there. We actually use tuplestore (as well) in the RemoteQuery >>>>> node; the same method can be used. >>>>> >>>>> >>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <ni...@st... >>>>> > wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> Have a Query about ExecRemoteQuery. >>>>>> >>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, that >>>>>> it should have been modeled after ExecScan because we fetch tuples, and >>>>>> those which match the qual should be sent up. ExecMaterial is for >>>>>> materializing and collecting and storing tuples. >>>>>> >>>>>> Can anyone explain? The reason for asking this is I am suspecting a >>>>>> big memory leak in this code path. We are not using any expression context >>>>>> nor we are freeing up tuples as we scan for the one which qualifies. >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>>> ------------------------------------------------------------------------------ >>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>> It's a free troubleshooting tool designed for production. >>>>>> Get down to code-level detail for bottlenecks, with <2% overhead. >>>>>> Download for free and get started troubleshooting in minutes. >>>>>> >>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>> _______________________________________________ >>>>>> Postgres-xc-developers mailing list >>>>>> Pos...@li... >>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Best Wishes, >>>>> Ashutosh Bapat >>>>> EntepriseDB Corporation >>>>> The Postgres Database Company >>>>> >>>>> >>>> >>>> >>>> -- >>>> Best Wishes, >>>> Ashutosh Bapat >>>> EntepriseDB Corporation >>>> The Postgres Database Company >>>> >>> >>> >>> >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >> >> >> >> -- >> Best Wishes, >> Ashutosh Bapat >> EntepriseDB Corporation >> The Postgres Database Company >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Nikhil S. <ni...@st...> - 2013-08-14 13:36:00
|
Using a tuplestore for data coming from RemoteQuery is kinda wrong and that's what has introduced this issue. Looks like just changing the memory context will not work as it interferes with the other functioning of the tuplestore :-| Regards, Nikhils On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < ash...@en...> wrote: > Yes, that's correct. > > My patch was not intended to fix this. This was added while fixing a bug > for parameterised quals on RemoteQuery I think. Check commits by Amit in > this area. > > > On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke <ni...@st...>wrote: > >> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >> >> Here's the snippet from RemoteQueryNext: >> >> if (tuplestorestate && !TupIsNull(scanslot)) >> tuplestore_puttupleslot(tuplestorestate, scanslot); >> >> I printed the current memory context inside this function, it is >> ""ExecutorState". This means that the tuple will stay around till the query >> is executing in its entirety! For large COPY queries this is bound to cause >> issues as is also reported by Hitoshi san on another thread. >> >> I propose that in RemoteQueryNext, before calling the >> tuplestore_puttupleslot we switch into >> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It will get >> reset, when the next tuple has to be returned to the caller and the leak >> will be curtailed. Thoughts? >> >> Regards, >> Nikhils >> >> >> >> >> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >> ash...@en...> wrote: >> >>> There has been an overhaul in the planner (and corresponding parts of >>> executor) in 1.1, so it would be better if they move to 1.1 after GA. >>> >>> >>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> Ah, I see. >>>> >>>> I was looking at REL_1_0 sources. >>>> >>>> There are people out there using REL_1_0 as well. >>>> >>>> Regards, >>>> Nikhils >>>> >>>> >>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>> ash...@en...> wrote: >>>> >>>>> It should be part of 1.1 as well. It was done to support projection >>>>> out of RemoteQuery node. >>>>> >>>>> >>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <ni...@st...>wrote: >>>>> >>>>>> Hi Ashutosh, >>>>>> >>>>>> I guess you have changed it in pgxc head? I was looking at 103 and 11 >>>>>> branches and saw this. In that even ExecRemoteQuery seems to have an issue >>>>>> wherein it's not using the appropriate context. >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> Sent from my iPhone >>>>>> >>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>> ash...@en...> wrote: >>>>>> >>>>>> >>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>> >>>>>> Sometime back, I have changed ExecRemoteQuery to be called in the >>>>>> same fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >>>>>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >>>>>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >>>>>> >>>>>> This code needs a lot of rework, removing duplications, using proper >>>>>> way of materialisation, central response handler and error handler etc. If >>>>>> we clean up this code, some improvements in planner (like using MergeAppend >>>>>> plan) for Sort, will be possible. Regarding materialisation, the code uses >>>>>> a linked list for materialising the rows from datanodes (in case the same >>>>>> connection needs to be given to other remote query node), which must be >>>>>> eating a lot of performance. Instead we should be using some kind of >>>>>> tuplestore there. We actually use tuplestore (as well) in the RemoteQuery >>>>>> node; the same method can be used. >>>>>> >>>>>> >>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>> ni...@st...> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Have a Query about ExecRemoteQuery. >>>>>>> >>>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, that >>>>>>> it should have been modeled after ExecScan because we fetch tuples, and >>>>>>> those which match the qual should be sent up. ExecMaterial is for >>>>>>> materializing and collecting and storing tuples. >>>>>>> >>>>>>> Can anyone explain? The reason for asking this is I am suspecting a >>>>>>> big memory leak in this code path. We are not using any expression context >>>>>>> nor we are freeing up tuples as we scan for the one which qualifies. >>>>>>> >>>>>>> Regards, >>>>>>> Nikhils >>>>>>> -- >>>>>>> StormDB - http://www.stormdb.com >>>>>>> The Database Cloud >>>>>>> >>>>>>> ------------------------------------------------------------------------------ >>>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>>> It's a free troubleshooting tool designed for production. >>>>>>> Get down to code-level detail for bottlenecks, with <2% overhead. >>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>> >>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>> _______________________________________________ >>>>>>> Postgres-xc-developers mailing list >>>>>>> Pos...@li... >>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Best Wishes, >>>>>> Ashutosh Bapat >>>>>> EntepriseDB Corporation >>>>>> The Postgres Database Company >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Best Wishes, >>>>> Ashutosh Bapat >>>>> EntepriseDB Corporation >>>>> The Postgres Database Company >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> Best Wishes, >>> Ashutosh Bapat >>> EntepriseDB Corporation >>> The Postgres Database Company >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > Best Wishes, > Ashutosh Bapat > EntepriseDB Corporation > The Postgres Database Company > -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Nikhil S. <ni...@st...> - 2013-08-15 17:14:13
|
Looks like my theory was wrong, make installcheck is giving more errors with this patch applied. Will have to look at a different solution.. Regards, Nikhils On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <ni...@st...>wrote: > So, I looked at this code carefully and ISTM, that because of the way we > fetch the data from the connections and return it immediately inside > RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot > is NOT required at all. > > So, I have removed the call to tuplestore_puttupleslot and things seem to > be ok for me. I guess we should do a FULL test run with this patch just to > ensure that it does not cause issues in any scenarios. > > A careful look by new set of eyes will help here. I think, if there are no > issues, this plugs a major leak in the RemoteQuery code path which is > almost always used in our case. > > Regards, > Nikhils > > > On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke <ni...@st...>wrote: > >> Using a tuplestore for data coming from RemoteQuery is kinda wrong and >> that's what has introduced this issue. Looks like just changing the memory >> context will not work as it interferes with the other functioning of the >> tuplestore :-| >> >> Regards, >> Nikhils >> >> >> >> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >> ash...@en...> wrote: >> >>> Yes, that's correct. >>> >>> My patch was not intended to fix this. This was added while fixing a bug >>> for parameterised quals on RemoteQuery I think. Check commits by Amit in >>> this area. >>> >>> >>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>> >>>> Here's the snippet from RemoteQueryNext: >>>> >>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>> tuplestore_puttupleslot(tuplestorestate, scanslot); >>>> >>>> I printed the current memory context inside this function, it is >>>> ""ExecutorState". This means that the tuple will stay around till the query >>>> is executing in its entirety! For large COPY queries this is bound to cause >>>> issues as is also reported by Hitoshi san on another thread. >>>> >>>> I propose that in RemoteQueryNext, before calling the >>>> tuplestore_puttupleslot we switch into >>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It will >>>> get reset, when the next tuple has to be returned to the caller and the >>>> leak will be curtailed. Thoughts? >>>> >>>> Regards, >>>> Nikhils >>>> >>>> >>>> >>>> >>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>> ash...@en...> wrote: >>>> >>>>> There has been an overhaul in the planner (and corresponding parts of >>>>> executor) in 1.1, so it would be better if they move to 1.1 after GA. >>>>> >>>>> >>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke <ni...@st... >>>>> > wrote: >>>>> >>>>>> Ah, I see. >>>>>> >>>>>> I was looking at REL_1_0 sources. >>>>>> >>>>>> There are people out there using REL_1_0 as well. >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> >>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>> ash...@en...> wrote: >>>>>> >>>>>>> It should be part of 1.1 as well. It was done to support projection >>>>>>> out of RemoteQuery node. >>>>>>> >>>>>>> >>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>> ni...@st...> wrote: >>>>>>> >>>>>>>> Hi Ashutosh, >>>>>>>> >>>>>>>> I guess you have changed it in pgxc head? I was looking at 103 and >>>>>>>> 11 branches and saw this. In that even ExecRemoteQuery seems to have an >>>>>>>> issue wherein it's not using the appropriate context. >>>>>>>> >>>>>>>> Regards, >>>>>>>> Nikhils >>>>>>>> >>>>>>>> Sent from my iPhone >>>>>>>> >>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>> ash...@en...> wrote: >>>>>>>> >>>>>>>> >>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>> >>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called in the >>>>>>>> same fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >>>>>>>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >>>>>>>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >>>>>>>> >>>>>>>> This code needs a lot of rework, removing duplications, using >>>>>>>> proper way of materialisation, central response handler and error handler >>>>>>>> etc. If we clean up this code, some improvements in planner (like using >>>>>>>> MergeAppend plan) for Sort, will be possible. Regarding materialisation, >>>>>>>> the code uses a linked list for materialising the rows from datanodes (in >>>>>>>> case the same connection needs to be given to other remote query node), >>>>>>>> which must be eating a lot of performance. Instead we should be using some >>>>>>>> kind of tuplestore there. We actually use tuplestore (as well) in the >>>>>>>> RemoteQuery node; the same method can be used. >>>>>>>> >>>>>>>> >>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>> ni...@st...> wrote: >>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>> >>>>>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, >>>>>>>>> that it should have been modeled after ExecScan because we fetch tuples, >>>>>>>>> and those which match the qual should be sent up. ExecMaterial is for >>>>>>>>> materializing and collecting and storing tuples. >>>>>>>>> >>>>>>>>> Can anyone explain? The reason for asking this is I am suspecting >>>>>>>>> a big memory leak in this code path. We are not using any expression >>>>>>>>> context nor we are freeing up tuples as we scan for the one which qualifies. >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Nikhils >>>>>>>>> -- >>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>> The Database Cloud >>>>>>>>> >>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>> Get down to code-level detail for bottlenecks, with <2% overhead. >>>>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>>>> >>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>> _______________________________________________ >>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>> Pos...@li... >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Best Wishes, >>>>>>>> Ashutosh Bapat >>>>>>>> EntepriseDB Corporation >>>>>>>> The Postgres Database Company >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Best Wishes, >>>>>>> Ashutosh Bapat >>>>>>> EntepriseDB Corporation >>>>>>> The Postgres Database Company >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Best Wishes, >>>>> Ashutosh Bapat >>>>> EntepriseDB Corporation >>>>> The Postgres Database Company >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> Best Wishes, >>> Ashutosh Bapat >>> EntepriseDB Corporation >>> The Postgres Database Company >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Nikhil S. <ni...@st...> - 2013-08-15 18:27:51
|
Looks like we cannot do away with the tuplestore that easily. Especially if the remote query is an inner scan node, then it makes sense to materialize and store the rows in the tuplestore for subsequent rescans. However this should have been based on row statistics. If the query brings in a lot of rows, then we are sure to run out of memory like some reports that we have seen. Additionally, ISTM, that the tuplestore should only be used for inner nodes. There's no need to store outer nodes in tuplestores. Also if it's a single SELECT query, then there's no need to use tuplestore at all as well. Looks like if we can pass hints during plan creation as to whether the remote scan is part of a join (and is inner node) or not, then accordingly decision can be taken to materialize into the tuplestore. Regards, Nikhils On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke <ni...@st...>wrote: > Looks like my theory was wrong, make installcheck is giving more errors > with this patch applied. Will have to look at a different solution.. > > Regards, > Nikhils > > > On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <ni...@st...>wrote: > >> So, I looked at this code carefully and ISTM, that because of the way we >> fetch the data from the connections and return it immediately inside >> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >> is NOT required at all. >> >> So, I have removed the call to tuplestore_puttupleslot and things seem to >> be ok for me. I guess we should do a FULL test run with this patch just to >> ensure that it does not cause issues in any scenarios. >> >> A careful look by new set of eyes will help here. I think, if there are >> no issues, this plugs a major leak in the RemoteQuery code path which is >> almost always used in our case. >> >> Regards, >> Nikhils >> >> >> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke <ni...@st...>wrote: >> >>> Using a tuplestore for data coming from RemoteQuery is kinda wrong and >>> that's what has introduced this issue. Looks like just changing the memory >>> context will not work as it interferes with the other functioning of the >>> tuplestore :-| >>> >>> Regards, >>> Nikhils >>> >>> >>> >>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>> ash...@en...> wrote: >>> >>>> Yes, that's correct. >>>> >>>> My patch was not intended to fix this. This was added while fixing a >>>> bug for parameterised quals on RemoteQuery I think. Check commits by Amit >>>> in this area. >>>> >>>> >>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke <ni...@st...>wrote: >>>> >>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>>> >>>>> Here's the snippet from RemoteQueryNext: >>>>> >>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>> tuplestore_puttupleslot(tuplestorestate, scanslot); >>>>> >>>>> I printed the current memory context inside this function, it is >>>>> ""ExecutorState". This means that the tuple will stay around till the query >>>>> is executing in its entirety! For large COPY queries this is bound to cause >>>>> issues as is also reported by Hitoshi san on another thread. >>>>> >>>>> I propose that in RemoteQueryNext, before calling the >>>>> tuplestore_puttupleslot we switch into >>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It will >>>>> get reset, when the next tuple has to be returned to the caller and the >>>>> leak will be curtailed. Thoughts? >>>>> >>>>> Regards, >>>>> Nikhils >>>>> >>>>> >>>>> >>>>> >>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>> ash...@en...> wrote: >>>>> >>>>>> There has been an overhaul in the planner (and corresponding parts of >>>>>> executor) in 1.1, so it would be better if they move to 1.1 after GA. >>>>>> >>>>>> >>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>> ni...@st...> wrote: >>>>>> >>>>>>> Ah, I see. >>>>>>> >>>>>>> I was looking at REL_1_0 sources. >>>>>>> >>>>>>> There are people out there using REL_1_0 as well. >>>>>>> >>>>>>> Regards, >>>>>>> Nikhils >>>>>>> >>>>>>> >>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>> ash...@en...> wrote: >>>>>>> >>>>>>>> It should be part of 1.1 as well. It was done to support projection >>>>>>>> out of RemoteQuery node. >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>> ni...@st...> wrote: >>>>>>>> >>>>>>>>> Hi Ashutosh, >>>>>>>>> >>>>>>>>> I guess you have changed it in pgxc head? I was looking at 103 and >>>>>>>>> 11 branches and saw this. In that even ExecRemoteQuery seems to have an >>>>>>>>> issue wherein it's not using the appropriate context. >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Nikhils >>>>>>>>> >>>>>>>>> Sent from my iPhone >>>>>>>>> >>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>> ash...@en...> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>> >>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called in the >>>>>>>>> same fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >>>>>>>>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >>>>>>>>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >>>>>>>>> >>>>>>>>> This code needs a lot of rework, removing duplications, using >>>>>>>>> proper way of materialisation, central response handler and error handler >>>>>>>>> etc. If we clean up this code, some improvements in planner (like using >>>>>>>>> MergeAppend plan) for Sort, will be possible. Regarding materialisation, >>>>>>>>> the code uses a linked list for materialising the rows from datanodes (in >>>>>>>>> case the same connection needs to be given to other remote query node), >>>>>>>>> which must be eating a lot of performance. Instead we should be using some >>>>>>>>> kind of tuplestore there. We actually use tuplestore (as well) in the >>>>>>>>> RemoteQuery node; the same method can be used. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>> ni...@st...> wrote: >>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>> >>>>>>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, >>>>>>>>>> that it should have been modeled after ExecScan because we fetch tuples, >>>>>>>>>> and those which match the qual should be sent up. ExecMaterial is for >>>>>>>>>> materializing and collecting and storing tuples. >>>>>>>>>> >>>>>>>>>> Can anyone explain? The reason for asking this is I am suspecting >>>>>>>>>> a big memory leak in this code path. We are not using any expression >>>>>>>>>> context nor we are freeing up tuples as we scan for the one which qualifies. >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Nikhils >>>>>>>>>> -- >>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>> The Database Cloud >>>>>>>>>> >>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% overhead. >>>>>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>>>>> >>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>> _______________________________________________ >>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>> Pos...@li... >>>>>>>>>> >>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Best Wishes, >>>>>>>>> Ashutosh Bapat >>>>>>>>> EntepriseDB Corporation >>>>>>>>> The Postgres Database Company >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Best Wishes, >>>>>>>> Ashutosh Bapat >>>>>>>> EntepriseDB Corporation >>>>>>>> The Postgres Database Company >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> StormDB - http://www.stormdb.com >>>>>>> The Database Cloud >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Best Wishes, >>>>>> Ashutosh Bapat >>>>>> EntepriseDB Corporation >>>>>> The Postgres Database Company >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> StormDB - http://www.stormdb.com >>>>> The Database Cloud >>>>> >>>> >>>> >>>> >>>> -- >>>> Best Wishes, >>>> Ashutosh Bapat >>>> EntepriseDB Corporation >>>> The Postgres Database Company >>>> >>> >>> >>> >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Nikhil S. <ni...@st...> - 2013-08-16 12:20:29
|
> Additionally, ISTM, that the tuplestore should only be used for inner > nodes. There's no need to store outer nodes in tuplestores. Also if it's a > single SELECT query, then there's no need to use tuplestore at all as well. > > PFA, a patch which tries to avoid using the tuplestore in the above two cases. During planning we decide if a tuplestore should be used for the RemoteQuery. The default is true, and we set it to false for the above two cases for now. I ran regression test cases with and without the patch and got the exact same set of failures (and more importantly same diffs). To be clear this patch is not specific to COPY TO, but it's a generic change to avoid using tuplestore in certain simple scenarios thereby reducing the memory footprint of the remote query execution. Note that it also does not solve Hitoshi-san's COPY FROM issues. Will submit a separate patch for that. Regards, Nikhils > Looks like if we can pass hints during plan creation as to whether the > remote scan is part of a join (and is inner node) or not, then accordingly > decision can be taken to materialize into the tuplestore. > > Regards, > Nikhils > > On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke <ni...@st...>wrote: > >> Looks like my theory was wrong, make installcheck is giving more errors >> with this patch applied. Will have to look at a different solution.. >> >> Regards, >> Nikhils >> >> >> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <ni...@st...>wrote: >> >>> So, I looked at this code carefully and ISTM, that because of the way we >>> fetch the data from the connections and return it immediately inside >>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >>> is NOT required at all. >>> >>> So, I have removed the call to tuplestore_puttupleslot and things seem >>> to be ok for me. I guess we should do a FULL test run with this patch just >>> to ensure that it does not cause issues in any scenarios. >>> >>> A careful look by new set of eyes will help here. I think, if there are >>> no issues, this plugs a major leak in the RemoteQuery code path which is >>> almost always used in our case. >>> >>> Regards, >>> Nikhils >>> >>> >>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> Using a tuplestore for data coming from RemoteQuery is kinda wrong and >>>> that's what has introduced this issue. Looks like just changing the memory >>>> context will not work as it interferes with the other functioning of the >>>> tuplestore :-| >>>> >>>> Regards, >>>> Nikhils >>>> >>>> >>>> >>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>>> ash...@en...> wrote: >>>> >>>>> Yes, that's correct. >>>>> >>>>> My patch was not intended to fix this. This was added while fixing a >>>>> bug for parameterised quals on RemoteQuery I think. Check commits by Amit >>>>> in this area. >>>>> >>>>> >>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke <ni...@st...>wrote: >>>>> >>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>>>> >>>>>> Here's the snippet from RemoteQueryNext: >>>>>> >>>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>>> tuplestore_puttupleslot(tuplestorestate, >>>>>> scanslot); >>>>>> >>>>>> I printed the current memory context inside this function, it is >>>>>> ""ExecutorState". This means that the tuple will stay around till the query >>>>>> is executing in its entirety! For large COPY queries this is bound to cause >>>>>> issues as is also reported by Hitoshi san on another thread. >>>>>> >>>>>> I propose that in RemoteQueryNext, before calling the >>>>>> tuplestore_puttupleslot we switch into >>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It will >>>>>> get reset, when the next tuple has to be returned to the caller and the >>>>>> leak will be curtailed. Thoughts? >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>>> ash...@en...> wrote: >>>>>> >>>>>>> There has been an overhaul in the planner (and corresponding parts >>>>>>> of executor) in 1.1, so it would be better if they move to 1.1 after GA. >>>>>>> >>>>>>> >>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>>> ni...@st...> wrote: >>>>>>> >>>>>>>> Ah, I see. >>>>>>>> >>>>>>>> I was looking at REL_1_0 sources. >>>>>>>> >>>>>>>> There are people out there using REL_1_0 as well. >>>>>>>> >>>>>>>> Regards, >>>>>>>> Nikhils >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>>> ash...@en...> wrote: >>>>>>>> >>>>>>>>> It should be part of 1.1 as well. It was done to support >>>>>>>>> projection out of RemoteQuery node. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>>> ni...@st...> wrote: >>>>>>>>> >>>>>>>>>> Hi Ashutosh, >>>>>>>>>> >>>>>>>>>> I guess you have changed it in pgxc head? I was looking at 103 >>>>>>>>>> and 11 branches and saw this. In that even ExecRemoteQuery seems to have an >>>>>>>>>> issue wherein it's not using the appropriate context. >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Nikhils >>>>>>>>>> >>>>>>>>>> Sent from my iPhone >>>>>>>>>> >>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>>> ash...@en...> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>>> >>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called in the >>>>>>>>>> same fashion as other Scan nodes. So, you will see ExecRemoteQuery calling >>>>>>>>>> ExecScan with RemoteQueryNext as the iterator. So, I assume your comment >>>>>>>>>> pertains to RemoteQueryNext and its minions and not ExecRemoteQuery per say! >>>>>>>>>> >>>>>>>>>> This code needs a lot of rework, removing duplications, using >>>>>>>>>> proper way of materialisation, central response handler and error handler >>>>>>>>>> etc. If we clean up this code, some improvements in planner (like using >>>>>>>>>> MergeAppend plan) for Sort, will be possible. Regarding materialisation, >>>>>>>>>> the code uses a linked list for materialising the rows from datanodes (in >>>>>>>>>> case the same connection needs to be given to other remote query node), >>>>>>>>>> which must be eating a lot of performance. Instead we should be using some >>>>>>>>>> kind of tuplestore there. We actually use tuplestore (as well) in the >>>>>>>>>> RemoteQuery node; the same method can be used. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>>> ni...@st...> wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>>> >>>>>>>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, >>>>>>>>>>> that it should have been modeled after ExecScan because we fetch tuples, >>>>>>>>>>> and those which match the qual should be sent up. ExecMaterial is for >>>>>>>>>>> materializing and collecting and storing tuples. >>>>>>>>>>> >>>>>>>>>>> Can anyone explain? The reason for asking this is I am >>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any >>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one >>>>>>>>>>> which qualifies. >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Nikhils >>>>>>>>>>> -- >>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>> The Database Cloud >>>>>>>>>>> >>>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% overhead. >>>>>>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>>>>>> >>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>>> Pos...@li... >>>>>>>>>>> >>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Best Wishes, >>>>>>>>>> Ashutosh Bapat >>>>>>>>>> EntepriseDB Corporation >>>>>>>>>> The Postgres Database Company >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Best Wishes, >>>>>>>>> Ashutosh Bapat >>>>>>>>> EntepriseDB Corporation >>>>>>>>> The Postgres Database Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> StormDB - http://www.stormdb.com >>>>>>>> The Database Cloud >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Best Wishes, >>>>>>> Ashutosh Bapat >>>>>>> EntepriseDB Corporation >>>>>>> The Postgres Database Company >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Best Wishes, >>>>> Ashutosh Bapat >>>>> EntepriseDB Corporation >>>>> The Postgres Database Company >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Ashutosh B. <ash...@en...> - 2013-08-21 09:04:07
|
Hi Nikhil, The patch needs some more comments as to why and when we should materialise or not. Also, I am not so in favour of adding another switch all the way in RemoteQuery path. Well, I am always not in favour of adding new switch, since it needs to be maintained at various places in code; and if somebody forgets to set/reset it somewhere it shows up as bug in entirely different areas. For example, if some code forgets to set/reset this switch while creating path, it would end up as a bug in executor or a performance regression in the executor and would be difficult to trace it back all the way to the path creation. Is there a way, we can do this based on some other members of RemoteQuery or RemoteQueryPath structure? On Fri, Aug 16, 2013 at 5:57 PM, Nikhil Sontakke <ni...@st...>wrote: > Duh.. Patch attached :^) > > > On Fri, Aug 16, 2013 at 5:50 PM, Nikhil Sontakke <ni...@st...>wrote: > >> >> Additionally, ISTM, that the tuplestore should only be used for inner >>> nodes. There's no need to store outer nodes in tuplestores. Also if it's a >>> single SELECT query, then there's no need to use tuplestore at all as well. >>> >>> >> PFA, a patch which tries to avoid using the tuplestore in the above two >> cases. During planning we decide if a tuplestore should be used for the >> RemoteQuery. The default is true, and we set it to false for the above two >> cases for now. >> >> I ran regression test cases with and without the patch and got the exact >> same set of failures (and more importantly same diffs). >> >> To be clear this patch is not specific to COPY TO, but it's a generic >> change to avoid using tuplestore in certain simple scenarios thereby >> reducing the memory footprint of the remote query execution. Note that it >> also does not solve Hitoshi-san's COPY FROM issues. Will submit a separate >> patch for that. >> >> Regards, >> Nikhils >> >> >> >>> Looks like if we can pass hints during plan creation as to whether the >>> remote scan is part of a join (and is inner node) or not, then accordingly >>> decision can be taken to materialize into the tuplestore. >>> >>> Regards, >>> Nikhils >>> >>> On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> Looks like my theory was wrong, make installcheck is giving more errors >>>> with this patch applied. Will have to look at a different solution.. >>>> >>>> Regards, >>>> Nikhils >>>> >>>> >>>> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <ni...@st...>wrote: >>>> >>>>> So, I looked at this code carefully and ISTM, that because of the way >>>>> we fetch the data from the connections and return it immediately inside >>>>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >>>>> is NOT required at all. >>>>> >>>>> So, I have removed the call to tuplestore_puttupleslot and things seem >>>>> to be ok for me. I guess we should do a FULL test run with this patch just >>>>> to ensure that it does not cause issues in any scenarios. >>>>> >>>>> A careful look by new set of eyes will help here. I think, if there >>>>> are no issues, this plugs a major leak in the RemoteQuery code path which >>>>> is almost always used in our case. >>>>> >>>>> Regards, >>>>> Nikhils >>>>> >>>>> >>>>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke <ni...@st...>wrote: >>>>> >>>>>> Using a tuplestore for data coming from RemoteQuery is kinda wrong >>>>>> and that's what has introduced this issue. Looks like just changing the >>>>>> memory context will not work as it interferes with the other functioning of >>>>>> the tuplestore :-| >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> >>>>>> >>>>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>>>>> ash...@en...> wrote: >>>>>> >>>>>>> Yes, that's correct. >>>>>>> >>>>>>> My patch was not intended to fix this. This was added while fixing a >>>>>>> bug for parameterised quals on RemoteQuery I think. Check commits by Amit >>>>>>> in this area. >>>>>>> >>>>>>> >>>>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke < >>>>>>> ni...@st...> wrote: >>>>>>> >>>>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>>>>>> >>>>>>>> Here's the snippet from RemoteQueryNext: >>>>>>>> >>>>>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>>>>> tuplestore_puttupleslot(tuplestorestate, >>>>>>>> scanslot); >>>>>>>> >>>>>>>> I printed the current memory context inside this function, it is >>>>>>>> ""ExecutorState". This means that the tuple will stay around till the query >>>>>>>> is executing in its entirety! For large COPY queries this is bound to cause >>>>>>>> issues as is also reported by Hitoshi san on another thread. >>>>>>>> >>>>>>>> I propose that in RemoteQueryNext, before calling the >>>>>>>> tuplestore_puttupleslot we switch into >>>>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It >>>>>>>> will get reset, when the next tuple has to be returned to the caller and >>>>>>>> the leak will be curtailed. Thoughts? >>>>>>>> >>>>>>>> Regards, >>>>>>>> Nikhils >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>>>>> ash...@en...> wrote: >>>>>>>> >>>>>>>>> There has been an overhaul in the planner (and corresponding parts >>>>>>>>> of executor) in 1.1, so it would be better if they move to 1.1 after GA. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>>>>> ni...@st...> wrote: >>>>>>>>> >>>>>>>>>> Ah, I see. >>>>>>>>>> >>>>>>>>>> I was looking at REL_1_0 sources. >>>>>>>>>> >>>>>>>>>> There are people out there using REL_1_0 as well. >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Nikhils >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>>>>> ash...@en...> wrote: >>>>>>>>>> >>>>>>>>>>> It should be part of 1.1 as well. It was done to support >>>>>>>>>>> projection out of RemoteQuery node. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi Ashutosh, >>>>>>>>>>>> >>>>>>>>>>>> I guess you have changed it in pgxc head? I was looking at 103 >>>>>>>>>>>> and 11 branches and saw this. In that even ExecRemoteQuery seems to have an >>>>>>>>>>>> issue wherein it's not using the appropriate context. >>>>>>>>>>>> >>>>>>>>>>>> Regards, >>>>>>>>>>>> Nikhils >>>>>>>>>>>> >>>>>>>>>>>> Sent from my iPhone >>>>>>>>>>>> >>>>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>>>>> >>>>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called in >>>>>>>>>>>> the same fashion as other Scan nodes. So, you will see ExecRemoteQuery >>>>>>>>>>>> calling ExecScan with RemoteQueryNext as the iterator. So, I assume your >>>>>>>>>>>> comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery >>>>>>>>>>>> per say! >>>>>>>>>>>> >>>>>>>>>>>> This code needs a lot of rework, removing duplications, using >>>>>>>>>>>> proper way of materialisation, central response handler and error handler >>>>>>>>>>>> etc. If we clean up this code, some improvements in planner (like using >>>>>>>>>>>> MergeAppend plan) for Sort, will be possible. Regarding materialisation, >>>>>>>>>>>> the code uses a linked list for materialising the rows from datanodes (in >>>>>>>>>>>> case the same connection needs to be given to other remote query node), >>>>>>>>>>>> which must be eating a lot of performance. Instead we should be using some >>>>>>>>>>>> kind of tuplestore there. We actually use tuplestore (as well) in the >>>>>>>>>>>> RemoteQuery node; the same method can be used. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> >>>>>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>>>>> >>>>>>>>>>>>> The logic seems to have been modeled after ExecMaterial. ISTM, >>>>>>>>>>>>> that it should have been modeled after ExecScan because we fetch tuples, >>>>>>>>>>>>> and those which match the qual should be sent up. ExecMaterial is for >>>>>>>>>>>>> materializing and collecting and storing tuples. >>>>>>>>>>>>> >>>>>>>>>>>>> Can anyone explain? The reason for asking this is I am >>>>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any >>>>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one >>>>>>>>>>>>> which qualifies. >>>>>>>>>>>>> >>>>>>>>>>>>> Regards, >>>>>>>>>>>>> Nikhils >>>>>>>>>>>>> -- >>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>> >>>>>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% >>>>>>>>>>>>> overhead. >>>>>>>>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>>>>>>>> >>>>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>>>>> Pos...@li... >>>>>>>>>>>>> >>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Best Wishes, >>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Best Wishes, >>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>> The Postgres Database Company >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>> The Database Cloud >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Best Wishes, >>>>>>>>> Ashutosh Bapat >>>>>>>>> EntepriseDB Corporation >>>>>>>>> The Postgres Database Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> StormDB - http://www.stormdb.com >>>>>>>> The Database Cloud >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Best Wishes, >>>>>>> Ashutosh Bapat >>>>>>> EntepriseDB Corporation >>>>>>> The Postgres Database Company >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> StormDB - http://www.stormdb.com >>>>> The Database Cloud >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Nikhil S. <ni...@st...> - 2013-08-21 10:25:06
|
> The patch needs some more comments as to why and when we should > materialise or not. > The existing code *always* materializes in all cases which can be a huge performance penalty in some cases. This patch tries to address those cases. > Also, I am not so in favour of adding another switch all the way in > RemoteQuery path. Well, I am always not in favour of adding new switch, > since it needs to be maintained at various places in code; and if somebody > forgets to set/reset it somewhere it shows up as bug in entirely different > areas. For example, if some code forgets to set/reset this switch while > creating path, it would end up as a bug in executor or a performance > regression in the executor and would be difficult to trace it back all the > way to the path creation. > This patch sets it to true when we initialize the remote query. It's not a conditional setting. So it's always set. That ways we avoid causing havoc in other parts until unless we know about other cases where it's not needed. > Is there a way, we can do this based on some other members of RemoteQuery > or RemoteQueryPath structure? > > I did not find any and this follows the standard percolating information down from higher nodes to lower nodes strategy. While execution we rarely have any logic to go to the parent to check for stuff, so this is pretty consistent with existing code I would say. Regards, Nikhils > > On Fri, Aug 16, 2013 at 5:57 PM, Nikhil Sontakke <ni...@st...>wrote: > >> Duh.. Patch attached :^) >> >> >> On Fri, Aug 16, 2013 at 5:50 PM, Nikhil Sontakke <ni...@st...>wrote: >> >>> >>> Additionally, ISTM, that the tuplestore should only be used for inner >>>> nodes. There's no need to store outer nodes in tuplestores. Also if it's a >>>> single SELECT query, then there's no need to use tuplestore at all as well. >>>> >>>> >>> PFA, a patch which tries to avoid using the tuplestore in the above two >>> cases. During planning we decide if a tuplestore should be used for the >>> RemoteQuery. The default is true, and we set it to false for the above two >>> cases for now. >>> >>> I ran regression test cases with and without the patch and got the exact >>> same set of failures (and more importantly same diffs). >>> >>> To be clear this patch is not specific to COPY TO, but it's a generic >>> change to avoid using tuplestore in certain simple scenarios thereby >>> reducing the memory footprint of the remote query execution. Note that it >>> also does not solve Hitoshi-san's COPY FROM issues. Will submit a separate >>> patch for that. >>> >>> Regards, >>> Nikhils >>> >>> >>> >>>> Looks like if we can pass hints during plan creation as to whether the >>>> remote scan is part of a join (and is inner node) or not, then accordingly >>>> decision can be taken to materialize into the tuplestore. >>>> >>>> Regards, >>>> Nikhils >>>> >>>> On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke <ni...@st...>wrote: >>>> >>>>> Looks like my theory was wrong, make installcheck is giving more >>>>> errors with this patch applied. Will have to look at a different solution.. >>>>> >>>>> Regards, >>>>> Nikhils >>>>> >>>>> >>>>> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <ni...@st...>wrote: >>>>> >>>>>> So, I looked at this code carefully and ISTM, that because of the way >>>>>> we fetch the data from the connections and return it immediately inside >>>>>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >>>>>> is NOT required at all. >>>>>> >>>>>> So, I have removed the call to tuplestore_puttupleslot and things >>>>>> seem to be ok for me. I guess we should do a FULL test run with this patch >>>>>> just to ensure that it does not cause issues in any scenarios. >>>>>> >>>>>> A careful look by new set of eyes will help here. I think, if there >>>>>> are no issues, this plugs a major leak in the RemoteQuery code path which >>>>>> is almost always used in our case. >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> >>>>>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke <ni...@st... >>>>>> > wrote: >>>>>> >>>>>>> Using a tuplestore for data coming from RemoteQuery is kinda wrong >>>>>>> and that's what has introduced this issue. Looks like just changing the >>>>>>> memory context will not work as it interferes with the other functioning of >>>>>>> the tuplestore :-| >>>>>>> >>>>>>> Regards, >>>>>>> Nikhils >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>>>>>> ash...@en...> wrote: >>>>>>> >>>>>>>> Yes, that's correct. >>>>>>>> >>>>>>>> My patch was not intended to fix this. This was added while fixing >>>>>>>> a bug for parameterised quals on RemoteQuery I think. Check commits by Amit >>>>>>>> in this area. >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke < >>>>>>>> ni...@st...> wrote: >>>>>>>> >>>>>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>>>>>>> >>>>>>>>> Here's the snippet from RemoteQueryNext: >>>>>>>>> >>>>>>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>>>>>> tuplestore_puttupleslot(tuplestorestate, >>>>>>>>> scanslot); >>>>>>>>> >>>>>>>>> I printed the current memory context inside this function, it is >>>>>>>>> ""ExecutorState". This means that the tuple will stay around till the query >>>>>>>>> is executing in its entirety! For large COPY queries this is bound to cause >>>>>>>>> issues as is also reported by Hitoshi san on another thread. >>>>>>>>> >>>>>>>>> I propose that in RemoteQueryNext, before calling the >>>>>>>>> tuplestore_puttupleslot we switch into >>>>>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It >>>>>>>>> will get reset, when the next tuple has to be returned to the caller and >>>>>>>>> the leak will be curtailed. Thoughts? >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Nikhils >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>>>>>> ash...@en...> wrote: >>>>>>>>> >>>>>>>>>> There has been an overhaul in the planner (and corresponding >>>>>>>>>> parts of executor) in 1.1, so it would be better if they move to 1.1 after >>>>>>>>>> GA. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>>>>>> ni...@st...> wrote: >>>>>>>>>> >>>>>>>>>>> Ah, I see. >>>>>>>>>>> >>>>>>>>>>> I was looking at REL_1_0 sources. >>>>>>>>>>> >>>>>>>>>>> There are people out there using REL_1_0 as well. >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Nikhils >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>> >>>>>>>>>>>> It should be part of 1.1 as well. It was done to support >>>>>>>>>>>> projection out of RemoteQuery node. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi Ashutosh, >>>>>>>>>>>>> >>>>>>>>>>>>> I guess you have changed it in pgxc head? I was looking at 103 >>>>>>>>>>>>> and 11 branches and saw this. In that even ExecRemoteQuery seems to have an >>>>>>>>>>>>> issue wherein it's not using the appropriate context. >>>>>>>>>>>>> >>>>>>>>>>>>> Regards, >>>>>>>>>>>>> Nikhils >>>>>>>>>>>>> >>>>>>>>>>>>> Sent from my iPhone >>>>>>>>>>>>> >>>>>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>>>>>> >>>>>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called in >>>>>>>>>>>>> the same fashion as other Scan nodes. So, you will see ExecRemoteQuery >>>>>>>>>>>>> calling ExecScan with RemoteQueryNext as the iterator. So, I assume your >>>>>>>>>>>>> comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery >>>>>>>>>>>>> per say! >>>>>>>>>>>>> >>>>>>>>>>>>> This code needs a lot of rework, removing duplications, using >>>>>>>>>>>>> proper way of materialisation, central response handler and error handler >>>>>>>>>>>>> etc. If we clean up this code, some improvements in planner (like using >>>>>>>>>>>>> MergeAppend plan) for Sort, will be possible. Regarding materialisation, >>>>>>>>>>>>> the code uses a linked list for materialising the rows from datanodes (in >>>>>>>>>>>>> case the same connection needs to be given to other remote query node), >>>>>>>>>>>>> which must be eating a lot of performance. Instead we should be using some >>>>>>>>>>>>> kind of tuplestore there. We actually use tuplestore (as well) in the >>>>>>>>>>>>> RemoteQuery node; the same method can be used. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>> >>>>>>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>>>>>> >>>>>>>>>>>>>> The logic seems to have been modeled after ExecMaterial. >>>>>>>>>>>>>> ISTM, that it should have been modeled after ExecScan because we fetch >>>>>>>>>>>>>> tuples, and those which match the qual should be sent up. ExecMaterial is >>>>>>>>>>>>>> for materializing and collecting and storing tuples. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Can anyone explain? The reason for asking this is I am >>>>>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any >>>>>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one >>>>>>>>>>>>>> which qualifies. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>>> >>>>>>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics Lite! >>>>>>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% >>>>>>>>>>>>>> overhead. >>>>>>>>>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>>>>>>>>> >>>>>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>>>>>> Pos...@li... >>>>>>>>>>>>>> >>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Best Wishes, >>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>> The Database Cloud >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Best Wishes, >>>>>>>>>> Ashutosh Bapat >>>>>>>>>> EntepriseDB Corporation >>>>>>>>>> The Postgres Database Company >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>> The Database Cloud >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Best Wishes, >>>>>>>> Ashutosh Bapat >>>>>>>> EntepriseDB Corporation >>>>>>>> The Postgres Database Company >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> StormDB - http://www.stormdb.com >>>>>>> The Database Cloud >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> StormDB - http://www.stormdb.com >>>>> The Database Cloud >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > Best Wishes, > Ashutosh Bapat > EntepriseDB Corporation > The Postgres Database Company > -- StormDB - http://www.stormdb.com The Database Cloud |
|
From: Ashutosh B. <ash...@en...> - 2013-08-21 10:30:45
|
On Wed, Aug 21, 2013 at 3:54 PM, Nikhil Sontakke <ni...@st...>wrote: > > The patch needs some more comments as to why and when we should >> materialise or not. >> > > The existing code *always* materializes in all cases which can be a huge > performance penalty in some cases. This patch tries to address those cases. > There are no comments in the code as to WHY we need to re/set this flag wherever set/reset. > > >> Also, I am not so in favour of adding another switch all the way in >> RemoteQuery path. Well, I am always not in favour of adding new switch, >> since it needs to be maintained at various places in code; and if somebody >> forgets to set/reset it somewhere it shows up as bug in entirely different >> areas. For example, if some code forgets to set/reset this switch while >> creating path, it would end up as a bug in executor or a performance >> regression in the executor and would be difficult to trace it back all the >> way to the path creation. >> > > This patch sets it to true when we initialize the remote query. It's not a > conditional setting. So it's always set. That ways we avoid causing havoc > in other parts until unless we know about other cases where it's not needed. > I am talking about some code added in future (and since this is very happening area, we will add code in near future), where the coder or reviewer has to be congnizant that this flag needs to be set/reset at some places. > > >> Is there a way, we can do this based on some other members of RemoteQuery >> or RemoteQueryPath structure? >> >> > I did not find any and this follows the standard percolating information > down from higher nodes to lower nodes strategy. While execution we rarely > have any logic to go to the parent to check for stuff, so this is pretty > consistent with existing code I would say. > > If possible, we do use member of Plan/Parse node/s in execution code. It's not a common practice to percolate the information down through every structure that you come across. > Regards, > Nikhils > > > >> >> On Fri, Aug 16, 2013 at 5:57 PM, Nikhil Sontakke <ni...@st...>wrote: >> >>> Duh.. Patch attached :^) >>> >>> >>> On Fri, Aug 16, 2013 at 5:50 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> >>>> Additionally, ISTM, that the tuplestore should only be used for inner >>>>> nodes. There's no need to store outer nodes in tuplestores. Also if it's a >>>>> single SELECT query, then there's no need to use tuplestore at all as well. >>>>> >>>>> >>>> PFA, a patch which tries to avoid using the tuplestore in the above two >>>> cases. During planning we decide if a tuplestore should be used for the >>>> RemoteQuery. The default is true, and we set it to false for the above two >>>> cases for now. >>>> >>>> I ran regression test cases with and without the patch and got the >>>> exact same set of failures (and more importantly same diffs). >>>> >>>> To be clear this patch is not specific to COPY TO, but it's a generic >>>> change to avoid using tuplestore in certain simple scenarios thereby >>>> reducing the memory footprint of the remote query execution. Note that it >>>> also does not solve Hitoshi-san's COPY FROM issues. Will submit a separate >>>> patch for that. >>>> >>>> Regards, >>>> Nikhils >>>> >>>> >>>> >>>>> Looks like if we can pass hints during plan creation as to whether the >>>>> remote scan is part of a join (and is inner node) or not, then accordingly >>>>> decision can be taken to materialize into the tuplestore. >>>>> >>>>> Regards, >>>>> Nikhils >>>>> >>>>> On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke <ni...@st... >>>>> > wrote: >>>>> >>>>>> Looks like my theory was wrong, make installcheck is giving more >>>>>> errors with this patch applied. Will have to look at a different solution.. >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> >>>>>> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <ni...@st... >>>>>> > wrote: >>>>>> >>>>>>> So, I looked at this code carefully and ISTM, that because of the >>>>>>> way we fetch the data from the connections and return it immediately inside >>>>>>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >>>>>>> is NOT required at all. >>>>>>> >>>>>>> So, I have removed the call to tuplestore_puttupleslot and things >>>>>>> seem to be ok for me. I guess we should do a FULL test run with this patch >>>>>>> just to ensure that it does not cause issues in any scenarios. >>>>>>> >>>>>>> A careful look by new set of eyes will help here. I think, if there >>>>>>> are no issues, this plugs a major leak in the RemoteQuery code path which >>>>>>> is almost always used in our case. >>>>>>> >>>>>>> Regards, >>>>>>> Nikhils >>>>>>> >>>>>>> >>>>>>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke < >>>>>>> ni...@st...> wrote: >>>>>>> >>>>>>>> Using a tuplestore for data coming from RemoteQuery is kinda wrong >>>>>>>> and that's what has introduced this issue. Looks like just changing the >>>>>>>> memory context will not work as it interferes with the other functioning of >>>>>>>> the tuplestore :-| >>>>>>>> >>>>>>>> Regards, >>>>>>>> Nikhils >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>>>>>>> ash...@en...> wrote: >>>>>>>> >>>>>>>>> Yes, that's correct. >>>>>>>>> >>>>>>>>> My patch was not intended to fix this. This was added while fixing >>>>>>>>> a bug for parameterised quals on RemoteQuery I think. Check commits by Amit >>>>>>>>> in this area. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke < >>>>>>>>> ni...@st...> wrote: >>>>>>>>> >>>>>>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>>>>>>>> >>>>>>>>>> Here's the snippet from RemoteQueryNext: >>>>>>>>>> >>>>>>>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>>>>>>> tuplestore_puttupleslot(tuplestorestate, >>>>>>>>>> scanslot); >>>>>>>>>> >>>>>>>>>> I printed the current memory context inside this function, it is >>>>>>>>>> ""ExecutorState". This means that the tuple will stay around till the query >>>>>>>>>> is executing in its entirety! For large COPY queries this is bound to cause >>>>>>>>>> issues as is also reported by Hitoshi san on another thread. >>>>>>>>>> >>>>>>>>>> I propose that in RemoteQueryNext, before calling the >>>>>>>>>> tuplestore_puttupleslot we switch into >>>>>>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It >>>>>>>>>> will get reset, when the next tuple has to be returned to the caller and >>>>>>>>>> the leak will be curtailed. Thoughts? >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Nikhils >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>>>>>>> ash...@en...> wrote: >>>>>>>>>> >>>>>>>>>>> There has been an overhaul in the planner (and corresponding >>>>>>>>>>> parts of executor) in 1.1, so it would be better if they move to 1.1 after >>>>>>>>>>> GA. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>> >>>>>>>>>>>> Ah, I see. >>>>>>>>>>>> >>>>>>>>>>>> I was looking at REL_1_0 sources. >>>>>>>>>>>> >>>>>>>>>>>> There are people out there using REL_1_0 as well. >>>>>>>>>>>> >>>>>>>>>>>> Regards, >>>>>>>>>>>> Nikhils >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> It should be part of 1.1 as well. It was done to support >>>>>>>>>>>>> projection out of RemoteQuery node. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Ashutosh, >>>>>>>>>>>>>> >>>>>>>>>>>>>> I guess you have changed it in pgxc head? I was looking at >>>>>>>>>>>>>> 103 and 11 branches and saw this. In that even ExecRemoteQuery seems to >>>>>>>>>>>>>> have an issue wherein it's not using the appropriate context. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>> >>>>>>>>>>>>>> Sent from my iPhone >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called in >>>>>>>>>>>>>> the same fashion as other Scan nodes. So, you will see ExecRemoteQuery >>>>>>>>>>>>>> calling ExecScan with RemoteQueryNext as the iterator. So, I assume your >>>>>>>>>>>>>> comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery >>>>>>>>>>>>>> per say! >>>>>>>>>>>>>> >>>>>>>>>>>>>> This code needs a lot of rework, removing duplications, using >>>>>>>>>>>>>> proper way of materialisation, central response handler and error handler >>>>>>>>>>>>>> etc. If we clean up this code, some improvements in planner (like using >>>>>>>>>>>>>> MergeAppend plan) for Sort, will be possible. Regarding materialisation, >>>>>>>>>>>>>> the code uses a linked list for materialising the rows from datanodes (in >>>>>>>>>>>>>> case the same connection needs to be given to other remote query node), >>>>>>>>>>>>>> which must be eating a lot of performance. Instead we should be using some >>>>>>>>>>>>>> kind of tuplestore there. We actually use tuplestore (as well) in the >>>>>>>>>>>>>> RemoteQuery node; the same method can be used. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The logic seems to have been modeled after ExecMaterial. >>>>>>>>>>>>>>> ISTM, that it should have been modeled after ExecScan because we fetch >>>>>>>>>>>>>>> tuples, and those which match the qual should be sent up. ExecMaterial is >>>>>>>>>>>>>>> for materializing and collecting and storing tuples. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Can anyone explain? The reason for asking this is I am >>>>>>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any >>>>>>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one >>>>>>>>>>>>>>> which qualifies. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics >>>>>>>>>>>>>>> Lite! >>>>>>>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% >>>>>>>>>>>>>>> overhead. >>>>>>>>>>>>>>> Download for free and get started troubleshooting in minutes. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>>>>>>> Pos...@li... >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>> The Database Cloud >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Best Wishes, >>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>> The Postgres Database Company >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>> The Database Cloud >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Best Wishes, >>>>>>>>> Ashutosh Bapat >>>>>>>>> EntepriseDB Corporation >>>>>>>>> The Postgres Database Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> StormDB - http://www.stormdb.com >>>>>>>> The Database Cloud >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> StormDB - http://www.stormdb.com >>>>>>> The Database Cloud >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> StormDB - http://www.stormdb.com >>>>> The Database Cloud >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> StormDB - http://www.stormdb.com >>> The Database Cloud >>> >> >> >> >> -- >> Best Wishes, >> Ashutosh Bapat >> EntepriseDB Corporation >> The Postgres Database Company >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Ashutosh B. <ash...@en...> - 2013-08-21 10:39:37
|
I think, I need to be more clear on this. To me and to the reader of the code, it should be clear as to why or why not (or when or when not) to have this flag set/reset. The comments in the code do not say that. They just given one particular case where materialisation is not needed and again, we don't know why it's not needed. If you have seen the code, we reduce paths and not plans. So, when I am reducing two paths into one RemoteQuery path, what should happen to this flag? How should it be used if it's set in both the paths/only one of the path etc.? On Wed, Aug 21, 2013 at 4:00 PM, Ashutosh Bapat < ash...@en...> wrote: > > > > On Wed, Aug 21, 2013 at 3:54 PM, Nikhil Sontakke <ni...@st...>wrote: > >> >> The patch needs some more comments as to why and when we should >>> materialise or not. >>> >> >> The existing code *always* materializes in all cases which can be a huge >> performance penalty in some cases. This patch tries to address those cases. >> > > There are no comments in the code as to WHY we need to re/set this flag > wherever set/reset. > > >> >> >>> Also, I am not so in favour of adding another switch all the way in >>> RemoteQuery path. Well, I am always not in favour of adding new switch, >>> since it needs to be maintained at various places in code; and if somebody >>> forgets to set/reset it somewhere it shows up as bug in entirely different >>> areas. For example, if some code forgets to set/reset this switch while >>> creating path, it would end up as a bug in executor or a performance >>> regression in the executor and would be difficult to trace it back all the >>> way to the path creation. >>> >> >> This patch sets it to true when we initialize the remote query. It's not >> a conditional setting. So it's always set. That ways we avoid causing havoc >> in other parts until unless we know about other cases where it's not needed. >> > > I am talking about some code added in future (and since this is very > happening area, we will add code in near future), where the coder or > reviewer has to be congnizant that this flag needs to be set/reset at some > places. > >> >> >>> Is there a way, we can do this based on some other members of >>> RemoteQuery or RemoteQueryPath structure? >>> >>> >> I did not find any and this follows the standard percolating information >> down from higher nodes to lower nodes strategy. While execution we rarely >> have any logic to go to the parent to check for stuff, so this is pretty >> consistent with existing code I would say. >> >> > If possible, we do use member of Plan/Parse node/s in execution code. It's > not a common practice to percolate the information down through every > structure that you come across. > > >> Regards, >> Nikhils >> >> >> >>> >>> On Fri, Aug 16, 2013 at 5:57 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> Duh.. Patch attached :^) >>>> >>>> >>>> On Fri, Aug 16, 2013 at 5:50 PM, Nikhil Sontakke <ni...@st...>wrote: >>>> >>>>> >>>>> Additionally, ISTM, that the tuplestore should only be used for inner >>>>>> nodes. There's no need to store outer nodes in tuplestores. Also if it's a >>>>>> single SELECT query, then there's no need to use tuplestore at all as well. >>>>>> >>>>>> >>>>> PFA, a patch which tries to avoid using the tuplestore in the above >>>>> two cases. During planning we decide if a tuplestore should be used for the >>>>> RemoteQuery. The default is true, and we set it to false for the above two >>>>> cases for now. >>>>> >>>>> I ran regression test cases with and without the patch and got the >>>>> exact same set of failures (and more importantly same diffs). >>>>> >>>>> To be clear this patch is not specific to COPY TO, but it's a generic >>>>> change to avoid using tuplestore in certain simple scenarios thereby >>>>> reducing the memory footprint of the remote query execution. Note that it >>>>> also does not solve Hitoshi-san's COPY FROM issues. Will submit a separate >>>>> patch for that. >>>>> >>>>> Regards, >>>>> Nikhils >>>>> >>>>> >>>>> >>>>>> Looks like if we can pass hints during plan creation as to whether >>>>>> the remote scan is part of a join (and is inner node) or not, then >>>>>> accordingly decision can be taken to materialize into the tuplestore. >>>>>> >>>>>> Regards, >>>>>> Nikhils >>>>>> >>>>>> On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke < >>>>>> ni...@st...> wrote: >>>>>> >>>>>>> Looks like my theory was wrong, make installcheck is giving more >>>>>>> errors with this patch applied. Will have to look at a different solution.. >>>>>>> >>>>>>> Regards, >>>>>>> Nikhils >>>>>>> >>>>>>> >>>>>>> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke < >>>>>>> ni...@st...> wrote: >>>>>>> >>>>>>>> So, I looked at this code carefully and ISTM, that because of the >>>>>>>> way we fetch the data from the connections and return it immediately inside >>>>>>>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >>>>>>>> is NOT required at all. >>>>>>>> >>>>>>>> So, I have removed the call to tuplestore_puttupleslot and things >>>>>>>> seem to be ok for me. I guess we should do a FULL test run with this patch >>>>>>>> just to ensure that it does not cause issues in any scenarios. >>>>>>>> >>>>>>>> A careful look by new set of eyes will help here. I think, if there >>>>>>>> are no issues, this plugs a major leak in the RemoteQuery code path which >>>>>>>> is almost always used in our case. >>>>>>>> >>>>>>>> Regards, >>>>>>>> Nikhils >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke < >>>>>>>> ni...@st...> wrote: >>>>>>>> >>>>>>>>> Using a tuplestore for data coming from RemoteQuery is kinda wrong >>>>>>>>> and that's what has introduced this issue. Looks like just changing the >>>>>>>>> memory context will not work as it interferes with the other functioning of >>>>>>>>> the tuplestore :-| >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Nikhils >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>>>>>>>> ash...@en...> wrote: >>>>>>>>> >>>>>>>>>> Yes, that's correct. >>>>>>>>>> >>>>>>>>>> My patch was not intended to fix this. This was added while >>>>>>>>>> fixing a bug for parameterised quals on RemoteQuery I think. Check commits >>>>>>>>>> by Amit in this area. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke < >>>>>>>>>> ni...@st...> wrote: >>>>>>>>>> >>>>>>>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it. >>>>>>>>>>> >>>>>>>>>>> Here's the snippet from RemoteQueryNext: >>>>>>>>>>> >>>>>>>>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>>>>>>>> tuplestore_puttupleslot(tuplestorestate, >>>>>>>>>>> scanslot); >>>>>>>>>>> >>>>>>>>>>> I printed the current memory context inside this function, it is >>>>>>>>>>> ""ExecutorState". This means that the tuple will stay around till the query >>>>>>>>>>> is executing in its entirety! For large COPY queries this is bound to cause >>>>>>>>>>> issues as is also reported by Hitoshi san on another thread. >>>>>>>>>>> >>>>>>>>>>> I propose that in RemoteQueryNext, before calling the >>>>>>>>>>> tuplestore_puttupleslot we switch into >>>>>>>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. It >>>>>>>>>>> will get reset, when the next tuple has to be returned to the caller and >>>>>>>>>>> the leak will be curtailed. Thoughts? >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Nikhils >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>> >>>>>>>>>>>> There has been an overhaul in the planner (and corresponding >>>>>>>>>>>> parts of executor) in 1.1, so it would be better if they move to 1.1 after >>>>>>>>>>>> GA. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Ah, I see. >>>>>>>>>>>>> >>>>>>>>>>>>> I was looking at REL_1_0 sources. >>>>>>>>>>>>> >>>>>>>>>>>>> There are people out there using REL_1_0 as well. >>>>>>>>>>>>> >>>>>>>>>>>>> Regards, >>>>>>>>>>>>> Nikhils >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> It should be part of 1.1 as well. It was done to support >>>>>>>>>>>>>> projection out of RemoteQuery node. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hi Ashutosh, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I guess you have changed it in pgxc head? I was looking at >>>>>>>>>>>>>>> 103 and 11 branches and saw this. In that even ExecRemoteQuery seems to >>>>>>>>>>>>>>> have an issue wherein it's not using the appropriate context. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Sent from my iPhone >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called >>>>>>>>>>>>>>> in the same fashion as other Scan nodes. So, you will see ExecRemoteQuery >>>>>>>>>>>>>>> calling ExecScan with RemoteQueryNext as the iterator. So, I assume your >>>>>>>>>>>>>>> comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery >>>>>>>>>>>>>>> per say! >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> This code needs a lot of rework, removing duplications, >>>>>>>>>>>>>>> using proper way of materialisation, central response handler and error >>>>>>>>>>>>>>> handler etc. If we clean up this code, some improvements in planner (like >>>>>>>>>>>>>>> using MergeAppend plan) for Sort, will be possible. Regarding >>>>>>>>>>>>>>> materialisation, the code uses a linked list for materialising the rows >>>>>>>>>>>>>>> from datanodes (in case the same connection needs to be given to other >>>>>>>>>>>>>>> remote query node), which must be eating a lot of performance. Instead we >>>>>>>>>>>>>>> should be using some kind of tuplestore there. We actually use tuplestore >>>>>>>>>>>>>>> (as well) in the RemoteQuery node; the same method can be used. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> The logic seems to have been modeled after ExecMaterial. >>>>>>>>>>>>>>>> ISTM, that it should have been modeled after ExecScan because we fetch >>>>>>>>>>>>>>>> tuples, and those which match the qual should be sent up. ExecMaterial is >>>>>>>>>>>>>>>> for materializing and collecting and storing tuples. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Can anyone explain? The reason for asking this is I am >>>>>>>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any >>>>>>>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one >>>>>>>>>>>>>>>> which qualifies. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics >>>>>>>>>>>>>>>> Lite! >>>>>>>>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% >>>>>>>>>>>>>>>> overhead. >>>>>>>>>>>>>>>> Download for free and get started troubleshooting in >>>>>>>>>>>>>>>> minutes. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>>>>>>>> Pos...@li... >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Best Wishes, >>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>> The Database Cloud >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Best Wishes, >>>>>>>>>> Ashutosh Bapat >>>>>>>>>> EntepriseDB Corporation >>>>>>>>>> The Postgres Database Company >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>> The Database Cloud >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> StormDB - http://www.stormdb.com >>>>>>>> The Database Cloud >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> StormDB - http://www.stormdb.com >>>>>>> The Database Cloud >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> StormDB - http://www.stormdb.com >>>>> The Database Cloud >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> Best Wishes, >>> Ashutosh Bapat >>> EntepriseDB Corporation >>> The Postgres Database Company >>> >> >> >> >> -- >> StormDB - http://www.stormdb.com >> The Database Cloud >> > > > > -- > Best Wishes, > Ashutosh Bapat > EntepriseDB Corporation > The Postgres Database Company > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
|
From: Nikhil S. <ni...@st...> - 2013-08-21 18:08:18
|
> To me and to the reader of the code, it should be clear as to why or why
> not (or when or when not) to have this flag set/reset. The comments in the
> code do not say that. They just given one particular case where
> materialisation is not needed and again, we don't know why it's not needed.
>
>
I will try to add more comments. But looks like you are not reading this
mail thread at all :-)
I have mentioned multiple times on this thread:
1)
Here's the snippet from RemoteQueryNext:
if (tuplestorestate && !TupIsNull(scanslot))
tuplestore_puttupleslot(
tuplestorestate, scanslot);
I printed the current memory context inside this function, it is
""ExecutorState". This means that the tuple will stay around till the query
is executing in its entirety! For large queries this is bound to cause
issues.
2) 1 above means that we need to decide if we need the tuplestore in ALL
cases or not. The code today always materializes. We DO NOT NEED to
materialize if we are not going to rescan this RemoteQuery executor node.
3) ISTM, that the tuplestore should only be used for inner nodes of joins.
There's no need to store outer nodes in tuplestores. Also if it's a single
SELECT query, then there's no need to use tuplestore at all as well.It's
possible that there are other cases where we might not need to materialize.
But for now I have tried to optimize the above two cases. Someone who has
dabbled with this code more should try to see if it's needed in other cases
as well. As unnecessarily materializing is a huge performance penalty in
terms of RAM resource usage.
> If you have seen the code, we reduce paths and not plans. So, when I am
> reducing two paths into one RemoteQuery path, what should happen to this
> flag? How should it be used if it's set in both the paths/only one of the
> path etc.?
>
>
That's why the need to do it early on in the pathing/planning process. If a
[potentially reduced] RemoteQuery node is topmost, then it's not going to
be rescanned, so no need to materialize.
Additionally even if a [potentially reduced] RemoteQuery node is part of a
join depending on whether it's inner or outer should we materialize it.
This patch will help in general improvement in single top RemoteQuery node
plans as well as some join cases which is a good start.
Regards,
Nikhils
>
> On Wed, Aug 21, 2013 at 4:00 PM, Ashutosh Bapat <
> ash...@en...> wrote:
>
>>
>>
>>
>> On Wed, Aug 21, 2013 at 3:54 PM, Nikhil Sontakke <ni...@st...>wrote:
>>
>>>
>>> The patch needs some more comments as to why and when we should
>>>> materialise or not.
>>>>
>>>
>>> The existing code *always* materializes in all cases which can be a huge
>>> performance penalty in some cases. This patch tries to address those cases.
>>>
>>
>> There are no comments in the code as to WHY we need to re/set this flag
>> wherever set/reset.
>>
>>
>>>
>>>
>>>> Also, I am not so in favour of adding another switch all the way in
>>>> RemoteQuery path. Well, I am always not in favour of adding new switch,
>>>> since it needs to be maintained at various places in code; and if somebody
>>>> forgets to set/reset it somewhere it shows up as bug in entirely different
>>>> areas. For example, if some code forgets to set/reset this switch while
>>>> creating path, it would end up as a bug in executor or a performance
>>>> regression in the executor and would be difficult to trace it back all the
>>>> way to the path creation.
>>>>
>>>
>>> This patch sets it to true when we initialize the remote query. It's not
>>> a conditional setting. So it's always set. That ways we avoid causing havoc
>>> in other parts until unless we know about other cases where it's not needed.
>>>
>>
>> I am talking about some code added in future (and since this is very
>> happening area, we will add code in near future), where the coder or
>> reviewer has to be congnizant that this flag needs to be set/reset at some
>> places.
>>
>>>
>>>
>>>> Is there a way, we can do this based on some other members of
>>>> RemoteQuery or RemoteQueryPath structure?
>>>>
>>>>
>>> I did not find any and this follows the standard percolating information
>>> down from higher nodes to lower nodes strategy. While execution we rarely
>>> have any logic to go to the parent to check for stuff, so this is pretty
>>> consistent with existing code I would say.
>>>
>>>
>> If possible, we do use member of Plan/Parse node/s in execution code.
>> It's not a common practice to percolate the information down through every
>> structure that you come across.
>>
>>
>>> Regards,
>>> Nikhils
>>>
>>>
>>>
>>>>
>>>> On Fri, Aug 16, 2013 at 5:57 PM, Nikhil Sontakke <ni...@st...>wrote:
>>>>
>>>>> Duh.. Patch attached :^)
>>>>>
>>>>>
>>>>> On Fri, Aug 16, 2013 at 5:50 PM, Nikhil Sontakke <ni...@st...>wrote:
>>>>>
>>>>>>
>>>>>> Additionally, ISTM, that the tuplestore should only be used for inner
>>>>>>> nodes. There's no need to store outer nodes in tuplestores. Also if it's a
>>>>>>> single SELECT query, then there's no need to use tuplestore at all as well.
>>>>>>>
>>>>>>>
>>>>>> PFA, a patch which tries to avoid using the tuplestore in the above
>>>>>> two cases. During planning we decide if a tuplestore should be used for the
>>>>>> RemoteQuery. The default is true, and we set it to false for the above two
>>>>>> cases for now.
>>>>>>
>>>>>> I ran regression test cases with and without the patch and got the
>>>>>> exact same set of failures (and more importantly same diffs).
>>>>>>
>>>>>> To be clear this patch is not specific to COPY TO, but it's a generic
>>>>>> change to avoid using tuplestore in certain simple scenarios thereby
>>>>>> reducing the memory footprint of the remote query execution. Note that it
>>>>>> also does not solve Hitoshi-san's COPY FROM issues. Will submit a separate
>>>>>> patch for that.
>>>>>>
>>>>>> Regards,
>>>>>> Nikhils
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Looks like if we can pass hints during plan creation as to whether
>>>>>>> the remote scan is part of a join (and is inner node) or not, then
>>>>>>> accordingly decision can be taken to materialize into the tuplestore.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Nikhils
>>>>>>>
>>>>>>> On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke <
>>>>>>> ni...@st...> wrote:
>>>>>>>
>>>>>>>> Looks like my theory was wrong, make installcheck is giving more
>>>>>>>> errors with this patch applied. Will have to look at a different solution..
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Nikhils
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke <
>>>>>>>> ni...@st...> wrote:
>>>>>>>>
>>>>>>>>> So, I looked at this code carefully and ISTM, that because of the
>>>>>>>>> way we fetch the data from the connections and return it immediately inside
>>>>>>>>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot
>>>>>>>>> is NOT required at all.
>>>>>>>>>
>>>>>>>>> So, I have removed the call to tuplestore_puttupleslot and things
>>>>>>>>> seem to be ok for me. I guess we should do a FULL test run with this patch
>>>>>>>>> just to ensure that it does not cause issues in any scenarios.
>>>>>>>>>
>>>>>>>>> A careful look by new set of eyes will help here. I think, if
>>>>>>>>> there are no issues, this plugs a major leak in the RemoteQuery code path
>>>>>>>>> which is almost always used in our case.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Nikhils
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke <
>>>>>>>>> ni...@st...> wrote:
>>>>>>>>>
>>>>>>>>>> Using a tuplestore for data coming from RemoteQuery is kinda
>>>>>>>>>> wrong and that's what has introduced this issue. Looks like just changing
>>>>>>>>>> the memory context will not work as it interferes with the other
>>>>>>>>>> functioning of the tuplestore :-|
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Nikhils
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat <
>>>>>>>>>> ash...@en...> wrote:
>>>>>>>>>>
>>>>>>>>>>> Yes, that's correct.
>>>>>>>>>>>
>>>>>>>>>>> My patch was not intended to fix this. This was added while
>>>>>>>>>>> fixing a bug for parameterised quals on RemoteQuery I think. Check commits
>>>>>>>>>>> by Amit in this area.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke <
>>>>>>>>>>> ni...@st...> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in it.
>>>>>>>>>>>>
>>>>>>>>>>>> Here's the snippet from RemoteQueryNext:
>>>>>>>>>>>>
>>>>>>>>>>>> if (tuplestorestate && !TupIsNull(scanslot))
>>>>>>>>>>>> tuplestore_puttupleslot(tuplestorestate,
>>>>>>>>>>>> scanslot);
>>>>>>>>>>>>
>>>>>>>>>>>> I printed the current memory context inside this function, it
>>>>>>>>>>>> is ""ExecutorState". This means that the tuple will stay around till the
>>>>>>>>>>>> query is executing in its entirety! For large COPY queries this is bound to
>>>>>>>>>>>> cause issues as is also reported by Hitoshi san on another thread.
>>>>>>>>>>>>
>>>>>>>>>>>> I propose that in RemoteQueryNext, before calling the
>>>>>>>>>>>> tuplestore_puttupleslot we switch into
>>>>>>>>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context.
>>>>>>>>>>>> It will get reset, when the next tuple has to be returned to the caller and
>>>>>>>>>>>> the leak will be curtailed. Thoughts?
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Nikhils
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat <
>>>>>>>>>>>> ash...@en...> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> There has been an overhaul in the planner (and corresponding
>>>>>>>>>>>>> parts of executor) in 1.1, so it would be better if they move to 1.1 after
>>>>>>>>>>>>> GA.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke <
>>>>>>>>>>>>> ni...@st...> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Ah, I see.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I was looking at REL_1_0 sources.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> There are people out there using REL_1_0 as well.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>> Nikhils
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat <
>>>>>>>>>>>>>> ash...@en...> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> It should be part of 1.1 as well. It was done to support
>>>>>>>>>>>>>>> projection out of RemoteQuery node.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke <
>>>>>>>>>>>>>>> ni...@st...> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi Ashutosh,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I guess you have changed it in pgxc head? I was looking at
>>>>>>>>>>>>>>>> 103 and 11 branches and saw this. In that even ExecRemoteQuery seems to
>>>>>>>>>>>>>>>> have an issue wherein it's not using the appropriate context.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>> Nikhils
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Sent from my iPhone
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat <
>>>>>>>>>>>>>>>> ash...@en...> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Welcome to the mess ;) and enjoy junk food.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called
>>>>>>>>>>>>>>>> in the same fashion as other Scan nodes. So, you will see ExecRemoteQuery
>>>>>>>>>>>>>>>> calling ExecScan with RemoteQueryNext as the iterator. So, I assume your
>>>>>>>>>>>>>>>> comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery
>>>>>>>>>>>>>>>> per say!
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This code needs a lot of rework, removing duplications,
>>>>>>>>>>>>>>>> using proper way of materialisation, central response handler and error
>>>>>>>>>>>>>>>> handler etc. If we clean up this code, some improvements in planner (like
>>>>>>>>>>>>>>>> using MergeAppend plan) for Sort, will be possible. Regarding
>>>>>>>>>>>>>>>> materialisation, the code uses a linked list for materialising the rows
>>>>>>>>>>>>>>>> from datanodes (in case the same connection needs to be given to other
>>>>>>>>>>>>>>>> remote query node), which must be eating a lot of performance. Instead we
>>>>>>>>>>>>>>>> should be using some kind of tuplestore there. We actually use tuplestore
>>>>>>>>>>>>>>>> (as well) in the RemoteQuery node; the same method can be used.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke <
>>>>>>>>>>>>>>>> ni...@st...> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Have a Query about ExecRemoteQuery.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The logic seems to have been modeled after ExecMaterial.
>>>>>>>>>>>>>>>>> ISTM, that it should have been modeled after ExecScan because we fetch
>>>>>>>>>>>>>>>>> tuples, and those which match the qual should be sent up. ExecMaterial is
>>>>>>>>>>>>>>>>> for materializing and collecting and storing tuples.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Can anyone explain? The reason for asking this is I am
>>>>>>>>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any
>>>>>>>>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one
>>>>>>>>>>>>>>>>> which qualifies.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>>>> Nikhils
>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>> StormDB - http://www.stormdb.com
>>>>>>>>>>>>>>>>> The Database Cloud
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics
>>>>>>>>>>>>>>>>> Lite!
>>>>>>>>>>>>>>>>> It's a free troubleshooting tool designed for production.
>>>>>>>>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2%
>>>>>>>>>>>>>>>>> overhead.
>>>>>>>>>>>>>>>>> Download for free and get started troubleshooting in
>>>>>>>>>>>>>>>>> minutes.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
>>>>>>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>>>>>>> Postgres-xc-developers mailing list
>>>>>>>>>>>>>>>>> Pos...@li...
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>> Best Wishes,
>>>>>>>>>>>>>>>> Ashutosh Bapat
>>>>>>>>>>>>>>>> EntepriseDB Corporation
>>>>>>>>>>>>>>>> The Postgres Database Company
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> Best Wishes,
>>>>>>>>>>>>>>> Ashutosh Bapat
>>>>>>>>>>>>>>> EntepriseDB Corporation
>>>>>>>>>>>>>>> The Postgres Database Company
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> StormDB - http://www.stormdb.com
>>>>>>>>>>>>>> The Database Cloud
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Best Wishes,
>>>>>>>>>>>>> Ashutosh Bapat
>>>>>>>>>>>>> EntepriseDB Corporation
>>>>>>>>>>>>> The Postgres Database Company
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> StormDB - http://www.stormdb.com
>>>>>>>>>>>> The Database Cloud
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Best Wishes,
>>>>>>>>>>> Ashutosh Bapat
>>>>>>>>>>> EntepriseDB Corporation
>>>>>>>>>>> The Postgres Database Company
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> StormDB - http://www.stormdb.com
>>>>>>>>>> The Database Cloud
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> StormDB - http://www.stormdb.com
>>>>>>>>> The Database Cloud
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> StormDB - http://www.stormdb.com
>>>>>>>> The Database Cloud
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> StormDB - http://www.stormdb.com
>>>>>>> The Database Cloud
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> StormDB - http://www.stormdb.com
>>>>>> The Database Cloud
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> StormDB - http://www.stormdb.com
>>>>> The Database Cloud
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> Best Wishes,
>>>> Ashutosh Bapat
>>>> EntepriseDB Corporation
>>>> The Postgres Database Company
>>>>
>>>
>>>
>>>
>>> --
>>> StormDB - http://www.stormdb.com
>>> The Database Cloud
>>>
>>
>>
>>
>> --
>> Best Wishes,
>> Ashutosh Bapat
>> EntepriseDB Corporation
>> The Postgres Database Company
>>
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EntepriseDB Corporation
> The Postgres Database Company
>
--
StormDB - http://www.stormdb.com
The Database Cloud
|
|
From: Ashutosh B. <ash...@en...> - 2013-08-22 04:10:11
|
On Wed, Aug 21, 2013 at 11:37 PM, Nikhil Sontakke <ni...@st...>wrote: > > > >> To me and to the reader of the code, it should be clear as to why or why >> not (or when or when not) to have this flag set/reset. The comments in the >> code do not say that. They just given one particular case where >> materialisation is not needed and again, we don't know why it's not needed. >> >> > I will try to add more comments. But looks like you are not reading this > mail thread at all :-) > Whether or not I have read this mail thread, the reader of the code is not going to read it. So it's important to have comments in the code. > > I have mentioned multiple times on this thread: > > 1) > Here's the snippet from RemoteQueryNext: > > if (tuplestorestate && !TupIsNull(scanslot)) > tuplestore_puttupleslot( > tuplestorestate, scanslot); > > I printed the current memory context inside this function, it is > ""ExecutorState". This means that the tuple will stay around till the query > is executing in its entirety! For large queries this is bound to cause > issues. > > 2) 1 above means that we need to decide if we need the tuplestore in ALL > cases or not. The code today always materializes. We DO NOT NEED to > materialize if we are not going to rescan this RemoteQuery executor node. > > 3) ISTM, that the tuplestore should only be used for inner nodes of joins. > There's no need to store outer nodes in tuplestores. Also if it's a single > SELECT query, then there's no need to use tuplestore at all as well.It's > possible that there are other cases where we might not need to materialize. > But for now I have tried to optimize the above two cases. Someone who has > dabbled with this code more should try to see if it's needed in other cases > as well. As unnecessarily materializing is a huge performance penalty in > terms of RAM resource usage. > > >> If you have seen the code, we reduce paths and not plans. So, when I am >> reducing two paths into one RemoteQuery path, what should happen to this >> flag? How should it be used if it's set in both the paths/only one of the >> path etc.? >> >> > That's why the need to do it early on in the pathing/planning process. If > a [potentially reduced] RemoteQuery node is topmost, then it's not going to > be rescanned, so no need to materialize. > > That's not always true. In case of cursors (esp. random access cursors) even topmost RemoteQuery node needs a rescan. > Additionally even if a [potentially reduced] RemoteQuery node is part of a > join depending on whether it's inner or outer should we materialize it. > > This patch will help in general improvement in single top RemoteQuery node > plans as well as some join cases which is a good start. > > Regards, > Nikhils > > > >> >> On Wed, Aug 21, 2013 at 4:00 PM, Ashutosh Bapat < >> ash...@en...> wrote: >> >>> >>> >>> >>> On Wed, Aug 21, 2013 at 3:54 PM, Nikhil Sontakke <ni...@st...>wrote: >>> >>>> >>>> The patch needs some more comments as to why and when we should >>>>> materialise or not. >>>>> >>>> >>>> The existing code *always* materializes in all cases which can be a >>>> huge performance penalty in some cases. This patch tries to address those >>>> cases. >>>> >>> >>> There are no comments in the code as to WHY we need to re/set this flag >>> wherever set/reset. >>> >>> >>>> >>>> >>>>> Also, I am not so in favour of adding another switch all the way in >>>>> RemoteQuery path. Well, I am always not in favour of adding new switch, >>>>> since it needs to be maintained at various places in code; and if somebody >>>>> forgets to set/reset it somewhere it shows up as bug in entirely different >>>>> areas. For example, if some code forgets to set/reset this switch while >>>>> creating path, it would end up as a bug in executor or a performance >>>>> regression in the executor and would be difficult to trace it back all the >>>>> way to the path creation. >>>>> >>>> >>>> This patch sets it to true when we initialize the remote query. It's >>>> not a conditional setting. So it's always set. That ways we avoid causing >>>> havoc in other parts until unless we know about other cases where it's not >>>> needed. >>>> >>> >>> I am talking about some code added in future (and since this is very >>> happening area, we will add code in near future), where the coder or >>> reviewer has to be congnizant that this flag needs to be set/reset at some >>> places. >>> >>>> >>>> >>>>> Is there a way, we can do this based on some other members of >>>>> RemoteQuery or RemoteQueryPath structure? >>>>> >>>>> >>>> I did not find any and this follows the standard percolating >>>> information down from higher nodes to lower nodes strategy. While execution >>>> we rarely have any logic to go to the parent to check for stuff, so this is >>>> pretty consistent with existing code I would say. >>>> >>>> >>> If possible, we do use member of Plan/Parse node/s in execution code. >>> It's not a common practice to percolate the information down through every >>> structure that you come across. >>> >>> >>>> Regards, >>>> Nikhils >>>> >>>> >>>> >>>>> >>>>> On Fri, Aug 16, 2013 at 5:57 PM, Nikhil Sontakke <ni...@st...>wrote: >>>>> >>>>>> Duh.. Patch attached :^) >>>>>> >>>>>> >>>>>> On Fri, Aug 16, 2013 at 5:50 PM, Nikhil Sontakke <ni...@st... >>>>>> > wrote: >>>>>> >>>>>>> >>>>>>> Additionally, ISTM, that the tuplestore should only be used for >>>>>>>> inner nodes. There's no need to store outer nodes in tuplestores. Also if >>>>>>>> it's a single SELECT query, then there's no need to use tuplestore at all >>>>>>>> as well. >>>>>>>> >>>>>>>> >>>>>>> PFA, a patch which tries to avoid using the tuplestore in the above >>>>>>> two cases. During planning we decide if a tuplestore should be used for the >>>>>>> RemoteQuery. The default is true, and we set it to false for the above two >>>>>>> cases for now. >>>>>>> >>>>>>> I ran regression test cases with and without the patch and got the >>>>>>> exact same set of failures (and more importantly same diffs). >>>>>>> >>>>>>> To be clear this patch is not specific to COPY TO, but it's a >>>>>>> generic change to avoid using tuplestore in certain simple scenarios >>>>>>> thereby reducing the memory footprint of the remote query execution. Note >>>>>>> that it also does not solve Hitoshi-san's COPY FROM issues. Will submit a >>>>>>> separate patch for that. >>>>>>> >>>>>>> Regards, >>>>>>> Nikhils >>>>>>> >>>>>>> >>>>>>> >>>>>>>> Looks like if we can pass hints during plan creation as to whether >>>>>>>> the remote scan is part of a join (and is inner node) or not, then >>>>>>>> accordingly decision can be taken to materialize into the tuplestore. >>>>>>>> >>>>>>>> Regards, >>>>>>>> Nikhils >>>>>>>> >>>>>>>> On Thu, Aug 15, 2013 at 10:43 PM, Nikhil Sontakke < >>>>>>>> ni...@st...> wrote: >>>>>>>> >>>>>>>>> Looks like my theory was wrong, make installcheck is giving more >>>>>>>>> errors with this patch applied. Will have to look at a different solution.. >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Nikhils >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thu, Aug 15, 2013 at 2:11 PM, Nikhil Sontakke < >>>>>>>>> ni...@st...> wrote: >>>>>>>>> >>>>>>>>>> So, I looked at this code carefully and ISTM, that because of the >>>>>>>>>> way we fetch the data from the connections and return it immediately inside >>>>>>>>>> RemoteQueryNext, storing it in the tuplestore using tuplestore_puttupleslot >>>>>>>>>> is NOT required at all. >>>>>>>>>> >>>>>>>>>> So, I have removed the call to tuplestore_puttupleslot and things >>>>>>>>>> seem to be ok for me. I guess we should do a FULL test run with this patch >>>>>>>>>> just to ensure that it does not cause issues in any scenarios. >>>>>>>>>> >>>>>>>>>> A careful look by new set of eyes will help here. I think, if >>>>>>>>>> there are no issues, this plugs a major leak in the RemoteQuery code path >>>>>>>>>> which is almost always used in our case. >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Nikhils >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, Aug 14, 2013 at 7:05 PM, Nikhil Sontakke < >>>>>>>>>> ni...@st...> wrote: >>>>>>>>>> >>>>>>>>>>> Using a tuplestore for data coming from RemoteQuery is kinda >>>>>>>>>>> wrong and that's what has introduced this issue. Looks like just changing >>>>>>>>>>> the memory context will not work as it interferes with the other >>>>>>>>>>> functioning of the tuplestore :-| >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Nikhils >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 14, 2013 at 3:07 PM, Ashutosh Bapat < >>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>> >>>>>>>>>>>> Yes, that's correct. >>>>>>>>>>>> >>>>>>>>>>>> My patch was not intended to fix this. This was added while >>>>>>>>>>>> fixing a bug for parameterised quals on RemoteQuery I think. Check commits >>>>>>>>>>>> by Amit in this area. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Aug 14, 2013 at 3:03 PM, Nikhil Sontakke < >>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Yeah, but AFAICS, even 1.1 (and head) *still* has a leak in >>>>>>>>>>>>> it. >>>>>>>>>>>>> >>>>>>>>>>>>> Here's the snippet from RemoteQueryNext: >>>>>>>>>>>>> >>>>>>>>>>>>> if (tuplestorestate && !TupIsNull(scanslot)) >>>>>>>>>>>>> tuplestore_puttupleslot(tuplestorestate, >>>>>>>>>>>>> scanslot); >>>>>>>>>>>>> >>>>>>>>>>>>> I printed the current memory context inside this function, it >>>>>>>>>>>>> is ""ExecutorState". This means that the tuple will stay around till the >>>>>>>>>>>>> query is executing in its entirety! For large COPY queries this is bound to >>>>>>>>>>>>> cause issues as is also reported by Hitoshi san on another thread. >>>>>>>>>>>>> >>>>>>>>>>>>> I propose that in RemoteQueryNext, before calling the >>>>>>>>>>>>> tuplestore_puttupleslot we switch into >>>>>>>>>>>>> scan_node->ps.ps_ExprContext's ecxt_per_tuple_memory context. >>>>>>>>>>>>> It will get reset, when the next tuple has to be returned to the caller and >>>>>>>>>>>>> the leak will be curtailed. Thoughts? >>>>>>>>>>>>> >>>>>>>>>>>>> Regards, >>>>>>>>>>>>> Nikhils >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Aug 14, 2013 at 11:33 AM, Ashutosh Bapat < >>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> There has been an overhaul in the planner (and corresponding >>>>>>>>>>>>>> parts of executor) in 1.1, so it would be better if they move to 1.1 after >>>>>>>>>>>>>> GA. >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Wed, Aug 14, 2013 at 10:54 AM, Nikhil Sontakke < >>>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Ah, I see. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I was looking at REL_1_0 sources. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> There are people out there using REL_1_0 as well. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Tue, Aug 13, 2013 at 9:52 AM, Ashutosh Bapat < >>>>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> It should be part of 1.1 as well. It was done to support >>>>>>>>>>>>>>>> projection out of RemoteQuery node. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On Tue, Aug 13, 2013 at 7:17 AM, Nikhil Sontakke < >>>>>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hi Ashutosh, >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I guess you have changed it in pgxc head? I was looking at >>>>>>>>>>>>>>>>> 103 and 11 branches and saw this. In that even ExecRemoteQuery seems to >>>>>>>>>>>>>>>>> have an issue wherein it's not using the appropriate context. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Sent from my iPhone >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Aug 12, 2013, at 9:54 AM, Ashutosh Bapat < >>>>>>>>>>>>>>>>> ash...@en...> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Welcome to the mess ;) and enjoy junk food. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Sometime back, I have changed ExecRemoteQuery to be called >>>>>>>>>>>>>>>>> in the same fashion as other Scan nodes. So, you will see ExecRemoteQuery >>>>>>>>>>>>>>>>> calling ExecScan with RemoteQueryNext as the iterator. So, I assume your >>>>>>>>>>>>>>>>> comment pertains to RemoteQueryNext and its minions and not ExecRemoteQuery >>>>>>>>>>>>>>>>> per say! >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> This code needs a lot of rework, removing duplications, >>>>>>>>>>>>>>>>> using proper way of materialisation, central response handler and error >>>>>>>>>>>>>>>>> handler etc. If we clean up this code, some improvements in planner (like >>>>>>>>>>>>>>>>> using MergeAppend plan) for Sort, will be possible. Regarding >>>>>>>>>>>>>>>>> materialisation, the code uses a linked list for materialising the rows >>>>>>>>>>>>>>>>> from datanodes (in case the same connection needs to be given to other >>>>>>>>>>>>>>>>> remote query node), which must be eating a lot of performance. Instead we >>>>>>>>>>>>>>>>> should be using some kind of tuplestore there. We actually use tuplestore >>>>>>>>>>>>>>>>> (as well) in the RemoteQuery node; the same method can be used. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Sat, Aug 10, 2013 at 10:48 PM, Nikhil Sontakke < >>>>>>>>>>>>>>>>> ni...@st...> wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Have a Query about ExecRemoteQuery. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> The logic seems to have been modeled after ExecMaterial. >>>>>>>>>>>>>>>>>> ISTM, that it should have been modeled after ExecScan because we fetch >>>>>>>>>>>>>>>>>> tuples, and those which match the qual should be sent up. ExecMaterial is >>>>>>>>>>>>>>>>>> for materializing and collecting and storing tuples. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Can anyone explain? The reason for asking this is I am >>>>>>>>>>>>>>>>>> suspecting a big memory leak in this code path. We are not using any >>>>>>>>>>>>>>>>>> expression context nor we are freeing up tuples as we scan for the one >>>>>>>>>>>>>>>>>> which qualifies. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>>>> Nikhils >>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> ------------------------------------------------------------------------------ >>>>>>>>>>>>>>>>>> Get 100% visibility into Java/.NET code with AppDynamics >>>>>>>>>>>>>>>>>> Lite! >>>>>>>>>>>>>>>>>> It's a free troubleshooting tool designed for production. >>>>>>>>>>>>>>>>>> Get down to code-level detail for bottlenecks, with <2% >>>>>>>>>>>>>>>>>> overhead. >>>>>>>>>>>>>>>>>> Download for free and get started troubleshooting in >>>>>>>>>>>>>>>>>> minutes. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk >>>>>>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>>>>>> Postgres-xc-developers mailing list >>>>>>>>>>>>>>>>>> Pos...@li... >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> Best Wishes, >>>>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>>>> The Database Cloud >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Best Wishes, >>>>>>>>>>>> Ashutosh Bapat >>>>>>>>>>>> EntepriseDB Corporation >>>>>>>>>>>> The Postgres Database Company >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>>> The Database Cloud >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>>> The Database Cloud >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> StormDB - http://www.stormdb.com >>>>>>>>> The Database Cloud >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> StormDB - http://www.stormdb.com >>>>>>>> The Database Cloud >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> StormDB - http://www.stormdb.com >>>>>>> The Database Cloud >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> StormDB - http://www.stormdb.com >>>>>> The Database Cloud >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Best Wishes, >>>>> Ashutosh Bapat >>>>> EntepriseDB Corporation >>>>> The Postgres Database Company >>>>> >>>> >>>> >>>> >>>> -- >>>> StormDB - http://www.stormdb.com >>>> The Database Cloud >>>> >>> >>> >>> >>> -- >>> Best Wishes, >>> Ashutosh Bapat >>> EntepriseDB Corporation >>> The Postgres Database Company >>> >> >> >> >> -- >> Best Wishes, >> Ashutosh Bapat >> EntepriseDB Corporation >> The Postgres Database Company >> > > > > -- > StormDB - http://www.stormdb.com > The Database Cloud > -- Best Wishes, Ashutosh Bapat EntepriseDB Corporation The Postgres Database Company |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X