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

EXECUTE BLOCK statement and ISC_DSQL_EXECUTE2() problem. [CORE3680] #4030

Open
firebird-automations opened this issue Nov 30, 2011 · 19 comments

Comments

@firebird-automations
Copy link
Collaborator

Submitted by: @jasonwharton

It seems that Firebird 2.5 has an issue where repeated executions of an
EXECUTE BLOCK statement are not possible via ISC_DSQL_EXECUTE2() due to the
error:

"Attempt to reopen an open cursor"

Here is a trace in IB Objects (using a TIB_DSQL component) of what happened:

/*---
ALLOCATE STATEMENT
DB_HANDLE = 1
STMT_HANDLE = 188
----*/
/*---
PREPARE STATEMENT
TR_HANDLE = 186
STMT_HANDLE = 188

EXECUTE BLOCK
RETURNS (I INTEGER)
AS

BEGIN
SELECT FIRST 1 MON$ATTACHMENT_ID FROM MON$ATTACHMENTS
INTO :I;
EXIT;
END

PLAN (MON$ATTACHMENTS NATURAL)

FIELDS = [ Version 1 SQLd 1 SQLn 30
I = <NIL> ]

SECONDS = 0.016
----*/
/*---
STATEMENT INFO
STMT_HANDLE = 188
----*/
/*---
STATEMENT INFO
STMT_HANDLE = 188
----*/
/*---
EXECUTE2 ROW SINGLETON
TR_HANDLE = 186
STMT_HANDLE = 188
PARAMS = [ ]
FIELDS = [ Version 1 SQLd 1 SQLn 1
I = 53765 ]
----*/
/*---
EXECUTE2 ROW SINGLETON
TR_HANDLE = 186
STMT_HANDLE = 188
PARAMS = [ ]
FIELDS = [ Version 1 SQLd 1 SQLn 1
I = 53765 ]

ERRCODE = 335544569
----*/
/*---
INTERPRET BUFFER =

ERRCODE = 17
----*/
/*---
INTERPRET BUFFER = Dynamic SQL Error

ERRCODE = 21
----*/
/*---
INTERPRET BUFFER = SQL error code = -502

ERRCODE = 32
----*/
/*---
INTERPRET BUFFER = Attempt to reopen an open cursor

ERRCODE = -1
----*/
/*---
COMMIT
TR_HANDLE = 186
----*/

The call shown as EXECUTE2 ROW SINGLETON is the isc_dsql_execute2() API call.

Thanks to anyone who can do a low-level verification of this. It seems to me there shouldn't be an error here.

Thanks,
Jason Wharton

Commits: 58ec672

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

What happens if you
a) repeat execution of SELECT statement using ISC_DSQL_EXECUTE2() call ?
b) close cursor (using isc_dsql_free_stateement(..., DSQL_close)) between second and all next calls of ISC_DSQL_EXECUTE2() ?

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Vlad,

a) It works just great! No cursor seems to be left open that causes a problem.
b) Again, it works without problem when this is done.

My conclusion is the Firebird engine is opening a cursor by mistake.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

For (a) - are you sure you don't call isc_dsql_free_stateement() for SELECT statement ?

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Yes, I'm positive because I had to take code from the TIB_Dataset class level and bring it to the TIB_Statement level so that I could implement this workaround to explicitly close the cursor after executing the statement that I'll show you here:

procedure TIB_Statement.API_Execute2;
var
ii: integer;
CursorOpenErr: boolean;
begin
with IB_Session do
begin
CursorOpenErr := false;
errcode := API_QuickFetch( true );
if ( errcode <> 0 ) then
begin
if ( errcode = isc_stream_eof ) and
( TreatSingletonEmptySetAsEof ) and
( StatementType in [ stSelect, stSelectForUpdate ] ) then
errcode := 100
else
if ( errcode = isc_dsql_error ) then
begin
// When executing an EXECUTE BLOCK statement it opens a cursor by mistake.
for ii := Low( IB_Session.Status ) to High( IB_Session.Status ) do
begin
if IB_Session.Status[ii] = isc_dsql_cursor_open_err then
begin
CursorOpenErr := true;
Break;
end;
end;
if CursorOpenErr then
begin
API_CloseCursor;
errcode := API_QuickFetch( true );
if ( errcode <> 0 ) then
HandleException( Self );
end
else
HandleException( Self );
end
else
HandleException( Self );
end;
end;
end;

I never needed to do this before.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Well, seems i found where SELECT is different from selectable EXECUTE BLOCK.

SELECT statement is considered as singleton if it is executed using isc_dsql_execute2() with not NULL output parameters.
In this case cursor are not created.

The question is : should we interpret EXECUTE BLOCK as singleton in the same conditions...

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

The issue here seems to be a question of what behavior should the isc_dsql_execute2() call have. I don't think it is appropriate under any circumstances to automatically create a cursor. The purpose of a statement such as this is to efficiently execute over and over again without the necessity for chatter on the wire to close cursor, open cursor, fetch record, etc. Hopefully this can be improved such that the EXECUTE BLOCK will have the same behavior as a regular SELECT statement. Also, I'm curious about the NOT NULL or NULL columns having an influence on something being handled as a singleton or not. This seems like something that should not have an influence. Perhaps you can explain that a little more? Thanks!

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

> Also, I'm curious about the NOT NULL or NULL columns having an influence on something being handled as a singleton or not. This seems like something that should not have an influence. Perhaps you can explain that a little more? Thanks!

It was not about SQL NULL's :) It was about value of last parameter of isc_dsql_execute2() call (namely out_sqlda).

Inside the engine we have no separate isc_dsql_execute() and isc_dsql_execute2() calls. There is one common routine with signature of isc_dsql_execute2().

When last parameter (out_sqlda) is not set by user application (i.e. NULL\nil was passed into isc_dsql_execute2() or isc_dsql_execute() was used) then call
is considered as non-singleton and cursor is created.

When out_sqlda is non-empty, call is considered as singleton for SELECT statement, but (wrongly?) not for EXECUTE BLOCK statement.

Hope it is clear now.

@firebird-automations
Copy link
Collaborator Author

Modified by: @hvlad

assignee: Vlad Khorsun [ hvlad ]

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Ok, your explanation makes sense. This is how it tells the difference between isc_dsql_execute() and isc_dsql_execute2(). If there is an Out_DA parameter then it takes it as a singleton.

Do you agree in this case the EXECUTE BLOCK statement should also be treated as a singleton? Will this be difficult to fix?

@firebird-automations
Copy link
Collaborator Author

Commented by: Sean Leyne (seanleyne)

I do not believe that EXECUTE BLOCK should be treated as a singleton. I have created EXECUTE BLOCK which has returned a set of rows (a simple one time report based on complex logic).

@firebird-automations
Copy link
Collaborator Author

Commented by: @asfernandes

While on that, I have some memories that in old days, execute block with output parameters and without suspend (as below) was considered as singleton and returned one row in isql. But it don't now.

Was this changed or am I wrong?

execute block returns (n integer)
as
begin
end!

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Sean,

EXECUTE BLOCK wouldn't be treated as a singleton in all cases. This is only in the case where it is executed by the isc_dsql_execute2() API call. The point here is to get consistent behavior for all calls to isc_dsql_execute2() such that it will deliver a single row of output only or return an exception and also to not have any cursor created on the server. This would cause it to behave just like a regular SELECT statement when using this API call. The change I am requesting would not prevent you from opening a cursor and using it exactly as you have always used it in the past. Does this help clarify what I am asking or are you still not clear?

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Adriano,

I can't answer your question directly because I don't know how all of the internals work. This is an area of working with Firebird that I would like clarified.

If SUSPEND is not called and a cursor has been opened up for it then it should be treated as an empty result set.
If SUSPEND is not called then it could yet be treated as a singleton if called as such via isc_dsql_execute2().

This is how I request this be handled so that there is versatility in dealing with singleton's within the existing architecture of Firebird.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Jason,

> Do you agree in this case the EXECUTE BLOCK statement should also be treated as a singleton?
To be consistent - yes.

> Will this be difficult to fix?
The fix is easy.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Adriano,

IIRC, initial implementation was changed at v2.5 to be more consistent with more strict rules of usage of SUSPEND in PSQL

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

The fix is committed into v2.5.2
Please, try next snapshot and report back if it is ok or not.

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Ok, I got the latest snapshot and had a look at things.

This statement works perfectly.

EXECUTE BLOCK
RETURNS (I INTEGER)
AS
BEGIN
SELECT FIRST 1 MON$ATTACHMENT_ID FROM MON$ATTACHMENTS
INTO :I;
SUSPEND;
END

However, this statement returns the error:
attempt to fetch past the last record in a record stream

EXECUTE BLOCK
RETURNS (I INTEGER)
AS
BEGIN
SELECT FIRST 1 MON$ATTACHMENT_ID FROM MON$ATTACHMENTS
INTO :I;
EXIT;
END

When you are using a statement in the isc_dsql_execute2() mode, should it require a suspend whenever data is returned? In the past the procedure was treated as a singleton such that the use of SUSPEND was not required.

In my opinion, even if Firebird departs a bit from the standards, I think there is value in maintaining legacy compatibility. I suggest that if a SUSPEND is not in the body of the procedure block that its output results be returned as a singleton record automatically. Kind of like an implicit SUSPEND if the programmer neglects putting it in. Also, this should be sensitive to the API call being made. It should only apply the implicit SUSPEND when the execute2() singleton context exists.

@firebird-automations
Copy link
Collaborator Author

Commented by: @hvlad

Create procedure

CREATE PROCEDURE P1
RETURNS (I INT)
AS
BEGIN
I = 1;
END

and try to excecute SELECT * FROM P1 using isc_dsql_execute2().
You will get the same error.
This is exact analoug of EXECUTE BLOCK you are complaining about.

The rule about EXECUTE BLOCK is :
if it have output parameters then it is selectable and engine prepare and run it as SELECT,
else it is executable (non-selectable) and engine prepare and run it as EXECUTE PROCEDURE.

The principal difference between selectable and non-selectable procedures (and blocks) is that selectable ones returns records when SUSPEND is encountred *and* one more record on exit.
The last record not corresponds to any SUSPEND statement and contains EOS (end-of-stream) mark.
The non-selectable procedures and blocks always returns exactly one record (i speak about engine internals, not about client-side fetches).

The singleton check, performed inside the engine, have the following logic:
1. receive a record from request and move it into output message (it will be results)
2. try to receive next record - it must be EOS record
if there is no record, report isc_stream_eof - because previous record was EOS record and singletone check is failed
3. else try to receive next record
if there is no record - singleton check is passed OK
else report isc_sing_select_err

It is not possible to change generated code for EXECUTE BLOCK when execution is started and engine know about presence of singleton check.
Therefore is it impossible to make it "sensitive to the API call being made".

And, yes, selectable EXECUTE BLOCK *must* have SUSPEND inside.

@firebird-automations
Copy link
Collaborator Author

Commented by: @jasonwharton

Vlad,

Thank for your giving the detailed explanation of the server internals.

If I understand you correctly, there isn't a time when an EXECUTE BLOCK with output parameters will work properly if there isn't a SUSPEND in the body of it.

Wouldn't it be possible to detect that there wasn't a SUSPEND token in the body of an EXECUTE BLOCK (with output parameters) and therefore to automatically append an implicit SUSPEND at the end of it? This way the values of the records for output will be appropriately passed on as a singleton select.

(I realize by now this is merely a request for enhancement and that you have addressed the issue of this entry 100% with what you have already done.)

Note: I'll go ahead and make an enhancement request so that this ticket can be closed out as resolved. Big THANK YOU for your efforts here!

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

No branches or pull requests

2 participants