public inbox for [email protected]
help / color / mirror / Atom feed[PATCH] Fix FetchRefcursors issues
2+ messages / 1 participants
[nested] [flat]
* [PATCH] Fix FetchRefcursors issues
@ 2022-01-10 01:49 Adrian Grucza <[email protected]>
2022-05-09 02:31 ` Re: [PATCH] Fix FetchRefcursors issues Adrian Grucza <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Adrian Grucza @ 2022-01-10 01:49 UTC (permalink / raw)
To: [email protected]
Hi all,
I've created a patch (attached) that fixes some issues with the automatic
fetching of refcursors from a function/procedure (the
FetchRefcursors setting). It includes updates to the tests.
Below is a summary of the patch:
1. Fix crash if error occurs when fetching noninitial refcursor
2. Fix error when fetching NULL refcursor
3. Return empty result if all refcursors are NULL
Regards,
Adrian Grucza
Technical Lead
Tel: +61390185800
[email protected]
www.iress.com
Level 16 385 Bourke St
Melbourne, Victoria, 3000
The contents of this email originated from Iress. For this purpose Iress includes Iress Limited and/or any of its subsidiaries, holding companies and trading entities. If you have received this email in error please notify the sender immediately and delete this email.
nosig
Attachments:
[application/octet-stream] Fix-FetchRefcursors-issues.patch (5.9K, 3-Fix-FetchRefcursors-issues.patch)
download | inline diff:
From 4688179c11badff80a603600908096e1b333a760 Mon Sep 17 00:00:00 2001
From: Adrian Grucza <[email protected]>
Date: Mon, 10 Jan 2022 12:24:00 +1100
Subject: [PATCH 1/2] Fix FetchRefcursors issues 1. Fix crash if error occurs
when fetching noninitial refcursor 2. Fix error when fetching NULL refcursor
3. Return empty result if all refcursors are NULL
---
statement.c | 20 +++++++++++++++-----
test/expected/fetch-refcursors.out | 36 ++++++++++++++++++++++++++++++++----
test/src/fetch-refcursors-test.c | 29 +++++++++++++++++++----------
3 files changed, 66 insertions(+), 19 deletions(-)
diff --git a/statement.c b/statement.c
index e7f816f..f45cd79 100644
--- a/statement.c
+++ b/statement.c
@@ -2304,6 +2304,7 @@ MYLOG(DETAIL_LOG_LEVEL, "!!SC_fetch return =%d\n", ret);
{
char fetch[128];
QResultClass *last = NULL, *res;
+ BOOL refcursor_found = FALSE;
/* Iterate the columns in the result to look for refcursors */
numcols = QR_NumResultCols(rhold.first);
@@ -2318,7 +2319,12 @@ MYLOG(DETAIL_LOG_LEVEL, "!!SC_fetch return =%d\n", ret);
break;
}
+ refcursor_found = TRUE;
STR_TO_NAME(self->cursor_name, QR_get_value_backend_text(rhold.first, 0, i));
+ /* Skip NULL refcursors (allows procedure to return a variable number of results) */
+ if (!SC_cursor_is_valid(self))
+ continue;
+
SC_set_fetchcursor(self);
qi.result_in = NULL;
qi.cursor = SC_cursor_name(self);
@@ -2339,19 +2345,23 @@ MYLOG(DETAIL_LOG_LEVEL, "!!SC_fetch return =%d\n", ret);
QR_concat(last, res);
self->multi_statement = TRUE;
}
+ last = res;
if (!QR_command_maybe_successful(res))
{
SC_set_errorinfo(self, res, 0);
- QR_Destructor(rhold.first);
break;
}
-
- last = res;
}
}
}
- if (last)
- QR_Destructor(rhold.first);
+ if (refcursor_found)
+ {
+ /* Discard original result */
+ if (NULL == last)
+ SC_set_Result(self, QR_Constructor()); /* return empty result */
+ else
+ QR_Destructor(rhold.first);
+ }
}
}
cleanup:
diff --git a/test/expected/fetch-refcursors.out b/test/expected/fetch-refcursors.out
index fff9ee1..bfe6f85 100644
--- a/test/expected/fetch-refcursors.out
+++ b/test/expected/fetch-refcursors.out
@@ -2,19 +2,47 @@ Creating procedure 'refproc'
connected
disconnecting
--- TEST using FetchRefcursors=0 and SQL_ATTR_AUTOCOMMIT=1
+-- TEST using FetchRefcursors=0, autocommit=1, numresults=2
connected
Output param num_cursor is 2
--1 Result set:
-2 ref1 ref2
+2 <unnamed portal 1> <unnamed portal 2>
disconnecting
--- TEST using FetchRefcursors=1 and SQL_ATTR_AUTOCOMMIT=1
+-- TEST using FetchRefcursors=1, autocommit=1, numresults=2
connected
SQLExecute failed
HY000=Query must be executed in a transaction when FetchRefcursors setting is enabled.
--- TEST using FetchRefcursors=1 and SQL_ATTR_AUTOCOMMIT=0
+-- TEST using FetchRefcursors=1, autocommit=0, numresults=0
+connected
+Output param num_cursor is 0
+--1 Result set:
+disconnecting
+
+-- TEST using FetchRefcursors=1, autocommit=0, numresults=1
+connected
+Output param num_cursor is 1
+--1 Result set:
+1 foo
+2 bar
+3 foobar
+disconnecting
+
+-- TEST using FetchRefcursors=1, autocommit=0, numresults=2
+connected
+Output param num_cursor is 2
+--1 Result set:
+1 foo
+2 bar
+3 foobar
+--2 Result set:
+foobar 3
+bar 2
+foo 1
+disconnecting
+
+-- TEST using FetchRefcursors=1, autocommit=0, numresults=3
connected
Output param num_cursor is 2
--1 Result set:
diff --git a/test/src/fetch-refcursors-test.c b/test/src/fetch-refcursors-test.c
index ed9a47a..01e58e4 100644
--- a/test/src/fetch-refcursors-test.c
+++ b/test/src/fetch-refcursors-test.c
@@ -33,13 +33,19 @@ static void setup_procedure()
CHECK_CONN_RESULT(rc, "failed to allocate stmt handle", conn);
rc = SQLExecDirect(hstmt, "create or replace procedure refproc"
- "(inout num_cursor integer, inout ref1 refcursor default 'ref1', inout ref2 refcursor default 'ref2') as "
+ "(inout num_cursor integer, inout ref1 refcursor default null, inout ref2 refcursor default null) as "
"$procedure$ \n"
"DECLARE \n"
"BEGIN \n"
- "num_cursor := 2; \n"
- "OPEN ref1 FOR SELECT id, t FROM testtab1 ORDER BY id ASC; \n"
- "OPEN ref2 FOR SELECT t, id FROM testtab1 ORDER BY id DESC; \n"
+ "IF num_cursor > 0 THEN \n"
+ " OPEN ref1 FOR SELECT id, t FROM testtab1 ORDER BY id ASC; \n"
+ "END IF; \n"
+ "IF num_cursor > 1 THEN \n"
+ " OPEN ref2 FOR SELECT t, id FROM testtab1 ORDER BY id DESC; \n"
+ "END IF; \n"
+ "IF num_cursor > 2 THEN \n"
+ " num_cursor := 2; \n"
+ "END IF; \n"
"END; \n"
"$procedure$ \n"
"LANGUAGE plpgsql\n"
@@ -52,13 +58,13 @@ static void setup_procedure()
test_disconnect();
}
-static void refcursor_test(char* connectparams, SQLUINTEGER autocommit)
+static void refcursor_test(char* connectparams, SQLUINTEGER autocommit, int numresults)
{
SQLRETURN rc;
HSTMT hstmt = SQL_NULL_HSTMT;
- int num_cursor = 0;
+ int num_cursor = numresults;
- printf("\n-- TEST using %s and SQL_ATTR_AUTOCOMMIT=%u\n", connectparams, autocommit);
+ printf("\n-- TEST using %s, autocommit=%u, numresults=%d\n", connectparams, autocommit, numresults);
test_connect_ext(connectparams);
@@ -102,9 +108,12 @@ int main(int argc, char **argv)
{
setup_procedure();
- refcursor_test("FetchRefcursors=0", SQL_AUTOCOMMIT_ON);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_ON);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF);
+ refcursor_test("FetchRefcursors=0", SQL_AUTOCOMMIT_ON, 2);
+ refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_ON, 2);
+ refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 0);
+ refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 1);
+ refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 2);
+ refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 3);
return 0;
}
--
2.13.0.windows.1
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [PATCH] Fix FetchRefcursors issues
2022-01-10 01:49 [PATCH] Fix FetchRefcursors issues Adrian Grucza <[email protected]>
@ 2022-05-09 02:31 ` Adrian Grucza <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Adrian Grucza @ 2022-05-09 02:31 UTC (permalink / raw)
To: [email protected]
Hi all,
I've attached an additional patch that fixes a pre-existing bug where
cursors were being freed prematurely when there are multiple results that
use cursors. It can happen when the fetch cache size is smaller than the
number of rows being fetched. To ensure the bug doesn't resurface, the
patch includes updates to the test that was modified in my previous patch
(Fix-FetchRefcursors-issues.patch).
Some details about this patch:
The patch adds a condition to skip existing code that closes all subsequent
cursors when the current one gets closed. It does not seem logical that
closing one cursor should cause others to also be closed. The code was
originally added in commit c07342d22d82ea6293d27057840babfc2ff6d750 to fix
a crash when using SQL Server linked servers, but it's still not clear
exactly why it was needed, because the change that prevents a crash seems
to be in a different file (parse.c).
I was tempted to remove the code completely because there are no tests that
rely on its existence, but that may be because testing it would have
required integration with SQL Server. So I've assumed that the code is only
required for SQL Server, and therefore made it run only for SQL Server.
Regards,
Adrian Grucza
Technical Lead
Tel: +61390185800
[email protected]
www.iress.com
Level 16 385 Bourke St
Melbourne, Victoria, 3000
The contents of this email originated from Iress. For this purpose Iress includes Iress Limited and/or any of its subsidiaries, holding companies and trading entities. If you have received this email in error please notify the sender immediately and delete this email.
nosig
Attachments:
[application/octet-stream] Prevent-cursors-being-closed-prematurely.patch (4.1K, 3-Prevent-cursors-being-closed-prematurely.patch)
download | inline diff:
From 27af474c9795069e4e05716084e877ff2aaeb526 Mon Sep 17 00:00:00 2001
From: Adrian Grucza <[email protected]>
Date: Mon, 9 May 2022 11:49:09 +1000
Subject: [PATCH 2/2] Prevent cursors being closed prematurely
---
qresult.c | 22 ++++++++++++++++------
test/expected/fetch-refcursors.out | 12 ++++++------
test/src/fetch-refcursors-test.c | 13 +++++++------
3 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/qresult.c b/qresult.c
index 40ad3b7..f4e5fd4 100644
--- a/qresult.c
+++ b/qresult.c
@@ -99,14 +99,24 @@ QR_set_cursor(QResultClass *self, const char *name)
}
else
{
- QResultClass *res;
-
self->cursor_name = NULL;
- for (res = QR_nextr(self); NULL != res; res = QR_nextr(res))
+
+ /*
+ * The isSqlServr() check below was added because the code was freeing
+ * cursors prematurely when other results with open cursors exist. The
+ * code was originally added for a scenario using SQL Server linked
+ * servers in commit c07342d22d82ea6293d27057840babfc2ff6d750.
+ */
+ if (isSqlServr())
{
- if (NULL != res->cursor_name)
- free(res->cursor_name);
- res->cursor_name = NULL;
+ QResultClass *res;
+
+ for (res = QR_nextr(self); NULL != res; res = QR_nextr(res))
+ {
+ if (NULL != res->cursor_name)
+ free(res->cursor_name);
+ res->cursor_name = NULL;
+ }
}
}
}
diff --git a/test/expected/fetch-refcursors.out b/test/expected/fetch-refcursors.out
index bfe6f85..a594d4d 100644
--- a/test/expected/fetch-refcursors.out
+++ b/test/expected/fetch-refcursors.out
@@ -2,25 +2,25 @@ Creating procedure 'refproc'
connected
disconnecting
--- TEST using FetchRefcursors=0, autocommit=1, numresults=2
+-- TEST using Fetch=1;FetchRefcursors=0, autocommit=1, numresults=2
connected
Output param num_cursor is 2
--1 Result set:
2 <unnamed portal 1> <unnamed portal 2>
disconnecting
--- TEST using FetchRefcursors=1, autocommit=1, numresults=2
+-- TEST using Fetch=1;FetchRefcursors=1, autocommit=1, numresults=2
connected
SQLExecute failed
HY000=Query must be executed in a transaction when FetchRefcursors setting is enabled.
--- TEST using FetchRefcursors=1, autocommit=0, numresults=0
+-- TEST using Fetch=1;FetchRefcursors=1, autocommit=0, numresults=0
connected
Output param num_cursor is 0
--1 Result set:
disconnecting
--- TEST using FetchRefcursors=1, autocommit=0, numresults=1
+-- TEST using Fetch=1;FetchRefcursors=1, autocommit=0, numresults=1
connected
Output param num_cursor is 1
--1 Result set:
@@ -29,7 +29,7 @@ Output param num_cursor is 1
3 foobar
disconnecting
--- TEST using FetchRefcursors=1, autocommit=0, numresults=2
+-- TEST using Fetch=1;FetchRefcursors=1, autocommit=0, numresults=2
connected
Output param num_cursor is 2
--1 Result set:
@@ -42,7 +42,7 @@ bar 2
foo 1
disconnecting
--- TEST using FetchRefcursors=1, autocommit=0, numresults=3
+-- TEST using Fetch=1;FetchRefcursors=1, autocommit=0, numresults=3
connected
Output param num_cursor is 2
--1 Result set:
diff --git a/test/src/fetch-refcursors-test.c b/test/src/fetch-refcursors-test.c
index 01e58e4..9453949 100644
--- a/test/src/fetch-refcursors-test.c
+++ b/test/src/fetch-refcursors-test.c
@@ -108,12 +108,13 @@ int main(int argc, char **argv)
{
setup_procedure();
- refcursor_test("FetchRefcursors=0", SQL_AUTOCOMMIT_ON, 2);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_ON, 2);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 0);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 1);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 2);
- refcursor_test("FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 3);
+ /* Using a fetch cache size of 1 to test multiple fetches per cursor */
+ refcursor_test("Fetch=1;FetchRefcursors=0", SQL_AUTOCOMMIT_ON, 2);
+ refcursor_test("Fetch=1;FetchRefcursors=1", SQL_AUTOCOMMIT_ON, 2);
+ refcursor_test("Fetch=1;FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 0);
+ refcursor_test("Fetch=1;FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 1);
+ refcursor_test("Fetch=1;FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 2);
+ refcursor_test("Fetch=1;FetchRefcursors=1", SQL_AUTOCOMMIT_OFF, 3);
return 0;
}
--
2.35.1.windows.2
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2022-05-09 02:31 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-01-10 01:49 [PATCH] Fix FetchRefcursors issues Adrian Grucza <[email protected]>
2022-05-09 02:31 ` Adrian Grucza <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox