Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that JDBC Connections that are closed by the server do not repo… #1533

Merged
merged 1 commit into from Aug 30, 2019

Conversation

@timothyjward
Copy link
Contributor

@timothyjward timothyjward commented Jul 24, 2019

Signed-off-by: Tim Ward timothyjward@apache.org

As part of investigating a bug report against Apache Aries Tx Control I had to look at a potential issue in the Postgres JDBC driver. I noticed that a comment in PR #218 described the situation that I thought might be broken, so I wrote a test for it. The driver turned out not to be the problem, but I thought you might still want the additional test

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Does mvn checkstyle:check pass ?

Changes to Existing Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?
…rt as valid

Signed-off-by: Tim Ward <timothyjward@apache.org>
@davecramer
Copy link
Member

@davecramer davecramer commented Jul 24, 2019

Cool!, thanks

Loading

@AppVeyorBot
Copy link

@AppVeyorBot AppVeyorBot commented Jul 24, 2019

Loading


TestUtil.execute("select pg_terminate_backend(" + pid + ")", con2);

assertFalse("The Second connection should now be invalid", con.isValid(0));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that assertion messages could be refined, as isValid is still called on first connection as I read it here.

Loading

Copy link
Member

@davecramer davecramer Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Loading

Copy link
Member

@davecramer davecramer Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the code is correct. The first connection is the one that has been terminated.

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the code is correct. The first connection is the one that has been terminated.

that assertion message could be refined

IMO it'd be misleading in case of test failure. Message is about The Second connection but the condition is not checked for con2, but for First Connection (as named in line 84).

The first connection is the one that has been terminated.

Right, so I do not get why The Second connection should now be invalid.

Loading

Copy link
Member

@davecramer davecramer Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, I see what the issue is. The comment... I'll fix. Thanks

Loading

@codecov-io
Copy link

@codecov-io codecov-io commented Jul 24, 2019

Codecov Report

Merging #1533 into master will increase coverage by 2.54%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1533      +/-   ##
============================================
+ Coverage     66.39%   68.94%   +2.54%     
- Complexity     3787     3957     +170     
============================================
  Files           179      179              
  Lines         16475    16491      +16     
  Branches       2678     2678              
============================================
+ Hits          10939    11370     +431     
+ Misses         4296     3877     -419     
- Partials       1240     1244       +4

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Aug 27, 2019

@timothyjward can you fix up the call to isValid ?

Loading

@davecramer davecramer merged commit 56399ef into pgjdbc:master Aug 30, 2019
3 checks passed
Loading
@skyrocker121
Copy link

@skyrocker121 skyrocker121 commented Mar 4, 2020

@timothyjward @davecramer I was just randomly checking the issues that have been fixed in the recent releases of the postgres driver. The main reason for doing this is that we have seen a strange issue at our end where the connection pool is reaching the exhaust limit very fast. We had the Connection_maxActive setting to 150 and it was reaching this limit within a short duration. We use tomcat for connection pool in our java code. We support SQL server as well. So the code is generic and we never faced the connection limit issue until now.. However we recently upgraded the tomcat jars from v9.0.22 to v9.0.31 ...I did notice a comment on this thread that only the test case was fixed and the driver code was all fine. However i just want to pick your brains on this . After reverting to the older tomcat jars, one of the environments seems to be working fine (Just 1 day of testing and i guess it also depends on the workload - how many connections are being opened and how frequently) and not giving us the exhaust limit error. We are using postgresql-42.2.6.jar and i was thinking if I could move to the latest version and i don't see the issue even with apache tomcat V9.0.31 version ?? Any pointers on how i should approach this problem would be useful ..Thanks in advance :)

Error stack trace - you probably know what I mean by above but just pasting it here for reference
org.apache.tomcat.jdbc.pool.PoolExhaustedException: [Control] Timeout: Pool empty. Unable to fetch a connection in 30 seconds, none available[size:150; busy:150; idle:0; lastwait:30000].
at org.apache.tomcat.jdbc.pool.ConnectionPool.borrowConnection(ConnectionPool.java:717)
at org.apache.tomcat.jdbc.pool.ConnectionPool.getConnection(ConnectionPool.java:198)
at org.apache.tomcat.jdbc.pool.DataSourceProxy.getConnection(DataSourceProxy.java:132)

Loading

@davecramer
Copy link
Member

@davecramer davecramer commented Mar 5, 2020

@skyrocker121 to start with please upgrade the JDBC driver to the latest. That said I find it difficult to believe it is the JDBC driver but I've been wrong before. At any rate we don't really look at previous versions for bug fixes so start with the latest driver and report back.

Thanks

Loading

davecramer added a commit to davecramer/pgjdbc that referenced this issue Jul 5, 2021
…rt as valid (pgjdbc#1533)

Signed-off-by: Tim Ward <timothyjward@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants