Skip site navigation (1) Skip section navigation (2)

Re: [PATCHES] Re: High Memory Usage Patch -- Disregard my last message

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Barry Lind <barry(at)xythos(dot)com>
Cc: pgsql-patches(at)postgresql(dot)org, "'PostgreSQL jdbc list'" <pgsql-jdbc(at)postgresql(dot)org>
Subject: Re: [PATCHES] Re: High Memory Usage Patch -- Disregard my last message
Date: 2001-06-29 17:21:58
Message-ID: 200106291721.f5THLwn02654@candle.pha.pa.us (view raw or flat)
Thread:
Lists: pgsql-adminpgsql-jdbcpgsql-patches
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(at)xythos(dot)com] 
> >>Sent: June 25, 2001 11:39 AM
> >>To: Dave(at)micro-automation(dot)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(at)xythos(dot)com] 
> >>>Sent: June 25, 2001 12:37 AM
> >>>To: pgsql-patches(at)postgresql(dot)org
> >>>Cc: Dave(at)micro-automation(dot)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(at)postgresql(dot)org
> >>>>>[mailto:pgsql-jdbc-owner(at)postgresql(dot)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(at)vgkk(dot)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(at)polygnosis(dot)com
> >>>>>Senior Consultant, Polygnosis AS, http://www.polygnosis.com/
> >>>>>
> >>>>>---------------------------(end of
> >>>>>
> >>>>>
> >>>broadcast)---------------------------
> >>>
> >>>
> >>>>>TIP 1: subscribe and unsubscribe commands go to
> >>>>>
> >>>>>
> >>>majordomo(at)postgresql(dot)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(at)postgresql(dot)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(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

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

In response to

pgsql-admin by date

Next:From: Ross J. ReedstromDate: 2001-06-29 18:03:05
Subject: Re: Re: rename a table
Previous:From: Tom LaneDate: 2001-06-29 14:38:43
Subject: Re: Postgresql 7.1 dying on Debian Woody

pgsql-patches by date

Next:From: Barry LindDate: 2001-07-01 00:01:33
Subject: Patch to remove connection hook and JDK 1.3 dependencies
Previous:From: David D. KilzerDate: 2001-06-27 20:44:49
Subject: Re: [PATCH] Contrib C source for casting MONEY to INT[248] and FLOAT[48]

pgsql-jdbc by date

Next:From: Bruce MomjianDate: 2001-06-29 19:15:55
Subject: Re: Re: JDBC and security
Previous:From: Joel StevensonDate: 2001-06-29 16:39:48
Subject: Re: old product version number

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group