public inbox for [email protected]
help / color / mirror / Atom feedFrom: Adrian Grucza <[email protected]>
To: [email protected]
Subject: Re: [PATCH] Fix FetchRefcursors issues
Date: Mon, 9 May 2022 12:31:48 +1000
Message-ID: <CADF4wWqd4a=rRD27a2umxq0EFFog160d0fJrTxrS=74O7hQMRg@mail.gmail.com> (raw)
In-Reply-To: <CADF4wWr+n2FfqcKse-1MRXY7LsXuuNnH_9p44RuQMh=Jq40VLg@mail.gmail.com>
References: <CADF4wWr+n2FfqcKse-1MRXY7LsXuuNnH_9p44RuQMh=Jq40VLg@mail.gmail.com>
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
view thread (2+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: [PATCH] Fix FetchRefcursors issues
In-Reply-To: <CADF4wWqd4a=rRD27a2umxq0EFFog160d0fJrTxrS=74O7hQMRg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox