Re: non-trivial finalize() on AbstractJdbc2Statement - Mailing list pgsql-jdbc

Hello.

The finalizer thread is not a low priority thread in my JVM (openjdk) and as soon as Statement.close does nothing (and it does nothing if statement is closed), I don't see how can it be a problem to free memory. Actually statement creation is much more heavy thing than "if (flag) return". It's perfectly valid to close native resource (and as far as I know statement allocates server-side native resources) in finalizer. Another implementation can be to create a ReferenceQueue and a bunch of References that should be checked now and then during connection calls, but this introduces additional processing that is not needed most time. You can't drop this code altogether as while it's good practice to close statements in client code, you can't guarantee it's done everywhere. Actually it's common practice in java to clean-up external resources in finalize, see for example FileOutputStream.
As of your problem, I don't thing statement finalize method is the problem cause. I can see two causes:
1) The code bug mentioned in this thread
2) Some another class with long finalize method. This is the case that can lead to confusion. Say, you have an object created once in 5 minutes that has 3 minutes finalize. It won't hurt by itself as it's enough time to be finalized. But as soon as you have additional object with finalized method defined (say, Statement), even empty, all this objects can't be freed during this 3 minutes run and you will get OOM. And you will rarely blame your custom object. The best check that can be done to show real reason is to check Finalizer object stack with jstack. I did often see cases when custom finalize method is fast, but is synchronized by some heavy-used  resource.

Best regards, Vitalii Tymchyshyn


15.02.12 13:11, Imran написав(ла):

Hi Guys

I dont think I'm referring to the speed or workings of the finalize() method available on AbstractJdbc2Statement. As said earlier I agree that it should return quickly if statements are closed by clients and also having safeguards around it looks nice. But my point is that by having a non-trivial finalize() an extra JdbcxStatement object is been created and referenced through a Finalizer, which is in a reference queue that would be picked up by the Finalizer thread. And a GC wouldn't reclaim the memory until the finalizer thread has done its work. As we have many queries executing, the main thread keeps filling this queue at a much faster rate than the finalizer thread (which is a low priority thread by default) would get to it. Under heavy load this paves the way for possible OOM issues as we've noticed. Also just to check this, we've patched up the driver we have without the finalize() method and we've seen much better GC and have not experienced OOM on similar load that resulted in OOMs. 

The general feeling as I've learned over the years is that finalize() should be used cautiously. So I was wondering if its really required for the AbstractJdbc2Statement to have a finalize() that only closes the statement, which one would expect the client to do anyway.

Cheers

-- Imran

On Mon, Feb 13, 2012 at 4:49 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote:

******* ********<tivv00@gmail.com> wrote:
> Looking at the code... Can it be because
> org.postgresql.jdbc2.AbstractJdbc2Statement#isClosed is not
> volatile? There is no synchronization and finalizer thread may
> simply not see statement was just closed by another thread.

That sounds likely enough to me.  I don't know whether declaring the
flag volatile would be enough, but it needs either that or access
only through synchronized blocks.

In addition, I would recommend something like the attached to make
the code more bullet-proof.

-Kevin