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

SQL Plan greater than 2048 characters is not printed at all in ISQL [CORE2370] #2792

Closed
firebird-automations opened this issue Mar 12, 2009 · 7 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: Bill Oliver (verbguy)

Assigned to: Claudio Valderrama C. (robocop)

Jira_subtask_outward CORE2509
Is related to QA394

I have a query plan with unions that is greater than 2048 characters, it is 5764 characters. The result is that in ISQL, I only get a blank line for the plan!

The trivial fix is to bump up the size of the buffer in ISQL's process_plan(). I increased buffer size to 8096, and it worked fine.

Perhaps a better fix is to max out the buffer size in process_plan(), so we don't have to touch this area again. We know that the maximum possible length of the plan is MAX_SSHORT, see dsql.cpp's DSQL_get_plan_info(). Anything longer than this, and the plan will be returned, but it will end in ellipses. That's ok.

I've coded a patch for this issue. In testing, it seems to me that dsql.cpp's put_item() length check is off by one, but I'd like the committer to double-check this.

#⁠#⁠#⁠ Eclipse Workspace Patch 1.0
#⁠P firebird2
Index: src/dsql/dsql.cpp

RCS file: /cvsroot/firebird/firebird2/src/dsql/dsql.cpp,v
retrieving revision 1.276
diff -u -r1.276 dsql.cpp
--- src/dsql/dsql.cpp 3 Mar 2009 14:55:53 -0000 1.276
+++ src/dsql/dsql.cpp 12 Mar 2009 17:59:18 -0000
@@ -2740,7 +2740,7 @@
const UCHAR* const end)
{

- if (ptr + length + 3 >= end) {
+ if (ptr + length + 3 > end) {
*ptr = isc_info_truncated;
return NULL;
}
Index: src/isql/isql.epp

RCS file: /cvsroot/firebird/firebird2/src/isql/isql.epp,v
retrieving revision 1.268
diff -u -r1.268 isql.epp
--- src/isql/isql.epp 1 Mar 2009 15:42:18 -0000 1.268
+++ src/isql/isql.epp 12 Mar 2009 17:59:18 -0000
@@ -7943,11 +7943,11 @@
// any result to the caller.
static void process_plan()
{
- // Bug 7565: A plan larger than plan_buffer will not be displayed
- // Bug 7565: Note also that the plan must fit into Print_Buffer
-
const SCHAR plan_info[] = { isc_info_sql_get_plan };
- TEXT plan_buffer[PRINT_BUFFER_LENGTH];
+
+ // max length of plan is MAX_SSHORT
+ // add 1 for SQL info item, and 2 for actual length of the plan
+ TEXT plan_buffer[MAX_SSHORT + 3];
memset(plan_buffer, 0, sizeof(plan_buffer));
if (isc_dsql_sql_info(isc_status, &global_Stmt, sizeof(plan_info), plan_info,
sizeof(plan_buffer), plan_buffer))

Commits: 1faf0f0

@firebird-automations
Copy link
Collaborator Author

Modified by: @dyemanov

Fix Version: 2.5 RC1 [ 10300 ]

assignee: Dmitry Yemanov [ dimitr ]

@firebird-automations
Copy link
Collaborator Author

Commented by: Claudio Valderrama C. (robocop)

First, put_item() is correct, because it's reserving space for isc_info_end and isc_info_sql_describe_end. Look at the functions that use it. When you are going to finish the string, if you discover there's no room for a single byte, it's too late to put even isc_info_truncated.

Second, I have enlarged the internal interface to take plans of any length in practice (really, up to 2^31-1), but there are some public API calls that I cannot change, so these places appear as constraints with a maximum of 2^16 (USHORT).

Third, the amount of places where temporary buffers are used to retrieve plan information is really daunting, plus several possibilities of internal retries (with bigger buffers). The sole call stack is a mountain. I don't dare to touch that for now, but some day it should be streamlined.

@firebird-automations
Copy link
Collaborator Author

Modified by: Claudio Valderrama C. (robocop)

assignee: Dmitry Yemanov [ dimitr ] => Claudio Valderrama C. [ robocop ]

@firebird-automations
Copy link
Collaborator Author

Commented by: Claudio Valderrama C. (robocop)

Tried to be conservative for versions <= FB2.1 (less than 32K) and use the full plan length in FB2.5 (around 64K).

@firebird-automations
Copy link
Collaborator Author

Modified by: Claudio Valderrama C. (robocop)

status: Open [ 1 ] => Resolved [ 5 ]

resolution: Fixed [ 1 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pcisar

Link: This issue is related to QA394 [ QA394 ]

@firebird-automations
Copy link
Collaborator Author

Modified by: @pavel-zotov

QA Status: No test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment