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

fix: close refcursors when underlying cursor==null instead of relying on defaultRowFetchSize #2377

Merged
merged 1 commit into from May 23, 2022

Conversation

vlsi
Copy link
Member

@vlsi vlsi commented Dec 29, 2021

PgResultSet might hold ResultCursor cursor for fetching the next rows.
So we should use ResultCursor cursor==null condition to check if we want to close <unnamed portal 1> from refcursors.

See #2227
See #2371

/cc @davecramer

@vlsi
Copy link
Member Author

vlsi commented May 19, 2022

This PR fixes the following test as well (it is covered with the new parameterized test), so this PR fixes regression introduced in #2371

  @Test
  public void testRefCursorWithFetchSizeOnResultSet() throws SQLException {
    int cnt = 0;
    try (CallableStatement call = con.prepareCall("{? = call test_blob()}")) {
      con.setAutoCommit(false); // ref cursors only work if auto commit is off
      call.registerOutParameter(1, Types.REF_CURSOR);
      call.execute();
      try (ResultSet rs = (ResultSet) call.getObject(1)) {
        rs.setFetchSize(50);
        while (rs.next()) {
          cnt++;
        }
      }
      assertEquals(101, cnt);
    }
  }

@vlsi vlsi force-pushed the refcursor_close branch 2 times, most recently from d5d771d to acdb7dd Compare May 21, 2022 09:21
@vlsi
Copy link
Member Author

vlsi commented May 21, 2022

RefCursorFetchTest executes ~300+ tests and it takes ~30 seconds on my machine.
Frankly speaking, I think all the combinations make sense, and I do not think there's a trivial way to speed the test up.

It might be that reusing connections for the tests would help, however, it would likely require rework of BaseTest4 :-/

@davecramer
Copy link
Member

any reason we aren't merging this ?

@vlsi
Copy link
Member Author

vlsi commented May 23, 2022

I think it is fine

@vlsi vlsi merged commit 85f8581 into pgjdbc:master May 23, 2022
@vlsi
Copy link
Member Author

vlsi commented Jun 1, 2022

It might be this change brings a significant regression somehow. Apparently, GitHub Actions became way slower after this fix: ~40 min vs 6 hours.

CI duration

@davecramer
Copy link
Member

That's pretty significant. Thanks for tracking this down.

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