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

Clean up open connections to fix test failures on omni and appveyor #2251

Merged
merged 4 commits into from
Sep 11, 2021

Conversation

sehrope
Copy link
Member

@sehrope sehrope commented Sep 11, 2021

The omni CI and appveyor are failing on some of the older server versions. Couple unrelated causes.

First is #2245 added some DDL for adding a COMMENT ON FUNCTION ... f ... (rather than f()) that only works on server v10+.

Second is that some tests were creating connections but not closing them. Again on some older versions and on appveyor this was leading to connection exhaustion. Same PR had one offender and I found another in Statement Test.

Finally, the new test added in #2247 was failing on v8.4 as updating application_name is not supported.

This PR should fix all of that. The StatementTest change is just a wrapping that block in try-with-resources so viewing the diff with whitespaced ignored is a easier:

diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java
index d6ef9e8b..0901e5f2 100644
--- a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java
+++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/StatementTest.java
@@ -476,7 +476,7 @@ public class StatementTest {
   @Test
   public void testWarningsAreAvailableAsap()
       throws Exception {
-    final Connection outerLockCon = TestUtil.openDB();
+    try (final Connection outerLockCon = TestUtil.openDB()) {
       outerLockCon.setAutoCommit(false);
       //Acquire an exclusive lock so we can block the notice generating statement
       outerLockCon.createStatement().execute("LOCK TABLE test_lock IN ACCESS EXCLUSIVE MODE;");
@@ -529,6 +529,7 @@ public class StatementTest {
         executorService.shutdownNow();
       }
     }
+  }
 
   /**
    * <p>Demonstrates a safe approach to concurrently reading the latest

I've got it running the omni action on my branch: https://github.com/sehrope/pgjdbc/actions/runs/1224062070. Once it passes I'll merge this in.

…ment

Fixes DatabaseMetaDataTest to use older syntax for COMMENT ON FUNCTION with
explicit no-arg parameter parentheses as it is required on server versions
before v10.
Older versions do not allow setting the application_name property so restrict the
test to only v9.0+.
@sehrope sehrope force-pushed the fix-omni-and-appveyor-failures branch from 143d969 to 3a844b3 Compare September 11, 2021 10:40
@sehrope sehrope merged commit 18939c4 into pgjdbc:master Sep 11, 2021
@sehrope
Copy link
Member Author

sehrope commented Sep 11, 2021

This has been merged. I kicked off a manual omni run to verify everything is working fine in this repo as well: https://github.com/pgjdbc/pgjdbc/actions/runs/1224235785

@davecramer We should cherry-pick the parts of this that apply to the 42.2 branch. I think it's just the DatabaseMetaDataTest commits as the rest isn't part of that branch.

@davecramer
Copy link
Member

@sehrope Thanks for taking care of this! Agreed on cherry picking. If you have time, otherwise I'll get to it next week

I'd like to release 42.2.24 sooner than later and I'd like to do something about 42.3 soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants