Bug 341329

Comment 1 Brian Fitzpatrick 2011-03-30 09:41:56 EDT

So far as I can recall, much of the SQL functionality really wasn't meant for huge data sets. But even more interesting, I don't recall there being a "Return All Rows" menu anywhere. Are you sure this is DTP or an IBM product?

Comment 2 Brian Payton 2011-03-30 12:56:53 EDT

Hi Fitz, you are correct, this is an IBM extension.  I'll follow up with the Balaji.

Comment 3 Balaji Kadambi 2011-04-01 09:15:32 EDT

Brian, The out of memory exception occurred in an IBM product built over the DTP code. But the reason for memory leak was in DTP code. I will be attaching a patch shortly. Please let me know if I need to modify the description.

Thanks,
Balaji
(In reply to comment #1)
> So far as I can recall, much of the SQL functionality really wasn't meant for
> huge data sets. But even more interesting, I don't recall there being a "Return
> All Rows" menu anywhere. Are you sure this is DTP or an IBM product?

Comment 4 Balaji Kadambi 2011-04-01 09:27:56 EDT

Created attachment 192357 [details]
Patch for the bug

There was strong reference to the result objects after the remove all finished action in ResultSetViewerRegistryProvider. The viewer providers held a reference to the resuilt objects that has been removed.

There is a weak reference to the ResultSetObject in the jdk ObjectStreamClass. To minimize memory usage after the remove all finished action, the results in the ResultSetObject has been cleared.

Comment 5 Balaji Kadambi 2011-04-21 03:54:11 EDT

Created attachment 193784 [details]
Patch for bug 341329.

Comment 6 Balaji Kadambi 2011-09-27 05:07:44 EDT

Brian,  

Do you have an update as to when this fix will be done? This fix is important as it fixes a memory leak.

Thanks,
Balaji

Comment 7 Brian Payton 2011-09-27 12:08:09 EDT

Hi Balaji,

Is it guaranteed that a ResultSetViewerDescriptor has a ExternalResultSetViewerProvider?  That is, do we need a null check here in removeViewerProviderConstants?

  ResultSetViewerDescriptor desc = (ResultSetViewerDescriptor) viewerList.get(i);
  ExternalResultSetViewerProvider provider = desc.getViewerProvider();
  provider.setResult(null);
  provider.setResultInstance(null);

Comment 8 Balaji Kadambi 2011-09-28 13:22:07 EDT

In the scenario that I saw the ResultSetViewerDescriptor always has a  ExternalResultSetViewerPovider when removeViewerProviderConstants is called from RemoveAllVisibleFinishedResultAction class.

It would be safe to add a null check here even otherwise.

Do you need another patch from me with the null check. Please let me know.

Thanks,
Balaji

(In reply to comment #7)
> Hi Balaji,
> Is it guaranteed that a ResultSetViewerDescriptor has a
> ExternalResultSetViewerProvider?  That is, do we need a null check here in
> removeViewerProviderConstants?
>   ResultSetViewerDescriptor desc = (ResultSetViewerDescriptor)
> viewerList.get(i);
>   ExternalResultSetViewerProvider provider = desc.getViewerProvider();
>   provider.setResult(null);
>   provider.setResultInstance(null);

Comment 9 Brian Payton 2011-09-28 15:15:36 EDT

No, I can make the change.

Comment 10 Brian Payton 2011-09-30 18:53:05 EDT

Here's the Git commit link for the original patch:
http://git.eclipse.org/c/datatools/org.eclipse.datatools.sqltools.git/commit/?id=3e43d03e7d10d3f87e0003939759b86ff4489ee5

And the commit link for the addition of the null check:
http://git.eclipse.org/c/datatools/org.eclipse.datatools.sqltools.git/commit/?id=62390f577d82096b8d03a1971253f937c67b30f9

Comment 11 Balaji Kadambi 2011-10-03 12:29:43 EDT

Thanks Brian. Which version of DTP will actually have this fix?

Regards,
Balaji

(In reply to comment #10)
> Here's the Git commit link for the original patch:
> http://git.eclipse.org/c/datatools/org.eclipse.datatools.sqltools.git/commit/?id=3e43d03e7d10d3f87e0003939759b86ff4489ee5
> And the commit link for the addition of the null check:
> http://git.eclipse.org/c/datatools/org.eclipse.datatools.sqltools.git/commit/?id=62390f577d82096b8d03a1971253f937c67b30f9

Comment 12 Brian Payton 2011-10-03 19:53:16 EDT

It is included in the next maintenance release of DTP, v1.9.2, which goes GA on Feb 21, 2012.