Thread: RE: [ADMIN] High memory usage [PATCH]
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... ] > >
Barry, let me back out Dave's change until we can all agree on it, OK? It is easy to do. > 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 1: subscribe and unsubscribe commands go to majordomo@postgresql.org > -- 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
Bruce, I don't think you need to back out Dave's patch. It works fine as it is. I just think it can be improved upon. thanks, --Barry Bruce Momjian wrote: > Barry, let me back out Dave's change until we can all agree on it, OK? > It is easy to do. > > >>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 1: subscribe and unsubscribe commands go to majordomo@postgresql.org >> >> >
> Bruce, > > I don't think you need to back out Dave's patch. It works fine as it > is. I just think it can be improved upon. OK, you just let me know. -- 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
Bruce, Barry, How about checking for the existence of the date object in the constructor? This would mean that it would only be created once per thread. Dave -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Bruce Momjian Sent: June 25, 2001 2:50 PM To: Barry Lind Cc: Dave@micro-automation.net; 'PostgreSQL jdbc list' Subject: Re: [JDBC] RE: [ADMIN] High memory usage [PATCH] > Bruce, > > I don't think you need to back out Dave's patch. It works fine as it > is. I just think it can be improved upon. OK, you just let me know. -- 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 ---------------------------(end of broadcast)--------------------------- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
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... ] >>> >>> >>> >> >> > >
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... ] >> >> >> > >
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... ] >> >> >> > >
> 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. I am not keen on a hard-wired voting process because it makes winners and losers. I vote only as a last resort. I usually give patches two days and if there is no comment and it looks good, I apply it. If anyone objects before or after it is applied, it gets reversed out and we discuss it. -- 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
> 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. See my earlier post. We don't have official maintainers of the main code and I don't see a need for one in the interfaces. We can discuss issues just like we are doing now. I most cases groups get better results and single maintainers. You guys are the official maintainers now. :-) -- 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
> 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. Let me add I have been concerned about the jdbc interface for the past year. With you people involved, I am no longer concerned. -- 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
On Mon, Jun 25, 2001 at 09:32:38PM -0400, Bruce Momjian wrote: > > 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. > > Let me add I have been concerned about the jdbc interface for the past > year. With you people involved, I am no longer concerned. Hi, I've only been reading this list for about a week or so now. Joined it to find out what was happening with this piece of s/w that I was relying on. I think there is need to push on a bit since as remarked above it seems that Peter has disappeared. I think a main step is to find out where we are on the driver, for example its very hard to find out what isn't implemented yet without reading the code or trying it out. I haven't found a todo list. A great place to keep all this is on the web too which needs an update badly. Who has access to update these pages? I don't care about tutorials or stuff like that yet, but just info on known bugs, what works and what doesn't, the recommended versions to run for each database etc and somesort of changelog. I'm willing to help :) Cheers, Tom. -- Thomas O'Dowd. - Nooping - http://nooper.com tom@nooper.com - Testing - http://nooper.co.jp/labs
> > Let me add I have been concerned about the jdbc interface for the past > > year. With you people involved, I am no longer concerned. > > Hi, I've only been reading this list for about a week or so now. Joined > it to find out what was happening with this piece of s/w that I was > relying on. I think there is need to push on a bit since as remarked above > it seems that Peter has disappeared. I think a main step is to find out > where we are on the driver, for example its very hard to find out what > isn't implemented yet without reading the code or trying it out. I > haven't found a todo list. A great place to keep all this is on the web > too which needs an update badly. Who has access to update these pages? I > don't care about tutorials or stuff like that yet, but just info on > known bugs, what works and what doesn't, the recommended versions to > run for each database etc and somesort of changelog. I'm willing to help :) Good point. I don't understand JDBC well enough to maintain a TODO list. If people want I can create one and keep it current but people are going to have to give me items to add. I can keep it in the CVS tree and have a web page that links to it with a nice pretty HTML display. That is how I do the main TODO list, or we can just add a JDBC section to the main TODO list. That would work too. -- 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
On Mon, Jun 25, 2001 at 10:56:18PM -0400, Dave Cramer wrote: > I have to agree, we need to compile a todo list. > > Mine would include: > > 1) Comprehensive test suite. This may be available already. > 2) Updateable resultSet > 3) Improved DatabaseMetaData > 4) Compatible blob support That looks pretty good from where I stand. I would perhaps separate out also, the fact that the ResultSet.insertXXX() methods also need to be implemented as well as the updateXXX methods. I guess this comes under the updateable resultSet but it's not clear. What about current known bugs or compliance issues? How about the web site? Stuff I'd like added would be, where to find this list, where to find the cvs version, updated binaries, I know someone on this list has made them available. Tom. -- Thomas O'Dowd. - Nooping - http://nooper.com tom@nooper.com - Testing - http://nooper.co.jp/labs
I was the one who made the binaries available at http://jdbc.fastcrypt.com I can continue to offer an environment with 1.18, 1.2, 1.3, and j2ee so we can do builds. Someone else is going to have to do the build using cygwin; or we can create a microsoft make file for windows. The compliance issues are in that list, and are essentially 2, 3, 4 on the list. Dave -----Original Message----- From: Thomas O'Dowd [mailto:tom@uwillsee.com] On Behalf Of Thomas O'Dowd Sent: June 25, 2001 11:30 PM To: Dave Cramer Cc: pgsql-jdbc@postgresql.org Subject: Re: [JDBC] Todo/missing? (was Re: [ADMIN] High memory usage [PATCH]) On Mon, Jun 25, 2001 at 10:56:18PM -0400, Dave Cramer wrote: > I have to agree, we need to compile a todo list. > > Mine would include: > > 1) Comprehensive test suite. This may be available already. > 2) Updateable resultSet > 3) Improved DatabaseMetaData > 4) Compatible blob support That looks pretty good from where I stand. I would perhaps separate out also, the fact that the ResultSet.insertXXX() methods also need to be implemented as well as the updateXXX methods. I guess this comes under the updateable resultSet but it's not clear. What about current known bugs or compliance issues? How about the web site? Stuff I'd like added would be, where to find this list, where to find the cvs version, updated binaries, I know someone on this list has made them available. Tom. -- Thomas O'Dowd. - Nooping - http://nooper.com tom@nooper.com - Testing - http://nooper.co.jp/labs
> On Mon, Jun 25, 2001 at 10:56:18PM -0400, Dave Cramer wrote:
> > I have to agree, we need to compile a todo list.
> >
> > Mine would include:
> >
> > 1) Comprehensive test suite. This may be available already.
> > 2) Updateable resultSet
> > 3) Improved DatabaseMetaData
> > 4) Compatible blob support
Added to official PostgreSQL TODO:
* JDBC
* Comprehensive test suite. This may be available already.
* Updateable resultSet
* Improved DatabaseMetaData
* Compatible blob support
--
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
On Tue, Jun 26, 2001 at 12:04:42AM -0400, Bruce Momjian wrote: > > Added to official PostgreSQL TODO: > > * JDBC > * Comprehensive test suite. This may be available already. > * Updateable resultSet > * Improved DatabaseMetaData > * Compatible blob support Bruce, This is great. I'd like to see Error codes on the list too pending of course work on the backend first. Good to put it on the list though, right? * Error Codes (pending backend implementation) Cheers, Tom. -- Thomas O'Dowd. - Nooping - http://nooper.com tom@nooper.com - Testing - http://nooper.co.jp/labs
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)
* "Dave Cramer" <Dave@micro-automation.net> wrote:
|
| Bruce, Barry,
|
| How about checking for the existence of the date object in the
| constructor? This would mean that it would only be created once per
| thread.
|
I only briefly reviewd the code in PreparedStatement yeaterday with respect
to the usage of the SimpleDateFormat, my comments :
- Why the ThreadLocal usage, and not just syncronize on the same
SimpleDateFormat ?
- It seems to that the SimpleDateFormat is never modified after creation.
Wouldn't this mean that we actually could assign the SimpleDateFormat
object to a static member, so we only had to create the object once.
Or is concurrent usage of SimpleDateFormat.format() unsafe ?
I would prefer the latter if safe, as we would get less object creation and
synchronization.
regards,
Gunnar
--
Gunnar Rønning - gunnar@polygnosis.com
Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
> I only briefly reviewd the code in PreparedStatement yeaterday with respect > to the usage of the SimpleDateFormat, my comments : > > - Why the ThreadLocal usage, and not just syncronize on the same > SimpleDateFormat ? This won't scale. :o) > - It seems to that the SimpleDateFormat is never modified after creation. > Wouldn't this mean that we actually could assign the SimpleDateFormat > object to a static member, so we only had to create the object once. > Or is concurrent usage of SimpleDateFormat.format() unsafe ? SimpleDateFormat.format() and SimpleDateFormat.parse() are not threadsafe, I believe the relevant 'bug' is 4228335. > I would prefer the latter if safe, as we would get less object creation and > synchronization. The latter is what we used to have until it became clear that it wasn't threadsafe. I think the ThreadLocal solution is the best one available in the circumstances. Michael xxx
* Michael Stephenson <mstephenson@tirin.openworld.co.uk> wrote:
|
| > - Why the ThreadLocal usage, and not just syncronize on the same
| > SimpleDateFormat ?
|
| This won't scale. :o)
|
Why ? Performance measurements I have seen and also done before has indicated
that synchronization is faster than the usage of ThreadLocal.
cheers,
Gunnar
--
Gunnar Rønning - gunnar@polygnosis.com
Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
> | This won't scale. :o)
>
> Why ? Performance measurements I have seen and also done before has indicated
> that synchronization is faster than the usage of ThreadLocal.
I've just ran a couple of hugely simplified test (basically starting a
couple of thousand threads doing sdf.parse() with either synchronization
or ThreadLocals a couple of times).
With hot spot VM's (I used Sun JDK 1.3.0_02) the difference appears to be
pretty much non existant either way.
With non hot spot VM's (I used Sun JDK 1.2.2) it seems to have a clearer
pattern, the more threads you have the better the ThreadLocal method
performs, and which performs best depends on how many times each Thread
does the format operation (ie how often setDate is called).
For instance with 1000 threads, if they call setDate less than 5 times
the synchronization method seems to be quickest, 5 or more then the
ThreadLocal method performs better.
By the time you have 3000 threads, it only needs to be called 3 times
before the ThreadLocal method becomes quicker. By 5000 threads, if it's
called even twice per thread the syncronization method is 25% slower.
This is fairly inconclusive either way, because clearly different people
use this code in different ways, but I think backs up waht I said about
ThreadLocal's being more scalable, even if they are less efficient in
smaller cases.
I personally would advocate applying a patch similar to below to the
current cvs source.
Michael xxx
--- PreparedStatement.java.orig Tue Jun 26 16:11:16 2001
+++ PreparedStatement.java Tue Jun 26 16:16:26 2001
@@ -44,6 +44,9 @@
private static ThreadLocal tl_df = new ThreadLocal(); // setDate() SimpleDateFormat
private static ThreadLocal tl_tsdf = new ThreadLocal(); // setTimestamp() SimpleDateFormat
+ private SimpleDateFormat sdSdf; // setDate SimpleDateFormat
+ private SimpleDateFormat stSdf; // setTimeStamp SimpleDateFormat
+
/**
* Constructor for the PreparedStatement class.
* Split the SQL statement into segments - separated by the arguments.
@@ -65,13 +68,18 @@
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);
+ // set up our SimpleDateFormats
+ sdSdf = (SimpleDateFormat)tl_df.get();
+ if (sdSdf == null) {
+ sdSdf = new SimpleDateFormat("''yyyy-MM-dd''");
+ tl_df.set(sdSdf);
+ }
+ stSdf = (SimpleDateFormat)tl_tsdf.get();
+ if (stSdf == null) {
+ stSdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+ tl_tsdf.set(df);
+ }
for (i = 0; i < sql.length(); ++i)
{
@@ -95,17 +103,6 @@
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
*
@@ -342,9 +339,7 @@
*/
public void setDate(int parameterIndex, java.sql.Date x) throws SQLException
{
- SimpleDateFormat df = (SimpleDateFormat) tl_df.get();
-
- set(parameterIndex, df.format(x));
+ set(parameterIndex, sdSdf.format(x));
// The above is how the date should be handled.
//
@@ -381,13 +376,13 @@
*/
public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException
{
- SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get();
- df.setTimeZone(TimeZone.getTimeZone("GMT"));
+ stSdf.setTimeZone(TimeZone.getTimeZone("GMT"));
// Use the shared StringBuffer
synchronized(sbuf) {
sbuf.setLength(0);
- sbuf.append("'").append(df.format(x)).append('.').append(x.getNanos()/10000000).append("+00'");
+ sbuf.append("'").append(stSdf.format(x)).append('.')
+ .append(x.getNanos()/10000000).append("+00'");
set(parameterIndex, sbuf.toString());
}
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 > -- 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
> On Tue, Jun 26, 2001 at 12:04:42AM -0400, Bruce Momjian wrote: > > > > Added to official PostgreSQL TODO: > > > > * JDBC > > * Comprehensive test suite. This may be available already. > > * Updateable resultSet > > * Improved DatabaseMetaData > > * Compatible blob support > > Bruce, > > This is great. I'd like to see Error codes on the list too > pending of course work on the backend first. Good to put it on > the list though, right? > > * Error Codes (pending backend implementation) Added. -- 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
Bruce, I think this is what he is suggesting Dave -----Original Message----- From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] Sent: June 26, 2001 11:14 AM To: Dave@micro-automation.net Cc: 'Barry Lind'; 'PostgreSQL jdbc list' Subject: Re: [JDBC] High Memory Usage Patch -- Disregard my last message 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 > -- 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
Attachment
* "Dave Cramer" <Dave@micro-automation.net> wrote: | | 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 would say that it is better to break existing code that depends on broken behaviour than being out of sync with the standard. Not following the JDBC standard is just asking for a fork - there are a lot of object relational tools out there and if want these to work with postgresql we have no choice. -- Gunnar Rønning - gunnar@polygnosis.com Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
Michael,
Barry pointed out that it isn't necessary for the constructor to be
called from the same thread that is accessing the SimpleDateFormat
objects. I think this requires the test to be done when the objects are
being accessed?
Dave
-----Original Message-----
From: pgsql-jdbc-owner@postgresql.org
[mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Michael Stephenson
Sent: June 26, 2001 11:21 AM
To: 'PostgreSQL jdbc list'; Gunnar Rønning
Subject: Re: [JDBC] Re: RE: [ADMIN] High memory usage [PATCH]
> | This won't scale. :o)
>
> Why ? Performance measurements I have seen and also done before has
indicated
> that synchronization is faster than the usage of ThreadLocal.
I've just ran a couple of hugely simplified test (basically starting a
couple of thousand threads doing sdf.parse() with either synchronization
or ThreadLocals a couple of times).
With hot spot VM's (I used Sun JDK 1.3.0_02) the difference appears to
be
pretty much non existant either way.
With non hot spot VM's (I used Sun JDK 1.2.2) it seems to have a clearer
pattern, the more threads you have the better the ThreadLocal method
performs, and which performs best depends on how many times each Thread
does the format operation (ie how often setDate is called).
For instance with 1000 threads, if they call setDate less than 5 times
the synchronization method seems to be quickest, 5 or more then the
ThreadLocal method performs better.
By the time you have 3000 threads, it only needs to be called 3 times
before the ThreadLocal method becomes quicker. By 5000 threads, if it's
called even twice per thread the syncronization method is 25% slower.
This is fairly inconclusive either way, because clearly different people
use this code in different ways, but I think backs up waht I said about
ThreadLocal's being more scalable, even if they are less efficient in
smaller cases.
I personally would advocate applying a patch similar to below to the
current cvs source.
Michael xxx
--- PreparedStatement.java.orig Tue Jun 26 16:11:16 2001
+++ PreparedStatement.java Tue Jun 26 16:16:26 2001
@@ -44,6 +44,9 @@
private static ThreadLocal tl_df = new ThreadLocal(); //
setDate() SimpleDateFormat
private static ThreadLocal tl_tsdf = new ThreadLocal(); //
setTimestamp() SimpleDateFormat
+ private SimpleDateFormat sdSdf; // setDate SimpleDateFormat
+ private SimpleDateFormat stSdf; // setTimeStamp
SimpleDateFormat
+
/**
* Constructor for the PreparedStatement class.
* Split the SQL statement into segments - separated by the
arguments.
@@ -65,13 +68,18 @@
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);
+ // set up our SimpleDateFormats
+ sdSdf = (SimpleDateFormat)tl_df.get();
+ if (sdSdf == null) {
+ sdSdf = new SimpleDateFormat("''yyyy-MM-dd''");
+ tl_df.set(sdSdf);
+ }
+ stSdf = (SimpleDateFormat)tl_tsdf.get();
+ if (stSdf == null) {
+ stSdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+ tl_tsdf.set(df);
+ }
for (i = 0; i < sql.length(); ++i)
{
@@ -95,17 +103,6 @@
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
*
@@ -342,9 +339,7 @@
*/
public void setDate(int parameterIndex, java.sql.Date x) throws
SQLException
{
- SimpleDateFormat df = (SimpleDateFormat) tl_df.get();
-
- set(parameterIndex, df.format(x));
+ set(parameterIndex, sdSdf.format(x));
// The above is how the date should be handled.
//
@@ -381,13 +376,13 @@
*/
public void setTimestamp(int parameterIndex, Timestamp x) throws
SQLException
{
- SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get();
- df.setTimeZone(TimeZone.getTimeZone("GMT"));
+ stSdf.setTimeZone(TimeZone.getTimeZone("GMT"));
// Use the shared StringBuffer
synchronized(sbuf) {
sbuf.setLength(0);
-
sbuf.append("'").append(df.format(x)).append('.').append(x.getNanos()/10
000000).append("+00'");
+ sbuf.append("'").append(stSdf.format(x)).append('.')
+ .append(x.getNanos()/10000000).append("+00'");
set(parameterIndex, sbuf.toString());
}
---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
* Michael Stephenson <mstephenson@tirin.openworld.co.uk> wrote:
|
| I've just ran a couple of hugely simplified test (basically starting a
| couple of thousand threads doing sdf.parse() with either synchronization
| or ThreadLocals a couple of times).
|
I agree that the tests are pretty inconclusive. We need something that
simulates real usage better. What platform are you testing on Linux ?
If so with native threads or green threads ?
|
| I personally would advocate applying a patch similar to below to the
| current cvs source.
|
This I don't understand, was your intention to remove thread safety with this
patch ? Looking at the way you introduce SimpleDateFormat as an instance
variable it seems so.
| Michael xxx
|
| --- PreparedStatement.java.orig Tue Jun 26 16:11:16 2001
| +++ PreparedStatement.java Tue Jun 26 16:16:26 2001
| @@ -44,6 +44,9 @@
| private static ThreadLocal tl_df = new ThreadLocal(); // setDate() SimpleDateFormat
| private static ThreadLocal tl_tsdf = new ThreadLocal(); // setTimestamp() SimpleDateFormat
|
| + private SimpleDateFormat sdSdf; // setDate SimpleDateFormat
| + private SimpleDateFormat stSdf; // setTimeStamp SimpleDateFormat
| +
| /**
| * Constructor for the PreparedStatement class.
| * Split the SQL statement into segments - separated by the arguments.
| @@ -65,13 +68,18 @@
| 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);
| + // set up our SimpleDateFormats
| + sdSdf = (SimpleDateFormat)tl_df.get();
| + if (sdSdf == null) {
| + sdSdf = new SimpleDateFormat("''yyyy-MM-dd''");
| + tl_df.set(sdSdf);
| + }
| + stSdf = (SimpleDateFormat)tl_tsdf.get();
| + if (stSdf == null) {
| + stSdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
| + tl_tsdf.set(df);
| + }
|
| for (i = 0; i < sql.length(); ++i)
| {
| @@ -95,17 +103,6 @@
| 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
| *
| @@ -342,9 +339,7 @@
| */
| public void setDate(int parameterIndex, java.sql.Date x) throws SQLException
| {
| - SimpleDateFormat df = (SimpleDateFormat) tl_df.get();
| -
| - set(parameterIndex, df.format(x));
| + set(parameterIndex, sdSdf.format(x));
|
| // The above is how the date should be handled.
| //
| @@ -381,13 +376,13 @@
| */
| public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException
| {
| - SimpleDateFormat df = (SimpleDateFormat) tl_tsdf.get();
| - df.setTimeZone(TimeZone.getTimeZone("GMT"));
| + stSdf.setTimeZone(TimeZone.getTimeZone("GMT"));
|
| // Use the shared StringBuffer
| synchronized(sbuf) {
| sbuf.setLength(0);
| - sbuf.append("'").append(df.format(x)).append('.').append(x.getNanos()/10000000).append("+00'");
| + sbuf.append("'").append(stSdf.format(x)).append('.')
| + .append(x.getNanos()/10000000).append("+00'");
| set(parameterIndex, sbuf.toString());
| }
|
|
|
|
| ---------------------------(end of broadcast)---------------------------
| TIP 2: you can get off all lists at once with the unregister command
| (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
|
--
Gunnar Rønning - gunnar@polygnosis.com
Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
My only thought was that we could leave the existing driver in place for people who are depending on it. In retrospect this isn't a good idea. I would recommend breaking existing code to make the driver compatible with the standard. --dc-- -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Gunnar Rønning Sent: June 26, 2001 12:32 PM To: Dave@micro-automation.net Cc: 'Barry Lind'; 'PostgreSQL jdbc list' Subject: Re: [JDBC] RE: [ADMIN] High memory usage [PATCH] * "Dave Cramer" <Dave@micro-automation.net> wrote: | | 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 would say that it is better to break existing code that depends on broken behaviour than being out of sync with the standard. Not following the JDBC standard is just asking for a fork - there are a lot of object relational tools out there and if want these to work with postgresql we have no choice. -- Gunnar Rønning - gunnar@polygnosis.com Senior Consultant, Polygnosis AS, http://www.polygnosis.com/ ---------------------------(end of broadcast)--------------------------- TIP 4: Don't 'kill -9' the postmaster
> I agree that the tests are pretty inconclusive. We need something that > simulates real usage better. What platform are you testing on Linux ? > If so with native threads or green threads ? Linux, I'm using native threads with the hot spot I'm using native threads, which as I said was performing pretty much identically in both cases. The non hot spot machine was using green threads which is where the difference in performance was measurable. > | I personally would advocate applying a patch similar to below to the > | current cvs source. > > This I don't understand, was your intention to remove thread safety with this > patch ? Looking at the way you introduce SimpleDateFormat as an instance > variable it seems so. Yeah, the patch is rubbish, I did it that way to remove the (comparitively expensive) ThreadLocal.get inside the setDate and SetTimeStamp, which is clearly stupid. Michael xxx
Dave, I disagree with your understanding of the double locking issue. You state "The if (anyvar == null) check is flawed in an SMP environment". This statement would more correctly read "The if (anyvar == null) check is flawed in an SMP environment where multiple threads are accessing anyvar". If only one thread is accessing the variable (as is the case in this code since the variable is a local variable that comes from a ThreadLocal object) then there isn't any problem. If the variable in question here where a member variable, then I would agree with you that this if check could be problematic. thanks, --Barry Dave Cramer wrote: > 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... ] >>> >>> >>> >>> >> > > >
Dave, I don't personally know who is working on updateable cursors, but if you search the email archives, there have been a number of emails on this topic over the last couple of months. My issue with updateable cursors it that I think this should be functionality provided by the backend, not patched on to the JDBC driver. My reasoning here is that updateable cursors are very limited in what you can do (single table selects, no where clause, etc). In order to handle this it means that JDBC code is going to need to start parsing the SQL statement to do this. I don't like the idea of having a SQL parser on the client when one already exists on the server. You start running into issues about everytime the server gets a patch to support some new syntax in the SQL statement that the client parser also needs to be updated. In my years at Oracle this was always a problem where the client tools ended up breaking everytime the server added some new syntax support. Having said all of that, I think updateable cursors is a good feature, but I think the majority of the work should be done in the backend and not in the JDBC driver. The added benefit of this is that then all front ends have access to this functionality. thanks, --Barry Dave Cramer wrote: > 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) > >
>
> Having said all of that, I think updateable cursors is a good feature,
> but I think the majority of the work should be done in the backend and
> not in the JDBC driver. The added benefit of this is that then all
> front ends have access to this functionality.
Agreed. This is a backend issue. Updated TODO:
* JDBC
* Comprehensive test suite. This may be available already.
* Updateable resultSet (must be done in backend code)
* Improved DatabaseMetaData
* Compatible blob support
* Error Codes (pending backend implementation)
--
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
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
Greetings,
While I haven't participated yet in this discussion, I've been following it
for quite some time (iow, I'm avoiding work I should be doing). I have some
comments on Barry's patch, which I've included at the end of my message to
be sure I'm commenting on the correct patch.
Creating date formatters is fairly expensive compared to doing the actual
formatting, and I think the most frequent use case will be creating several
PreparedStatements to be accessed within a single Thread or occasionally
across multiple Threads. Given that, I would suggest making the
ThreadLocals static members of PreparedStatement, or if other classes could
use them -- ResultSet creates them on the fly for parsing -- putting them
into a DateFormatUtil class.
This would work since no two PreparedStatements could be accessed by the
same Thread concurrently, and two Threads would each get their own copy as
per ThreadLocal's contract. Also, if Thread A creates a PreparedStatement,
and that PS is accessed by several other Threads, under the current scheme
when the PS is closed, only the closing Thread's formatters are removed.
Granted, once the PS is GCed, I suspect the formatters in the other Threads
would also be GCed, but I haven't tested that out.
Finally, the time zone is set in setTimestamp() after each call to
ThreadLocal.get(). Is this necessary? Can't the formatter be setup with the
GMT time zone once at construction time? Or does each call to format() muck
with it?
Anyway, I have just grabbed the 7.1 source and looked over PS.
Unfortunately, I cannot provide an actual "patch" patch. Hopefully Barry or
others can integrate the following into their code if it is deemed useful
and correct.
Peace,
Dave
---------- 8< ------------------------------------------- 8< --------------
...
class PreparedStatement {
...
private static ThreadLocal tl_df =
new ThreadLocal() {
public Object initialValue() {
return new SimpleDateFormat("''yyyy-MM-dd''");
}
};
private static ThreadLocal tl_tsdf =
new ThreadLocal() {
public Object initialValue() {
SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
df.setTimeZone(TimeZone.getTimeZone("GMT"));
return df;
}
};
...
public static SimpleDateFormat getDateFormatter() {
return (SimpleDateFormat) tl_df.get();
}
public static SimpleDateFormat getTimestampFormatter() {
return (SimpleDateFormat) tl_tsdf.get();
}
...
public void setDate(int parameterIndex, java.sql.Date x) throws SQLException
{
SimpleDateFormat df = getDateFormatter();
set(parameterIndex, df.format(x));
...
}
public void setTimestamp(int parameterIndex, Timestamp x) throws
SQLException
{
SimpleDateFormat df = getTimestampFormatter();
// The following should go either here or in tl_tsdf.initialValue() above
// if the time zone doesn't need to be reset each time.
//df.setTimeZone(TimeZone.getTimeZone("GMT"));
// Use the shared StringBuffer
...
}
...
}
---------- 8< ------------------------------------------- 8< --------------
At 10:00 AM 6/26/2001, Barry Lind wrote:
>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
>
>***
>./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
On Tue, 26 Jun 2001, Thomas O'Dowd wrote: > On Mon, Jun 25, 2001 at 10:56:18PM -0400, Dave Cramer wrote: > > I have to agree, we need to compile a todo list. > > > > Mine would include: > > > > 1) Comprehensive test suite. This may be available already. > > 2) Updateable resultSet > > 3) Improved DatabaseMetaData > > That looks pretty good from where I stand. I would perhaps separate > out also, the fact that the ResultSet.insertXXX() methods also need to be > implemented as well as the updateXXX methods. I guess this comes under > the updateable resultSet but it's not clear. I've been looking at the updateable ResultSet. The best way of implementing it, imo, is adding (as I already mentioned) a property to Field, which tells us where the Field is coming from. Without this, we'll have to parse the SELECT, to see where the Field originated, and imo this is way too complex to be done in the updateXXX methods. Unless anyone has a better idea, I'll implement this. I'm working on the updateable ResultSet as I'm writing this. Everything is looking fine, although this might be a major overhaul. I'm re-reading the jdbc-2.1 spec, and I am consuming immense amounts of Coca-Cola. We'll see what it leads to. Ola -- Ola Sundell ola@miranda.org - olas@wiw.org - ola.sundell@upright.se http://miranda.org/~ola
Ola, How are you planning on determining if the underlying data has been changed by some other process? There is a system column called xmin which can be used, however you will have to add it to the select. Dave -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Ola Sundell Sent: June 26, 2001 6:23 PM To: 'PostgreSQL jdbc list' Subject: Re: [JDBC] Todo/missing? On Tue, 26 Jun 2001, Thomas O'Dowd wrote: > On Mon, Jun 25, 2001 at 10:56:18PM -0400, Dave Cramer wrote: > > I have to agree, we need to compile a todo list. > > > > Mine would include: > > > > 1) Comprehensive test suite. This may be available already. > > 2) Updateable resultSet > > 3) Improved DatabaseMetaData > > That looks pretty good from where I stand. I would perhaps separate > out also, the fact that the ResultSet.insertXXX() methods also need to be > implemented as well as the updateXXX methods. I guess this comes under > the updateable resultSet but it's not clear. I've been looking at the updateable ResultSet. The best way of implementing it, imo, is adding (as I already mentioned) a property to Field, which tells us where the Field is coming from. Without this, we'll have to parse the SELECT, to see where the Field originated, and imo this is way too complex to be done in the updateXXX methods. Unless anyone has a better idea, I'll implement this. I'm working on the updateable ResultSet as I'm writing this. Everything is looking fine, although this might be a major overhaul. I'm re-reading the jdbc-2.1 spec, and I am consuming immense amounts of Coca-Cola. We'll see what it leads to. Ola -- Ola Sundell ola@miranda.org - olas@wiw.org - ola.sundell@upright.se http://miranda.org/~ola ---------------------------(end of broadcast)--------------------------- TIP 4: Don't 'kill -9' the postmaster
* Bruce Momjian <pgman@candle.pha.pa.us> wrote: | | * Compatible blob support * BLOB support compatible with the JDBC standard. Breaks compatibility with earlier versions of the JDBC driver. I suggest that we go this path, so we open up postgresql as a choice for more third party tools and applications. -- Gunnar Rønning - gunnar@polygnosis.com Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
Updated:
* JDBC-standard BLOB support
> * Bruce Momjian <pgman@candle.pha.pa.us> wrote:
> |
>
> | * Compatible blob support
>
> * BLOB support compatible with the JDBC standard. Breaks compatibility
> with earlier versions of the JDBC driver.
>
> I suggest that we go this path, so we open up postgresql as a choice for
> more third party tools and applications.
>
> --
> Gunnar R?nning - gunnar@polygnosis.com
> Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
>
> ---------------------------(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
Can we also add: * support for binary data (bytea type) thanks, --Barry Bruce Momjian wrote: >>On Tue, Jun 26, 2001 at 12:04:42AM -0400, Bruce Momjian wrote: >> >>>Added to official PostgreSQL TODO: >>> >>>* JDBC >>> * Comprehensive test suite. This may be available already. >>> * Updateable resultSet >>> * Improved DatabaseMetaData >>> * Compatible blob support >>> >>Bruce, >> >>This is great. I'd like to see Error codes on the list too >>pending of course work on the backend first. Good to put it on >>the list though, right? >> >>* Error Codes (pending backend implementation) >> > > Added. > >
Dave, Can you give a little more detail on what you mean by 'Improved DatabaseMetaData'? What specific areas are currently lacking? thanks, --Barry >>On Mon, Jun 25, 2001 at 10:56:18PM -0400, Dave Cramer wrote: >> >>>I have to agree, we need to compile a todo list. >>> >>>Mine would include: >>> >>>1) Comprehensive test suite. This may be available already. >>>2) Updateable resultSet >>>3) Improved DatabaseMetaData >>>4) Compatible blob support >>> > > Added to official PostgreSQL TODO: > > * JDBC > * Comprehensive test suite. This may be available already. > * Updateable resultSet > * Improved DatabaseMetaData > * Compatible blob support > >
Done. > Can we also add: > > * support for binary data (bytea type) > > thanks, > --Barry > > > > Bruce Momjian wrote: > > >>On Tue, Jun 26, 2001 at 12:04:42AM -0400, Bruce Momjian wrote: > >> > >>>Added to official PostgreSQL TODO: > >>> > >>>* JDBC > >>> * Comprehensive test suite. This may be available already. > >>> * Updateable resultSet > >>> * Improved DatabaseMetaData > >>> * Compatible blob support > >>> > >>Bruce, > >> > >>This is great. I'd like to see Error codes on the list too > >>pending of course work on the backend first. Good to put it on > >>the list though, right? > >> > >>* Error Codes (pending backend implementation) > >> > > > > Added. > > > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://www.postgresql.org/search.mpl > -- 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
The ResultSetMetaData needs work as well! I will elaborate shortly. Dave -----Original Message----- From: pgsql-jdbc-owner@postgresql.org [mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of Barry Lind Sent: June 26, 2001 9:37 PM To: Bruce Momjian Cc: Thomas O'Dowd; pgsql-jdbc@postgresql.org Subject: [JDBC] Re: Todo/missing? (was Re: [ADMIN] High memory usage [PATCH]) Can we also add: * support for binary data (bytea type) thanks, --Barry Bruce Momjian wrote: >>On Tue, Jun 26, 2001 at 12:04:42AM -0400, Bruce Momjian wrote: >> >>>Added to official PostgreSQL TODO: >>> >>>* JDBC >>> * Comprehensive test suite. This may be available already. >>> * Updateable resultSet >>> * Improved DatabaseMetaData >>> * Compatible blob support >>> >>Bruce, >> >>This is great. I'd like to see Error codes on the list too >>pending of course work on the backend first. Good to put it on >>the list though, right? >> >>* Error Codes (pending backend implementation) >> > > Added. > > ---------------------------(end of broadcast)--------------------------- TIP 6: Have you searched our list archives? http://www.postgresql.org/search.mpl
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
I think this is what he is suggesting. --dc-- -----Original Message----- From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] Sent: June 26, 2001 11:14 AM To: Dave@micro-automation.net Cc: 'Barry Lind'; 'PostgreSQL jdbc list' Subject: Re: [JDBC] High Memory Usage Patch -- Disregard my last message 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 > -- 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
Attachment
On Tue, 26 Jun 2001, Dave Cramer wrote: > Ola, > > How are you planning on determining if the underlying data has been > changed by some other process? There is a system column called xmin > which can be used, however you will have to add it to the select. > > Dave At first, I will let the backend handle any and all problems that might occur. I will simply forward any exceptions I get, and let the overlying code handle the problems. I checked the MySQL driver, and that is how they are doing it. I wonder how it works with non-free databases. Does anyone here know? Ola -- Ola Sundell ola@miranda.org - olas@wiw.org - ola.sundell@upright.se http://miranda.org/~ola
On Sun, 8 Jul 2001, Ola Sundell wrote: > On Tue, 26 Jun 2001, Dave Cramer wrote: > > > Ola, > > > > How are you planning on determining if the underlying data has been > > changed by some other process? There is a system column called xmin > > which can be used, however you will have to add it to the select. > > > > Dave > > At first, I will let the backend handle any and all problems that might > occur. I will simply forward any exceptions I get, and let the overlying > code handle the problems. I checked the MySQL driver, and that is how they > are doing it. > > I wonder how it works with non-free databases. Does anyone here know? Silly me. When I woke up this morning, I realised that MySQL doesn't support transactions. I re-read the JDBC spec, and I will do a simple optimistic concurrency control by starting a transaction, re-reading the row and checking the values. mvh Ola -- Ola Sundell ola@miranda.org - olas@wiw.org - ola.sundell@upright.se http://miranda.org/~ola