Re: [PATCHES] Re: High Memory Usage Patch -- Disregard my last message - Mailing list pgsql-jdbc
From | Bruce Momjian |
---|---|
Subject | Re: [PATCHES] Re: High Memory Usage Patch -- Disregard my last message |
Date | |
Msg-id | 200106291721.f5THLwn02654@candle.pha.pa.us Whole thread Raw |
In response to | Re: High Memory Usage Patch -- Disregard my last message (Barry Lind <barry@xythos.com>) |
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
pgsql-jdbc by date: