Re: [Postgres-xc-developers] ExecRemoteQuery

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

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