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

Patch applied.  Thanks.


> 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

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026