Re: [Postgres-xc-developers] ExecRemoteQuery

[Nikhil S. <ni...@st...>]

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