feat: fetch size for RefCursor #2976

davecramer · 2023-11-01T15:40:01Z

This replaces the original PR #925 originally written by @rhavermans It was easier to pull in the changes from his PR instead of rebasing the original. This is entirely his code.

We now support setting the fetch size on a RefCursor.

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:

Does your submission pass tests?
Does ./gradlew autostyleCheck checkstyleAll pass ?
Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

Does this break existing behaviour? If so please explain.
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?

vlsi · 2023-11-07T04:51:43Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

+    // ignore request to set fetchSize to 0 if it is currently > 0
+    if (fetchSize > 0 && rows == 0) {
+      return;
+    }


Could you please clarify why ignore the new fetch size?
I guess users might want to adjust fetchSize as they fetch, so it might be unexpected to ignore the new values.

If you set the fetchSize to 0 here we will stop fetching more data.

We should treat fetchSize like a hint rather than an "exact value to use for fetching". Then 0 would not cause "0 rows to fetch"

The initial value for fetchSize is 0, so ignoring setFetchSize(0) sounds arbitrary

fetch size of zero is specifically called out as special If the fetch size specified is zero, the JDBC driver ignores the value and is free to make its own best guess as to what the fetch size should be.

pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

vlsi · 2023-11-07T05:10:59Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

+      if (refCursorName == null || refCursorName.isEmpty()) {
+        castNonNull(resultCursor, "resultCursor");
+        connection.getQueryExecutor()
+            .fetch(resultCursor, new CursorResultHandler(), fetchRows, adaptiveFetch);
+        // .fetch(...) could update this.cursor, and cursor==null means
+        // there are no more rows to fetch

-      // .fetch(...) could update this.cursor, and cursor==null means
-      // there are no more rows to fetch
-      closeRefCursor();
+      } else {
+        StringBuilder sb = new StringBuilder("FETCH FORWARD ");
+        sb.append(fetchRows);
+        sb.append(" IN ");
+        castNonNull(refCursorName, "refCursorName");
+        Utils.escapeIdentifier(sb, refCursorName);
+        final Query cursorForward = connection.getQueryExecutor().createSimpleQuery(sb.toString());
+        // pass maxRows=0 since fetchSize has already been corrected to account for maxRows
+        connection.getQueryExecutor().execute(cursorForward, null, new CursorResultHandler(), 0, fetchSize, QueryExecutor.QUERY_ONESHOT | QueryExecutor.QUERY_SUPPRESS_BEGIN
+            | QueryExecutor.QUERY_EXECUTE_AS_SIMPLE );


I wonder if we can reuse the existing "extended protocol" fetch to fetch the data rather than build "fetch forward" commands dynamically

@rhavermans

This replaces the original PR pgjdbc#925 originally written by @rhavermans It was easier to pull in the changes from his PR instead of rebasing the original. This is entirely his code. We now support setting the fetch size on a RefCursor.

…to work

Update docs for new behaviour of refcursor

davecramer · 2023-11-15T18:02:13Z

@vlsi I want to include this in the release. Thoughts ?

vlsi · 2023-11-16T08:50:47Z

pgjdbc/src/main/java/org/postgresql/jdbc/PgResultSet.java

+      if (refCursorName == null || refCursorName.isEmpty()) {
+        castNonNull(resultCursor, "resultCursor");
+        connection.getQueryExecutor()
+            .fetch(resultCursor, new CursorResultHandler(), fetchRows, adaptiveFetch);
+        // .fetch(...) could update this.cursor, and cursor==null means
+        // there are no more rows to fetch

-      // .fetch(...) could update this.cursor, and cursor==null means
-      // there are no more rows to fetch
-      closeRefCursor();
+      } else {
+        StringBuilder sb = new StringBuilder("FETCH FORWARD ");
+        sb.append(fetchRows);
+        sb.append(" IN ");


I am afraid this change does not fix the issue.

It uses fetchRows from the base ResultSet rather than fetchRows of the ResultSet that is associated with refcursor in question.

In other words:

Imagine we call {? = call test_blob(?)}.
CallableStatement.execute would gather call results, and it would execute getObject at

pgjdbc/pgjdbc/src/main/java/org/postgresql/jdbc/PgCallableStatement.java

Line 131 in ca25d10

callResult[j] = rs.getObject(i + 1);

Then it would delegate to PgResultSet.getObject -> internalGetObject -> getRefCursor, and it would effectively use fetchSize from a temporary resultset associated with {? = call test_blob(?)}.

I believe we should add a test to ensure fetch forward is triggered at all.

Here are the logs from running

public void testFetchSize() throws SQLException { checkAndRunResult(3); }

2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.136 ms execute <unnamed>/C_1: select * from testspg__getRefcursor ($1) as result 2023-11-16 06:53:04.080 EST [84077] DETAIL: parameters: $1 = NULL 2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.008 ms parse <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.002 ms bind <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.039 ms execute <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.002 ms parse <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.001 ms bind <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.080 EST [84077] LOG: duration: 0.003 ms execute <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.002 ms parse <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.002 ms bind <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.001 ms execute <unnamed>: FETCH FORWARD 3 IN "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.006 ms parse <unnamed>: CLOSE "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.001 ms bind <unnamed>: CLOSE "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.002 ms execute <unnamed>: CLOSE "<unnamed portal 1>" 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.005 ms parse S_2: COMMIT 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.002 ms bind S_2: COMMIT 2023-11-16 06:53:04.081 EST [84077] LOG: duration: 0.006 ms execute S_2: COMMIT

vlsi · 2023-11-16T12:22:06Z

pgjdbc/src/test/java/org/postgresql/test/jdbc2/RefCursorTest.java

+    if (fetchSize != null) {
+      call.setFetchSize(fetchSize);
+    }


Frankly speaking, I would not expect call.setFetchSize to be inherited when fetching the refcursor.

I'm not sure why not ?

Statement.setFetchSize() docs say

Gives the JDBC driver a hint as to the number of rows that should be fetched from the database when more rows are needed for ResultSet objects generated by this Statement. If the value specified is zero, then the hint is ignored. The default value is zero. Params: rows – the number of rows to fetch Throws: SQLException – if a database access error occurs, this method is called on a closed Statement or the condition rows >= 0 is not satisfied.

How else would they be used ?

ResultSet.setFetchSize would be ignored though

Yes, however it is allowed.

Gives the JDBC driver a hint as to the number of rows that should be fetched from the database when more rows are needed for this ResultSet object. If the fetch size specified is zero, the JDBC driver ignores the value and is free to make its own best guess as to what the fetch size should be. The default value is set by the Statement object that created the result set. The fetch size may be changed at any time.

I don't see a way to change it once we have the statement. Does that currently work with portals ?

davecramer requested a review from vlsi November 2, 2023 19:37

davecramer mentioned this pull request Nov 3, 2023

Setting the fetch size on a CallableStatement does not work. #1926

Open

vlsi reviewed Nov 7, 2023

View reviewed changes

vlsi requested changes Nov 7, 2023

View reviewed changes

davecramer added 3 commits November 15, 2023 10:00

feat: fetch size for RefCursor

eba5985

This replaces the original PR pgjdbc#925 originally written by @rhavermans It was easier to pull in the changes from his PR instead of rebasing the original. This is entirely his code. We now support setting the fetch size on a RefCursor.

Had to change the test to not set ResultSet.setFetchSize(0) for this …

e90b9c6

…to work

Ignore ResultSet.setFetchSize(0) if the fetch size was set previously

27c842a

Update docs for new behaviour of refcursor

davecramer force-pushed the fix_refcursor_fetch branch from 968e2c4 to 27c842a Compare November 15, 2023 15:01

close refcursor unconditionally

2be046b

vlsi reviewed Nov 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fetch size for RefCursor #2976

feat: fetch size for RefCursor #2976

davecramer commented Nov 1, 2023

vlsi Nov 7, 2023

davecramer Nov 7, 2023

vlsi Nov 16, 2023

vlsi Nov 16, 2023

davecramer Nov 16, 2023

vlsi Nov 7, 2023

davecramer commented Nov 15, 2023

vlsi Nov 16, 2023

davecramer Nov 16, 2023

vlsi Nov 16, 2023

davecramer Nov 16, 2023

vlsi Nov 16, 2023

davecramer Nov 16, 2023

feat: fetch size for RefCursor #2976

Are you sure you want to change the base?

feat: fetch size for RefCursor #2976

Conversation

davecramer commented Nov 1, 2023

All Submissions:

New Feature Submissions:

Changes to Existing Features:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecramer commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment