Re: [Firebird-devel] Firebird 5 and Update...Returning
Tony Whyman wrote 26.11.2021 18:01: Note that only if FGoToFirstRecordOnExecute is true will the returned values be available immediately. Which (accidentally) is default and has no sense to be changed ever. -- WBR, SD. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
That code is very legacy and dates from a time when Update..Returning did not exist. Actually, it works the same as the modern version. My point was about what the user expects. If you execute an Update...Returning with the code you pasted in below, with Firebird 4.0.0 it drops through the SQLExecProcedure and the user can access the returned values immediately afterwards. In Firebird 5, the code executes the SQLSelect case. Note that only if FGoToFirstRecordOnExecute is true will the returned values be available immediately. Otherwise, the user has to call "Next" explicitly. That is why the change to Update...Returning can break user code. In order to try and maintain the TIBSQL semantics for Firebird 5, I need to be able to distinguish between a true select query, an Update...Returning that returns a singleton row and one that returns a cursor. I may be able to find an answer by experimenting with the new behaviour, but that will not stop me moaning that the extension of Update..Returning has been implemented without keeping faith with existing code. On 26/11/2021 16:47, Dimitry Sibiryakov wrote: Tony Whyman wrote 26.11.2021 17:13: Legacy code can often appear "insane" with the benefit of hindsight. The problem is that it exists and it is your starting point. You don't break it without good reason. Ok, here is legacy code from TIBSQL.ExecQuery: case FSQLType of SQLSelect: begin Call(FGDSLibrary.isc_dsql_execute2(StatusVector, TRHandle, @FHandle, Database.SQLDialect, FSQLParams.AsXSQLDA, nil), True); Call( FGDSLibrary.isc_dsql_set_cursor_name(StatusVector, @FHandle, PChar(FCursor), 0), True); FOpen := True; FBOF := True; FEOF := False; FRecordCount := 0; if FGoToFirstRecordOnExecute then Next; end; SQLExecProcedure: begin fetch_res := Call(FGDSLibrary.isc_dsql_execute2(StatusVector, TRHandle, @FHandle, Database.SQLDialect, FSQLParams.AsXSQLDA, FSQLRecord.AsXSQLDA), False); . and so on... As you can see it IS sane and won't be broken so if _current_ IBX code differs from it - the one who "improved" it must be blamed. Firebird team cannot help in this case. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
On 26-11-2021 17:13, Tony Whyman wrote: On 26/11/2021 15:43, Dimitry Sibiryakov wrote: Tony Whyman wrote 26.11.2021 16:32: What you appear to be saying is that you have changed/expanded the semantic of Update...Returning, changed the SQL Statement type returned and then not expected the change to break any existing code... Correction: any sane code. There is an old joke that starts with one person asking another the way to Edinburgh. The answer given is "I wouldn't start from here if I were you". That answer may be true, but very unhelpful. Legacy code can often appear "insane" with the benefit of hindsight. The problem is that it exists and it is your starting point. You don't break it without good reason. There is a good reason: supporting multi-row RETURNING instead of the limitation in FB 4.0 and earlier of only supporting singleton updates. And this is done in a way that query tools and drivers inspecting the statement type will still be able to process it appropriately. The reasoning is similar as why isc_info_sql_stmt_exec_procedure was used when RETURNING was introduced in Firebird 2.0[1]: """ If the RETURNING clause is present, then the statement is described as isc_info_sql_stmt_exec_procedure by the API (instead of isc_info_sql_stmt_insert), so the existing connectivity drivers should support this feature automagically. """ Maybe Firebird shouldn't have implemented RETURNING at all, but instead implemented the SQL standard way of doing this using the data change delta table: `select * from final table (update sometable set x = 'a')` (or `select * from new table (update sometable set x = 'a')` or `select * from old table (update sometable set x = 'a')`) However, the semantics of supporting data change delta table is a bit more complex (from a quick read of the standard). Mark [1]: https://firebirdsql.org/file/documentation/release_notes/html/rlsnotes207.html#dml-dsql-returning -- Mark Rotteveel Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
Tony Whyman wrote 26.11.2021 17:13: Legacy code can often appear "insane" with the benefit of hindsight. The problem is that it exists and it is your starting point. You don't break it without good reason. Ok, here is legacy code from TIBSQL.ExecQuery: case FSQLType of SQLSelect: begin Call(FGDSLibrary.isc_dsql_execute2(StatusVector, TRHandle, @FHandle, Database.SQLDialect, FSQLParams.AsXSQLDA, nil), True); Call( FGDSLibrary.isc_dsql_set_cursor_name(StatusVector, @FHandle, PChar(FCursor), 0), True); FOpen := True; FBOF := True; FEOF := False; FRecordCount := 0; if FGoToFirstRecordOnExecute then Next; end; SQLExecProcedure: begin fetch_res := Call(FGDSLibrary.isc_dsql_execute2(StatusVector, TRHandle, @FHandle, Database.SQLDialect, FSQLParams.AsXSQLDA, FSQLRecord.AsXSQLDA), False); . and so on... As you can see it IS sane and won't be broken so if _current_ IBX code differs from it - the one who "improved" it must be blamed. Firebird team cannot help in this case. -- WBR, SD. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
26.11.2021 17:47, Tony Whyman wrote: 3. IBX tries to be general purpose and so prepares a statement, then checks the statement type, and then calls the appropriate IStatement method. Then it shouldn't be broken. It gets isc_info_sql_stmt_select, calls openCursor() and everything works as intended. Changing the statement type semantics broke the logic used to determine which method to call. I still don't see how it was broken, sorry. See above. Dmitry Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
On 26/11/2021 15:43, Dimitry Sibiryakov wrote: Tony Whyman wrote 26.11.2021 16:32: What you appear to be saying is that you have changed/expanded the semantic of Update...Returning, changed the SQL Statement type returned and then not expected the change to break any existing code... Correction: any sane code. There is an old joke that starts with one person asking another the way to Edinburgh. The answer given is "I wouldn't start from here if I were you". That answer may be true, but very unhelpful. Legacy code can often appear "insane" with the benefit of hindsight. The problem is that it exists and it is your starting point. You don't break it without good reason. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
Tony Whyman wrote 26.11.2021 16:32: What you appear to be saying is that you have changed/expanded the semantic of Update...Returning, changed the SQL Statement type returned and then not expected the change to break any existing code... Correction: any sane code. If a singleton row is expected then there is no need to do either of the above. Right now, I am not sure how this behaviour can be maintained (and not get inundated by users complaining that their code is broken) without being able to tell the difference between an Update...Returning that returns a singleton row from one that returns a cursor. This behavior can be maintained exactly as it has been done: if statement type is isc_info_sql_stmt_exec_procedure - single row is expected, if statement type is isc_info_sql_stmt_select - multiple rows are expected. And IIRC TIBSQL's code was written exactly like this. No, openCursor() must be called here, not execute(). Which is, of course, new behaviour. No, it is the old behavior. If statement type is described as select - open cursor. If it is procedure call - get singleton. And once again: execute() should work as well as long as UPDATE is changing exactly one record. -- WBR, SD. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
On 26-11-2021 16:32, Tony Whyman wrote: What you appear to be saying is that you have changed/expanded the semantic of Update...Returning, changed the SQL Statement type returned and then not expected the change to break any existing code... The change was done in a way that code looking at the statement type to know how to execute it would not break, and without adding a new statement type, so libraries not updated for this would still be able to handle it correctly. See also the discussion with subject "isc_info_sql_stmt_type/isc_info_sql_stmt_flags" back on the 19th and 20th of August 2021. Mark -- Mark Rotteveel Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
What you appear to be saying is that you have changed/expanded the semantic of Update...Returning, changed the SQL Statement type returned and then not expected the change to break any existing code... On 26/11/2021 15:01, Dimitry Sibiryakov wrote: Tony Whyman wrote 26.11.2021 15:28: 3. Prior to calling IStatement.Execute, the statement type is checked. The current (IBX) code raises an exception if the sql statement type is isc_info_sql_stmt_select in order to stop the wrong IStatement method being called. So it is actually the test suite that makes (now wrong) assumption that UPDATE...RETURNING has to be described as isc_info_sql_stmt_exec_procedure is wrong. But it used to be the correct assumption. 3. IBX tries to be general purpose and so prepares a statement, then checks the statement type, and then calls the appropriate IStatement method. Changing the statement type semantics broke the logic used to determine which method to call. That's why the change. IBX should check the statement type (which now is isc_info_sql_stmt_select) and call appropriate method i.e. openCursor(). What's wrong? The problem is in TIBSQL. This is a class that goes all the way back to Borland days. It has a common method "ExecQuery" that is called regardless of the statement type. If it is used to open a cursor, then the user either has to set the GoToFirstRecordOnExecute property to ensure that the cursor is automatically positioned on the first row, or explicitly call "Next". If a singleton row is expected then there is no need to do either of the above. Right now, I am not sure how this behaviour can be maintained (and not get inundated by users complaining that their code is broken) without being able to tell the difference between an Update...Returning that returns a singleton row from one that returns a cursor. 4. With reference to item 1 above, if "Update..Returning" returns a statement type of isc_info_sql_stmt_select then you _do_ have to also parse the statement in IBX so that you know its an UPDATE and IStatement.execute has to be called instead of IStatement.openCursor. No, openCursor() must be called here, not execute(). Which is, of course, new behaviour. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
Dimitry Sibiryakov wrote 26.11.2021 16:01: 4. With reference to item 1 above, if "Update..Returning" returns a statement type of isc_info_sql_stmt_select then you _do_ have to also parse the statement in IBX so that you know its an UPDATE and IStatement.execute has to be called instead of IStatement.openCursor. No, openCursor() must be called here, not execute(). Actually, execute() also should work as long as the query updates only one record. -- WBR, SD. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
Tony Whyman wrote 26.11.2021 15:28: 3. Prior to calling IStatement.Execute, the statement type is checked. The current (IBX) code raises an exception if the sql statement type is isc_info_sql_stmt_select in order to stop the wrong IStatement method being called. So it is actually the test suite that makes (now wrong) assumption that UPDATE...RETURNING has to be described as isc_info_sql_stmt_exec_procedure is wrong. 3. IBX tries to be general purpose and so prepares a statement, then checks the statement type, and then calls the appropriate IStatement method. Changing the statement type semantics broke the logic used to determine which method to call. That's why the change. IBX should check the statement type (which now is isc_info_sql_stmt_select) and call appropriate method i.e. openCursor(). What's wrong? 4. With reference to item 1 above, if "Update..Returning" returns a statement type of isc_info_sql_stmt_select then you _do_ have to also parse the statement in IBX so that you know its an UPDATE and IStatement.execute has to be called instead of IStatement.openCursor. No, openCursor() must be called here, not execute(). -- WBR, SD. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
On 11/26/21 5:47 PM, Tony Whyman wrote: On 26/11/2021 14:32, Dmitry Yemanov wrote: 26.11.2021 17:28, Tony Whyman wrote: 1. IAttachment.prepare is used to parse 'Update Employee Set Hire_Date = ? Where EMP_NO = ? Returning LAST_NAME' 2. IStatement.getType is then used to determine the statement type. OK so far. 3. Prior to calling IStatement.Execute, the statement type is checked. The current (IBX) code raises an exception if the sql statement type is isc_info_sql_stmt_select in order to stop the wrong IStatement method being called. Why? How is it different from SELECT returning isc_info_sql_stmt_select? They should work the same way. Is the SQL text also parsed to detect UPDATE and that conflicts with the returned statement type? Dmitry 1. This is not a backwards compatible change. If "Update...Returning" had always been given a statement type of isc_info_sql_stmt_select then the IBX code would have been written to support this. As it happens, it was not and so changing the statement type immediately introduces a backwards compatibility issue. Do you have a good reason to make this change? Support multiple rows returned from this statement. BTW - how do you determine that statement is "Update...Returning" ? Own parser? 2. IStatement has different methods "execute" and "openCursor". You need to know in advance which one to call. The former is called when you expect a singleton row (or no output) and the latter when you expect a cursor. Exactly. This is decided best of all based on Statement::getFlags(status), is flag Statement::FLAG_HAS_CURSOR set or not. 3. IBX tries to be general purpose and so prepares a statement, then checks the statement type, and then calls the appropriate IStatement method. Changing the statement type semantics broke the logic used to determine which method to call. Why? 4. With reference to item 1 above, if "Update..Returning" returns a statement type of isc_info_sql_stmt_select then you _do_ have to also parse the statement in IBX so that you know its an UPDATE and IStatement.execute has to be called instead of IStatement.openCursor. After currently discussed change openCursor() should be called for "Update..Returning", not execute. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
On 26/11/2021 14:32, Dmitry Yemanov wrote: 26.11.2021 17:28, Tony Whyman wrote: 1. IAttachment.prepare is used to parse 'Update Employee Set Hire_Date = ? Where EMP_NO = ? Returning LAST_NAME' 2. IStatement.getType is then used to determine the statement type. OK so far. 3. Prior to calling IStatement.Execute, the statement type is checked. The current (IBX) code raises an exception if the sql statement type is isc_info_sql_stmt_select in order to stop the wrong IStatement method being called. Why? How is it different from SELECT returning isc_info_sql_stmt_select? They should work the same way. Is the SQL text also parsed to detect UPDATE and that conflicts with the returned statement type? Dmitry 1. This is not a backwards compatible change. If "Update...Returning" had always been given a statement type of isc_info_sql_stmt_select then the IBX code would have been written to support this. As it happens, it was not and so changing the statement type immediately introduces a backwards compatibility issue. Do you have a good reason to make this change? 2. IStatement has different methods "execute" and "openCursor". You need to know in advance which one to call. The former is called when you expect a singleton row (or no output) and the latter when you expect a cursor. 3. IBX tries to be general purpose and so prepares a statement, then checks the statement type, and then calls the appropriate IStatement method. Changing the statement type semantics broke the logic used to determine which method to call. 4. With reference to item 1 above, if "Update..Returning" returns a statement type of isc_info_sql_stmt_select then you _do_ have to also parse the statement in IBX so that you know its an UPDATE and IStatement.execute has to be called instead of IStatement.openCursor. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
26.11.2021 17:28, Tony Whyman wrote: 1. IAttachment.prepare is used to parse 'Update Employee Set Hire_Date = ? Where EMP_NO = ? Returning LAST_NAME' 2. IStatement.getType is then used to determine the statement type. OK so far. 3. Prior to calling IStatement.Execute, the statement type is checked. The current (IBX) code raises an exception if the sql statement type is isc_info_sql_stmt_select in order to stop the wrong IStatement method being called. Why? How is it different from SELECT returning isc_info_sql_stmt_select? They should work the same way. Is the SQL text also parsed to detect UPDATE and that conflicts with the returned statement type? Dmitry Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
On 26/11/2021 14:16, Dimitry Sibiryakov wrote: Tony Whyman wrote 26.11.2021 15:10: This caused an error message to be generated because only a singleton row was expected and not a cursor. Well, all I can say is that "existing connectivity drivers do not support this feature automagically". Is it really then intended behaviour for Update...Returning to now return an SQL Type of isc_info_sql_stmt_select? Yes. What exactly call produces the error message and what exactly is the error? 1. IAttachment.prepare is used to parse 'Update Employee Set Hire_Date = ? Where EMP_NO = ? Returning LAST_NAME' 2. IStatement.getType is then used to determine the statement type. 3. Prior to calling IStatement.Execute, the statement type is checked. The current (IBX) code raises an exception if the sql statement type is isc_info_sql_stmt_select in order to stop the wrong IStatement method being called. Note that the exception is called by IBX and not Firebird. However, the exception is consequential on Firebird returning the statement type isc_info_sql_stmt_select when all previous versions of Firebird had returned isc_info_sql_stmt_exec_procedure for an Update...Returning statement. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
Re: [Firebird-devel] Firebird 5 and Update...Returning
Tony Whyman wrote 26.11.2021 15:10: This caused an error message to be generated because only a singleton row was expected and not a cursor. Well, all I can say is that "existing connectivity drivers do not support this feature automagically". Is it really then intended behaviour for Update...Returning to now return an SQL Type of isc_info_sql_stmt_select? Yes. What exactly call produces the error message and what exactly is the error? -- WBR, SD. Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
[Firebird-devel] Firebird 5 and Update...Returning
When testing out Dimtry's recent fix for scrollable cursors (adding support for the remote protocol), I ran the full IBX test suite on the current "master" branch. This flagged up a backwards compatibility issue with "Update...Returning". When the statement: 'Update Employee Set Hire_Date = ? Where EMP_NO = ? Returning LAST_NAME' was prepared, the SQL type was returned as isc_info_sql_stmt_select. The previous behaviour was to return isc_info_sql_stmt_exec_procedure. This caused an error message to be generated because only a singleton row was expected and not a cursor. The file doc/sql.extensions/README.returning has been updated and a quick diff with 4.0.0 returned, amongst other changes: 45,46c45,47 < isc_info_sql_stmt_exec_procedure by the API (instead of isc_info_sql_stmt_insert), < so the existing connectivity drivers should support this feature automagically. --- > isc_info_sql_stmt_exec_procedure by the API (for INSERT INTO ... VALUES and statements > with WHERE CURRENT OF) and isc_info_sql_stmt_select for the others statements, so the > existing connectivity drivers should support this feature automagically. Well, all I can say is that "existing connectivity drivers do not support this feature automagically". Is it really then intended behaviour for Update...Returning to now return an SQL Type of isc_info_sql_stmt_select? Tony Whyman MWA Firebird-Devel mailing list, web interface at https://lists.sourceforge.net/lists/listinfo/firebird-devel
