Re: High Memory Usage Patch -- Disregard my last message - Mailing list pgsql-jdbc

Bruce,

Here is the patch.  When I sent it originally I send from the wrong
email account so it didn't end up getting to the 'patches' list.

thanks,
--Barry

(file patch.txt attached)





Bruce Momjian wrote:

> Can someone send me the patch?  I haven't seen any patch from Barry.
> How am I missing it?
>
>
>
>>Barry,
>>
>>Your patch is correct, and thread-safe in the light of the next day and
>>a good night's sleep.
>>
>>Cheers,
>>
>>Dave
>>
>>Barry,
>>
>>My understanding of the problem is as follows:
>>
>>The if (anyvar == null) check is flawed in an SMP environment; according
>>to the spec anyvar can be in the process of being created the jvm could
>>set anyvar to point to the memory it is going to assign it to but not
>>complete the creation of the object. You end up with a non-null, but
>>invalid anyvar. This is the crux of the double locking flaw. While I
>>hope most compiler writers would be smarter than that, apparently it is
>>possible according to the spec.
>>
>>Well if we make the driver completely thread-safe we are going to take a
>>real performance hit. Personally I would prefer to code assuming it is
>>not completely thread-safe and have a lightweight driver.
>>
>>I am willing to take on some of the work for the driver, but I think the
>>process s/b a group process. I have learned a lot in this particular
>>thread.
>>As I already mentioned I would like to see a voting procedure like the
>>apache group.
>>
>>Regarding the catch-22 with Blob, etc. I think we need to make a harsh
>>decision here. Either break the existing code, or create another driver
>>codebase. If we don't do something we will be doomed to non-compliance.
>>This will hurt the driver in the not too distant future. There are a lot
>>of tools out there which the driver needs to be compatible with.
>>
>>I had a look at the updateable cursors and it looks possible. Do you
>>know who is working on it?
>>
>>Dave
>>
>>
>>
>>-----Original Message-----
>>From: Barry Lind [mailto:barry@xythos.com]
>>Sent: June 25, 2001 11:39 AM
>>To: Dave@micro-automation.net
>>Cc: 'PostgreSQL jdbc list'
>>Subject: Re: [ADMIN] High memory usage [PATCH]
>>
>>Dave,
>>
>>The patch I submitted is thread safe.  The "if (df==null)" check is
>>dealing with a ThreadLocal variable.  By definition a ThreadLocal
>>variable can't be used by any other thread since each thread has its own
>>
>>copy. (Unless the code goes and hands the object off to some other
>>thread thus causing threading issues, which it doesn't in this case).
>>Thus I believe the patch I submitted is perfectly thread safe.
>>
>>To your more general questions:
>>
>>I think thread safety is very important for all of the JDBC objects.
>>There are no restrictions placed on what a user could do with these
>>objects.  It is perfectly legal to create a Statement in one thread,
>>hand that statement off to two or more other threads to access.  Now I
>>wouldn't recommend doing this, but the spec permits it.
>>
>>As far as procedures and voting, I also believe that something needs to
>>be done for the JDBC code base.  Since Peter has apparently disappeared
>>(I haven't seen a post from him in about two months, and the
>>jdbc.postgresql.org website hasn't been updated for about 4 mounths - it
>>
>>doesn't even have the 7.1 code yet) I think the core group needs to step
>>
>>in and provide some direction for the JDBC code base.  Whether that is
>>appointing someone new as the official/unofficial JDBC guru or adopting
>>some other process, I don't know.  What I do know is that the JDBC code
>>base is suffering from lack of attention and direction.
>>
>>Issues that I am concerned about in the JDBC code base are:
>>   - get/setXXXStream methods are not really spec compliant
>>   - the 'bytea' datatype (i.e. binary) isn't supported, and can't be
>>without backward compatibility problems because the get/setBytes methods
>>
>>currently assume that binary means BLOB.
>>   - performance/performance/performance - lots of work could/should be
>>done to improve performance.  Some good work was started last year but
>>nothing came of it.
>>   - updateable cursors - I beleive some work is being done here by
>>various parties on the list, but I have some serious concerns about
>>if/how such functionality can/should be supported.
>>
>>thanks,
>>--Barry
>>
>>
>>Dave Cramer wrote:
>>
>>
>>>Barry,
>>>
>>>My patch was attempting to maintain some sort of thread safety. I do
>>>
>>not
>>
>>>think the if (df==null) test is thread-safe. It would need to be
>>>synchronized.
>>>
>>>However, as I have mentioned in a couple of previous posts; I'm not
>>>
>>sure
>>
>>>thread-safety is worthwhile. The driver appears to be thread safe in
>>>that multiple threads can each use different instances of connections,
>>>and statements, and resultset's however I don't think it is thread
>>>
>>safe
>>
>>>in the sense that multiple threads could use the same instance of the
>>>above objects. The JDBC guide suggests that the driver s/b threadsafe
>>>
>>in
>>
>>>this sense (multiple threads....same object). The guide suggests that
>>>one thread may instigate the retrieval of a result set, and another
>>>would display it.
>>>
>>>
>>>Where this is leading is: What kind of thread safety are we trying to
>>>achieve?
>>>
>>>If it's only one instance per thread then, I would have to agree that
>>>Barry's patch s/b applied.
>>>
>>>P.S.
>>>
>>>Is there a formal process within the postgres group for making these
>>>kind of decisions.
>>>
>>>If not I would like to suggest adopting the apache groups +1,-1 voting
>>>approach.
>>>
>>>-----Original Message-----
>>>From: Barry Lind [mailto:blind@xythos.com]
>>>Sent: June 25, 2001 12:37 AM
>>>To: pgsql-patches@postgresql.org
>>>Cc: Dave@micro-automation.net; 'PostgreSQL jdbc list'
>>>Subject: Re: [ADMIN] High memory usage [PATCH]
>>>
>>>Since this patch got applied before I got around to commenting on it,
>>>
>>I
>>
>>>have submitted a new patch to address my issues with the original
>>>
>>patch.
>>
>>>The problem with the patch as applied is that is always creates the
>>>SimpleDateFormat objects in the constructor of the PreparedStatement
>>>regardless of whether or not they will ever be used in the
>>>PreparedStatement.  I have reverted back to the old behavior that only
>>>
>>>creates them if necessary in the setDate and setTimestamp methods.
>>>
>>>I also removed the close() method.  It's only purpose was to free
>>>
>>these
>>
>>>two SimpleDateFormat objects.  I think it is much better to leave
>>>
>>these
>>
>>>two objects cached on the thread so that other PreparedStatements can
>>>use them.  (This was the intention of a patch I submitted back in
>>>February where I was trying to remove as many object creations as
>>>possible to improve performance.  That patch as written needed to get
>>>pulled because of the problem that SimpleDataFormat objects are not
>>>thread safe.  Peter then added the ThreadLocal code to try to solve
>>>
>>the
>>
>>>performance problem, but introduced the memory leak that originated
>>>
>>this
>>
>>>email thread.)  I think the cost of at most two SimpleDateFormat
>>>
>>objects
>>
>>>being cached on each thead is worth the benefits of less object
>>>
>>creation
>>
>>>and subsequent garbage collection.
>>>
>>>thanks,
>>>--Barry
>>>
>>>
>>>Bruce Momjian wrote:
>>>
>>>
>>>
>>>>Patch applied.  Thanks.
>>>>
>>>>
>>>>
>>>>
>>>>>Here is a patch which inspired by Michael Stephens that should work
>>>>>
>>>>>Dave
>>>>>
>>>>>
>>>>>-----Original Message-----
>>>>>From: pgsql-jdbc-owner@postgresql.org
>>>>>[mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar R?nning
>>>>>Sent: June 22, 2001 10:14 AM
>>>>>To: Rainer Mager
>>>>>Cc: Dave Cramer; Bruce Momjian; PostgreSQL jdbc list
>>>>>Subject: Re: [JDBC] Re: [ADMIN] High memory usage [PATCH]
>>>>>
>>>>>* "Rainer Mager" <rmager@vgkk.com> wrote:
>>>>>|
>>>>>
>>>>>| Interesting. I wasn't aware of this. If question #2 is answered
>>>>>
>>such
>>
>>>>>that
>>>>>| thread safe isn't necessary, then this problem goes away pretty
>>>>>easily. If
>>>>>| thread safety is needed then this would have to be rewritten, I can
>>>>>look
>>>>>| into doing this if you like.
>>>>>
>>>>>Thread safety is required by the spec. Do you have "JDBC API tutorial
>>>>>and
>>>>>reference, 2 ed." from Addison Wesley ? This book contains a section
>>>>>
>>>>>
>>>for
>>>
>>>
>>>>>JDBC driver writers and explains this issue.
>>>>>
>>>>>regards,
>>>>>
>>>>>      Gunnar
>>>>>
>>>>>--
>>>>>Gunnar R?nning - gunnar@polygnosis.com
>>>>>Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
>>>>>
>>>>>---------------------------(end of
>>>>>
>>>>>
>>>broadcast)---------------------------
>>>
>>>
>>>>>TIP 1: subscribe and unsubscribe commands go to
>>>>>
>>>>>
>>>majordomo@postgresql.org
>>>
>>>
>>>>[ Attachment, skipping... ]
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>>---------------------------(end of broadcast)---------------------------
>>TIP 2: you can get off all lists at once with the unregister command
>>    (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
>>
>>
>>
>>---------------------------(end of broadcast)---------------------------
>>TIP 4: Don't 'kill -9' the postmaster
>>
>>
>

*** ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java.orig    Sun Jun 24 21:05:29 2001
--- ./interfaces/jdbc/org/postgresql/jdbc2/PreparedStatement.java    Sun Jun 24 21:13:15 2001
***************
*** 65,78 ****
          this.sql = sql;
          this.connection = connection;

-         // might just as well create it here, so we don't take the hit later
-
-                 SimpleDateFormat df = new SimpleDateFormat("''yyyy-MM-dd''");
-                 tl_df.set(df);
-
-                 df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
-                 tl_tsdf.set(df);
-
          for (i = 0; i < sql.length(); ++i)
          {
              int c = sql.charAt(i);
--- 65,70 ----
***************
*** 95,111 ****
              templateStrings[i] = (String)v.elementAt(i);
      }

-         /**
-          * New in 7.1 - overides Statement.close() to dispose of a few local objects
-          */
-         public void close() throws SQLException
-      {
-           // free the ThreadLocal caches
-           tl_df.set(null);
-       tl_tsdf.set(null);
-           super.close();
-         }
-
      /**
       * A Prepared SQL query is executed and its ResultSet is returned
       *
--- 87,92 ----
***************
*** 343,348 ****
--- 324,333 ----
      public void setDate(int parameterIndex, java.sql.Date x) throws SQLException
      {
            SimpleDateFormat df = (SimpleDateFormat) tl_df.get();
+           if(df==null) {
+             df = new SimpleDateFormat("''yyyy-MM-dd''");
+             tl_df.set(df);
+           }

        set(parameterIndex, df.format(x));

***************
*** 382,387 ****
--- 367,376 ----
      public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException
          {
            SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get();
+           if(df==null) {
+             df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+             tl_tsdf.set(df);
+           }
            df.setTimeZone(TimeZone.getTimeZone("GMT"));

            // Use the shared StringBuffer