Thread: PGPoolingDataSource problem.

You have two major programming issues:

1) You ignore exceptions
2) You use static variables / singleton pattern without understanding

Get yourself a good debugger, where you can step through the code
line-by-line, and watch what happens to various "connection" variables
during your two procedures.  It will be obvious why and where you "released"
the original connection without closing it.

See notes below (in bold) to help you figure out where to focus your
efforts.

David J.

From: pgsql-jdbc-owner@postgresql.org
[mailto:pgsql-jdbc-owner@postgresql.org] On Behalf Of
Benjamin.Galaviz@LSGSkyChefs.com
Sent: Wednesday, June 08, 2011 1:37 PM
To: pgsql-jdbc@postgresql.org
Subject: [JDBC] PGPoolingDataSource problem.

      private static Connection conn = null;

      public Connection getConnection(){ ---- You are returning a static
variable from a non-static method...
            PGPoolingDataSource pgdb = (PGPoolingDataSource)getInstance();
            conn = null; ---- and what if there was already another
connection present..

            try{
                  conn = pgdb.getConnection();
            }catch(Exception e){
                  e.printStackTrace();
            }

            return conn;
      }

      public static void closeConnection(){
            if (conn != null){
                  try{conn.close();conn = null;}catch(Exception e){} ----
NEVER IGNORE EXCEPTIONS; ESPECIALLY CATCH (Exception e)
            }
      }

      try{DatabasePool.closeConnection();db=null;}catch(Exception e){}
---- NEVER IGNORE EXCEPTIONS; ESPECIALLY CATCH (Exception e)

!!!!!! You are likely capturing - then ignoring - a NullPointerException
here !!!!!!!!!

Private String getValue(String XX){
                                PreparedStatement ps  =
db.getConnection().preparedStatement(sql);
                                ResultSet rs = ps.executeQuery();
                                While (rs.next()){
                                                retVal = rs.getString(1);
                                }
             try{DatabasePool.closeConnection();db=null;}catch(Exception
e){}
       }
}

--- Open and close within the same function is OK IF there was no connection
already open

                                String myValue = getValue(XX);   ----Open
and Close
                                PreparedStatement ps =
db.getConnection().preparedStatement(sql);   --- Open
             try{DatabasePool.closeConnection();db=null;}catch(Exception
e){} ----Close

On 09/06/11 01:36, Benjamin.Galaviz@LSGSkyChefs.com wrote:
> I am using the JDBC3 PGPoolingDataSource to create a pool of 5
> connections (for test) to my database.

I'd recommend using a proper connection pool like C3P0 or DBCP. It'll be
less work in the long run, as PGPoolingDataSource is more of a
toy/testing tool.

What's your environment, anyway? Are you running in a standalone JVM
(j2SE) or in an application server context? If you're in an appserver or
servlet container you should be getting connections from the container
environment.

--
Craig Ringer

 On Thu, 09 Jun 2011 12:07:32 +0800, Craig Ringer wrote:
> On 09/06/11 01:36, Benjamin.Galaviz@LSGSkyChefs.com wrote:
>> I am using the JDBC3 PGPoolingDataSource to create a pool of 5
>> connections (for test) to my database.
>
> I'd recommend using a proper connection pool like C3P0 or DBCP. It'll
> be
> less work in the long run, as PGPoolingDataSource is more of a
> toy/testing tool.
>
> What's your environment, anyway? Are you running in a standalone JVM
> (j2SE) or in an application server context? If you're in an appserver
> or
> servlet container you should be getting connections from the
> container
> environment.
>
> --
> Craig Ringer

 From other posts, it looks like C3P0 is only JDBC2 aware.  so you will
 automatic mange them.

 Actually PoolingDataSource could track leak and add auto-close, I was
 sure it could do this, but I'm wrong.

 Backing to original post...

 From design point of view your implementation in not quite good, bad
 not bad.

 - You use singleton pattern to manage DataSource and it's good.
 - But private static Connection conn = null; should it really be
 static? getConnection is not static this causes leak.
 - And one tip - when I make singleton pattern I, mainly, create for
 class keeping this value private default constructor, so no one who use
 my code will not accidental call new Singleton, instead of
 Singleton.getInstance().
 - Just fix pool, and if you have time wrap your connections managed by
 pool in WeakReference (eventually adding pool to this reference). This
 is how pools are implemented. Everyone know to close connection but
 smart developers know that many forget it, and adding leaking support
 may save your time in future on writing try / catch trees.

 Have a luck.

 Regards,
 Radek

On 9/06/2011 3:30 PM, Radosław Smogura wrote:

>  From other posts, it looks like C3P0 is only JDBC2 aware. so you will
> automatic mange them.

Eh? What does the JDBC level supported have to do with how you manage
the connections?

Can you link to the posts in question? I'm a bit confused by that.

--
Craig Ringer

Tech-related writing at http://soapyfrogs.blogspot.com/

 On Thu, 09 Jun 2011 17:39:05 +0800, Craig Ringer wrote:
> On 9/06/2011 3:30 PM, Radosław Smogura wrote:
>
>>  From other posts, it looks like C3P0 is only JDBC2 aware. so you
>> will
>> automatic mange them.
>
> Eh? What does the JDBC level supported have to do with how you manage
> the connections?
>
> Can you link to the posts in question? I'm a bit confused by that.

 http://archives.postgresql.org/pgsql-jdbc/2011-06/msg00004.php

 It means you may pool, but you will have limited access to other API
 methods.

 Regards,
 Radek

On 06/08/11 18:36, Benjamin.Galaviz@LSGSkyChefs.com wrote:
> I am using the JDBC3 PGPoolingDataSource to create a pool of 5 connections
> (for test) to my database.
>
> public class DatabasePool {
>

Improved and corrected (albeit untested) versions of your code below -
but please read my comment at the end of this message on why you don't
want to reinvent the wheel.

public class ConnectionFactory {
     private static PGPoolingDataSource ds;

     private ConnectionFactory() {
         // enforce singleton
     }

     private static synchronized DataSource getDataSource() throws
SQLException {
         if (ds == null){
             ds = new PGPoolingDataSource();
             ds.setDataSourceName("DataSource");
             ds.setServerName("XX.XX.XX.XX");
             ds.setDatabaseName("dbname");
             ds.setUser("user");
             ds.setPassword("password");
             ds.setMaxConnections(5);
         }

         return ds;
     }

     public static Connection getConnection() throws SQLException {
         return getDataSource.getConnection();
     }

     public static void releaseConnection(final Connection connection) {
         if (connection != null) {
             try {
                 connection.close();
             } catch (SQLException exception) {
                 // log warning ...
             }
         }
     }
}
>
> I get a leaked connections when I grab more than one connection from the
> pool at a time.
>
> Public void insertData(){
>
> Private String getValue(String XX){
>

public void insertData(final String sql) throws SQLException {
     Connection connection = null;

     try {
         connection = ConnectionFactory.getConnection();

         PreparedStatement ps = connection.preparedStatement(sql);
         ps.setString(1, "foo");
         ps.executeUpdate();
         ps.close();
     } finally{
         ConnectionFactory.releaseConnection(connection);
     }
}

public String getValue(final String sql, final String foo) throws
SQLException {
     String value = null;

     Connection connection = null;

     try {
         connection = ConnectionFactory.getConnection();

         PreparedStatement ps = connection.preparedStatement(sql);

         ResultSet rs = ps.executeQuery();

         if (rs.next()) {
             value = rs.getString(1);
         }

         rs.close();

         ps.close();
     } finally{
         ConnectionFactory.releaseConnection(connection);
     }

     return value;
}

However, you'd be much better served by using a framework that hides
the verboseness of JDBC. The Spring framework has an excellent wrapper
for JDBC that also supports pooling, and deals well with the various
niggles that such a wrapper exposes.

Chris




Chris Wareham
Senior Software Engineer
London & Partners
6th floor,
2 More London Riverside
London SE1 2RR

Tel: +44 (0)20 7234 5848
Fax: +44 (0)20 7234 5753

http://www.londonandpartners.com/


'London & Partners Limited' is registered in England under No.7493460;
Registered Office:London & Partners, City Hall, The Queen's Walk, London SE1 2AA.

London & Partners is the official promotional agency for London attracting and delivering value to businesses, students
andvisitors. London & Partners is a not-for-profit public private partnership, funded by the Mayor of London and our
networkof commercial partners. 

The information contained in this e-mail is confidential and intended for the named recipient(s) only. If you have
receivedit in error, please notify the sender immediately and then delete the message. If you are not the intended
recipient,you must not use, disclose, copy or distribute this email. The views expressed in this e-mail are those of
theindividual and not of London & Partners Limited. We reserve the right to read and monitor any email or attachment
enteringor leaving our systems without prior notice. 

 Please don't print this e-mail unless you really need to.

On 06/09/2011 06:01 PM, Radosław Smogura wrote:

> It means you may pool, but you will have limited access to other API
> methods.

OK, then use DBCP 1.4, which supports JDBC4, instead of C3P0.

Thanks for the clarification of what you meant.

--
Craig Ringer