Re: Removing our datasource/pooling implementation.

Having received another report[1] of the lack of robustness of our pooling
implementation I think we should scrap our datasource and pooling
implementation. I previously advocated keeping it around because it
"basically worked" and didn't really cost us anything to keep it. Now
we're aware that it doesn't really work and I for one don't want to spend
time fixing it when there are better options out there.

I spent some time today testing jakarta's dbcp[2] and I couldn't find
anything our code does that it cannot and there are plenty of additional
features. Dynamic pool sizing, removing broken connections, and even
statement pooling are available. I was impressed.

Would anyone like to make a case for keeping our implementation around?

Kris Jurka

[1] http://gborg.postgresql.org/project/pgjdbc/bugs/bugupdate.php?1109
[2] http://jakarta.apache.org/commons/dbcp/

Kris,

AFAIR, the jdbccts requires a data source, not sure about the pooling
implementation.

Hibernate has a sub-standard pooling implementation and explicitely logs
this fact when it starts up.
I'm of two minds here, if we tell people that it isn't intended for
production, AND they actually read it, we could leave it in.
On the other hand if it really doesn't work and isn't necessary then it
is just extra code we need to maintain.

+1 for removal of the pooling implementation if it isn't required for
jdbccts.

Dave

Kris Jurka wrote:

>Having received another report[1] of the lack of robustness of our pooling
>implementation I think we should scrap our datasource and pooling
>implementation. I previously advocated keeping it around because it
>"basically worked" and didn't really cost us anything to keep it. Now
>we're aware that it doesn't really work and I for one don't want to spend
>time fixing it when there are better options out there.
>
>I spent some time today testing jakarta's dbcp[2] and I couldn't find
>anything our code does that it cannot and there are plenty of additional
>features. Dynamic pool sizing, removing broken connections, and even
>statement pooling are available. I was impressed.
>
>Would anyone like to make a case for keeping our implementation around?
>
>Kris Jurka
>
>[1] http://gborg.postgresql.org/project/pgjdbc/bugs/bugupdate.php?1109
>[2] http://jakarta.apache.org/commons/dbcp/
>
>---------------------------(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
>
>
>
>

--
Dave Cramer
http://www.postgresintl.com
519 939 0336
ICQ#14675561

The DataSource implementation was never meant to be robust, and
can be scrapped as far as I'm concerned. The ConnectionPoolDataSource
implementation should be kept, though it's probably not too popular since
it seems no one much uses that interface.

Aaron

On Wed, 5 Jan 2005, Kris Jurka wrote:
> Having received another report[1] of the lack of robustness of our pooling
> implementation I think we should scrap our datasource and pooling
> implementation. I previously advocated keeping it around because it
> "basically worked" and didn't really cost us anything to keep it. Now
> we're aware that it doesn't really work and I for one don't want to spend
> time fixing it when there are better options out there.
>
> I spent some time today testing jakarta's dbcp[2] and I couldn't find
> anything our code does that it cannot and there are plenty of additional
> features. Dynamic pool sizing, removing broken connections, and even
> statement pooling are available. I was impressed.
>
> Would anyone like to make a case for keeping our implementation around?
>
> Kris Jurka
>
> [1] http://gborg.postgresql.org/project/pgjdbc/bugs/bugupdate.php?1109
> [2] http://jakarta.apache.org/commons/dbcp/
>
> ---------------------------(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
>

Kris Jurka wrote:

> Having received another report[1] of the lack of robustness of our pooling
> implementation I think we should scrap our datasource and pooling
> implementation. [...]

> Would anyone like to make a case for keeping our implementation around?

I use our plain DataSource and ConnectionPoolDataSource implementations.
Please keep them; the CPDS, especially, has scope to do driver-specific
work (consider RESET CONNECTION on proxy connection close()) that can't
be done at a higher level easily.

Dropping the "pooling" DataSource implementation is fine, dbcp fills
that role well by the sounds of it and there's no real reason it has to
be implemented by the driver.

-O

On Fri, 7 Jan 2005, Oliver Jowett wrote:

> I use our plain DataSource and ConnectionPoolDataSource implementations.
> Please keep them; the CPDS, especially, has scope to do driver-specific
> work (consider RESET CONNECTION on proxy connection close()) that can't
> be done at a higher level easily.
>

The real problems seem to be in our PooledConnection implemention which is
what I really wanted to get rid of. Since that's needed for CPDS I've
kept the pooling datasource implementation, but updated the documentation
to more strongly recommend against using it.

Kris Jurka

Kris Jurka wrote:
>
> On Fri, 7 Jan 2005, Oliver Jowett wrote:
>
>>I use our plain DataSource and ConnectionPoolDataSource implementations.
>>Please keep them; the CPDS, especially, has scope to do driver-specific
>>work (consider RESET CONNECTION on proxy connection close()) that can't
>>be done at a higher level easily.
>
> The real problems seem to be in our PooledConnection implemention which is
> what I really wanted to get rid of. Since that's needed for CPDS I've
> kept the pooling datasource implementation, but updated the documentation
> to more strongly recommend against using it.

What needs fixing in our PooledConnection implementation? I can take a
look at repairing whatever concerns you. I haven't noticed any problems
myself, but our app hardly exercises the whole class..

-O

On Wed, 12 Jan 2005, Oliver Jowett wrote:

> What needs fixing in our PooledConnection implementation? I can take a
> look at repairing whatever concerns you. I haven't noticed any problems
> myself, but our app hardly exercises the whole class..
>

First I don't use our PooledConnection and I'm also a little unclear on
whose resposibility (CPDS vs the actual pool) certain things are, so this
is just a combination of anecdotes and looking at the code. Nothing is
really broken for normal use, but when something goes wrong there is very
little error detection and fallback. Specifically when a connection to
the database server is lost via a server restart or even potentially user
level code Statement.execute("SET client_encoding TO 'LATIN1'); the dead
connection is not destroyed and will keep being given to clients.

Kris Jurka

Kris Jurka wrote:

> First I don't use our PooledConnection and I'm also a little unclear on
> whose resposibility (CPDS vs the actual pool) certain things are, so this
> is just a combination of anecdotes and looking at the code. Nothing is
> really broken for normal use, but when something goes wrong there is very
> little error detection and fallback. Specifically when a connection to
> the database server is lost via a server restart or even potentially user
> level code Statement.execute("SET client_encoding TO 'LATIN1'); the dead
> connection is not destroyed and will keep being given to clients.

The CPDS/PooledConnection has the responsibility for creating connection
proxy objects and reporting proxy connection closes and connection fatal
errors via a connection listener. The actual pool is responsible for
things like pool size, max idle time, whether to recheck connections
periodically, etc.

Looking at the code, it looks like fireConnectionFatalError() isn't
being called in the normal execution (non-setup) path at all. If it was
being called, then connection pools would have a chance to see the error
via a connection listener and discard the bad connection.

I will look at fixing this. Probably the best way to do it is monitor
the SQLState of thrown exceptions in the InvocationHandler and know
about which states are fatal (e.g. connection failure, driver internal
errors).

-O

On Wed, 12 Jan 2005, Oliver Jowett wrote:

> Looking at the code, it looks like fireConnectionFatalError() isn't
> being called in the normal execution (non-setup) path at all.

Yes, this is the root of the problem.

> I will look at fixing this. Probably the best way to do it is monitor
> the SQLState of thrown exceptions in the InvocationHandler and know
> about which states are fatal (e.g. connection failure, driver internal
> errors).
>

That could work, although coming up with the list of fatal states might be
tricky. I've never paid much attention to the sql states that the driver
assigns itself, this might be the time to change that and remove the
PSQLException constructor that doesn't take a PSQLState argument.

Kris Jurka

Kris Jurka wrote:

> That could work, although coming up with the list of fatal states might be
> tricky. I've never paid much attention to the sql states that the driver
> assigns itself, this might be the time to change that and remove the
> PSQLException constructor that doesn't take a PSQLState argument.

I've committed some changes that do this.

Some of the SQL states I assigned aren't very good matches. The main one
is that "you can't do that operation on that sort of object" type of
error is mapped to "object not in prerequisite state". Any better ideas?

PooledConnectionImpl has a list of SQL state classes that are considered
fatal, and inspects each thrown exception's SQL state to see whether to
tell the connection listener about the error.

I also changed the default autocommit setting for our CPDS
implementation to true to be consistent with normal connections & the
JDBC spec. I seem to remember this was discussed on the list ages ago
but I guess nothing ever got changed at the time.

-O

Oliver Jowett <oliver(at)opencloud(dot)com> writes:
> Some of the SQL states I assigned aren't very good matches. The main one
> is that "you can't do that operation on that sort of object" type of
> error is mapped to "object not in prerequisite state". Any better ideas?

ERRCODE_WRONG_OBJECT_TYPE (42809) seems like a better match. The
prerequisite-state error implies that you could have done that operation
on that object, if it were in a more receptive mood.

The 42xxx series covers pretty much anything in the
what-were-you-thinking category, AFAICS. If I'd been on the committee
I would have broken it down a bit more...

regards, tom lane

Tom Lane wrote:
> Oliver Jowett <oliver(at)opencloud(dot)com> writes:
>
>>Some of the SQL states I assigned aren't very good matches. The main one
>>is that "you can't do that operation on that sort of object" type of
>>error is mapped to "object not in prerequisite state". Any better ideas?
>
>
> ERRCODE_WRONG_OBJECT_TYPE (42809) seems like a better match. The
> prerequisite-state error implies that you could have done that operation
> on that object, if it were in a more receptive mood.

Thanks, that seems like a better match; I've changed most occurrences.
There were a couple of cases where "not in prerequisite state" was
closer, e.g. when trying to use a closed object.

-O