Issue Details (XML | Word | Printable)

Key: CORE-3680
Type: Bug Bug
Status: Open Open
Priority: Major Major
Assignee: Vlad Khorsun
Reporter: Jason Wharton
Votes: 0
Watchers: 5
Operations

If you were logged in you would be able to see more operations.
Firebird Core

EXECUTE BLOCK statement and ISC_DSQL_EXECUTE2() problem.

Created: 30/Nov/11 05:38 PM   Updated: 10/Dec/11 06:33 PM
Component/s: API / Client Library, Engine
Affects Version/s: 2.5.1
Fix Version/s: None

Environment: Using IB Objects in a Win32 environment.


 Description  « Hide
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



 All   Comments   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Vlad Khorsun added a comment - 30/Nov/11 06:37 PM
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() ?

Jason Wharton added a comment - 30/Nov/11 07:21 PM
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.

Vlad Khorsun added a comment - 30/Nov/11 08:04 PM
For (a) - are you sure you don't call isc_dsql_free_stateement() for SELECT statement ?

Jason Wharton added a comment - 30/Nov/11 08:13 PM
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.


Vlad Khorsun added a comment - 30/Nov/11 08:16 PM
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...

Jason Wharton added a comment - 30/Nov/11 08:30 PM
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!

Vlad Khorsun added a comment - 30/Nov/11 08:41 PM
> 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.

Jason Wharton added a comment - 30/Nov/11 09:54 PM
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?

Sean Leyne added a comment - 30/Nov/11 10:03 PM
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).

Adriano dos Santos Fernandes added a comment - 30/Nov/11 10:24 PM
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!

Jason Wharton added a comment - 30/Nov/11 10:37 PM
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?

Jason Wharton added a comment - 30/Nov/11 10:43 PM
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.


Vlad Khorsun added a comment - 01/Dec/11 01:52 PM
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.

Vlad Khorsun added a comment - 01/Dec/11 01:55 PM
Adriano,

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

Vlad Khorsun added a comment - 05/Dec/11 01:54 PM
The fix is committed into v2.5.2
Please, try next snapshot and report back if it is ok or not.

Jason Wharton added a comment - 07/Dec/11 04:33 PM - edited
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.

Vlad Khorsun added a comment - 08/Dec/11 04:14 PM
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.

Jason Wharton added a comment - 08/Dec/11 04:37 PM - edited
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!