ISQLDBStatement.Step should return Integer value instead of boolean?

edwinsn · 2022-11-19 07:42:00

@ab,

I think ISQLDBStatement.Step should return Integer value instead of boolean?

Otherwise in case of "update/delete" statements, how do we know if the returned value is SQLITE_OK, SQLITE_ERROR or SQLITE_BUSY?

I'm talking about mORMot 1, not sure about mORMot 2

Note, I know there is the `UpdateCount` function, but sometimes the update count is zero, all the caller wants to know is if the SQL was executed successfully by the engine.

Last edited by edwinsn (2022-11-19 07:47:25)

ab · 2022-11-19 18:56:34

SQLITE_BUSY and other constants are SQLite specific, so can't be translated outside of SQLIte.
And in particular, a SQLITE_BUSY error means that the DB is locked, which is not possible with our SQLite3 access with exclusive mode.

So a boolean sounds fine.

edwinsn · 2022-11-20 03:14:15

Imagine situations like executing `PRAGMA ...`, the return result is SQLITE_DONE in case of a successfully call, but inside `TSQLDBSQLite3Statement.Step`, only SQLITE_ROW is checked:

    result := fStatement.Step=SQLITE_ROW;

So there is no way to check the real return value.

I suggest to add a new ISQLDBStatement.LastStepReturnValue: Integer;

Last edited by edwinsn (2022-11-20 03:15:19)

edwinsn · 2022-11-20 07:07:21

@ab,

I've added ISQLDBStatement.LastStepReturnedValue, without affecting all previously existing functions,

github PR: https://github.com/synopse/mORMot/pull/435

edwinsn · 2022-11-20 07:35:17

More thoughts on the current design:

- Semantically, it only makes sense to check only against SQLITE_ROW for ISQLDBRows.Step, but for ISQLDBStatement.Step, the term "statement" has a broader meaning (it can be any CURD statement), the return value should be Integer.

- Semantically, ISQLDBRows should be inherited from ISQLDBStatement, but not the other way around as with the current design, because the term "statement" is more "low level", it can be any CURD statement; On the other hand, ISQLDBRows semantically equals the SELECT SQL statement. In other words, actually the class hierarchy should have been:

ISQLDBRows = interface(ISQLDBStatement)

Instead of the current design:

ISQLDBStatement = interface(ISQLDBRows)

Of course I'm not asking to change the design, I can live with that. But of course I'll be appreciated if you accept my github pull request above

ab · 2022-11-21 10:00:49

About ISqlDBRows, its design and purpose was stated in the comment/documentation:

  /// generic interface to access a SQL query result rows
  // - not all TSqlDBStatement methods are available, but only those to retrieve
  // data from a statement result: the purpose of this interface is to make
  // easy access to result rows, not provide all available features - therefore
  // you only have access to the Step() and Column*() methods
  ISqlDBRows = interface

So the inheritance is done from an interface point of view, i.e. its usage as results set, to hide unneeded methods, i.e. the binding/execution methods.
You are thinking in terms of classes, i.e. its implementation. Which was not the purpose of the interfaces.

About the pull request, you are still adding some SQLite3 specific information to the abstract ISqlDbStatement.
Why not just directly call the SQlite3 engine, and its TSqlDataBase class?

edwinsn · 2022-11-21 14:05:49

It seems that I'm messing up ISQLDBStatement.ExecutePrepared with ISQLDBRows.Step? Maybe I should use the former instead?

I guess so... So I go ahead and checked the implementation of 'TSQLDBSQLite3Statement.ExecutePrepared', it still has the same issue - didn't take into account result code other than SQLITE_ROW:

  try  // INSERT/UPDATE/DELETE (i.e. not SELECT) -> try to execute directly now
    repeat // Execute all steps of the first statement
    until fStatement.Step<>SQLITE_ROW;
    fUpdateCount := DB.LastChangeCount;
  finally
    ...
  end;

So it seems that my proposed pull request should have been done in TSQLDBSQLite3Statement.ExecutePrepared?

ab · 2022-11-21 14:18:21

Or just don't use ExecutePrepared, but the raw TSqlRequest.Step method - and finally TSqlRequest.Close.
That is, write your own Execute method.

For SQlite3 specific use case, you could rather use the SQlite3 layer, not the high-level ISQLDBStatement.

ab · 2022-11-21 14:24:49

Edit:
I looked again at the source code.

TSqlRequest.Execute will raise an exception on any error, i.e. if result <> SQLITE_ROW.
So I don't understand in which case you may have something interesting as result...

edwinsn · 2022-11-21 14:35:33

Thanks ab, I'll check TSqlRequest and feedback later.

edwinsn · 2022-11-22 01:25:32

ab wrote:

Edit:
I looked again at the source code.
TSqlRequest.Execute will raise an exception on any error, i.e. if result <> SQLITE_ROW.
So I don't understand in which case you may have something interesting as result...

I'm not sure if I understand it correctly, but if the SQL being executed is like this "delete from table1", SQLite's Step function will return SQLITE_DONE, but not SQLITE_ROW, and I have a `CheckStepCode` like the following to check if the "step" is a success or not:

function CheckStepCode(const aStatement: ISQLDBStatement): Boolean;
begin
  Result := not (aStatement.LastStepReturnedValue in SQLITE_ERRORS);
end;

I have a lot of code like the following that calls `CheckStepCode`:

var
  stmt: ISQLDBStatement;
begin
  stmt := FDb.NewThreadSafeStatementPrepared('delete from table1 where field1 = ?', False);
  stmt.Bind(1, aDoc.DocId);
  stmt.Step;
  Result := CheckStepCode(stmt);
end;

edwinsn · 2022-11-22 01:27:30

After checking out TSqlRequest, I think I won't use it since it's SQLite-specific, it won't allow me switch to other database systems easily.

So I will keep using the current modification of `ISQLDBStatement`, because if the PR mentioned above is not accepted, I can keep my local changes, it's a small modification and I can maintain that.

ab · 2022-11-22 09:02:58

The problem is what you do with the returned error code.
This integer value is SQlite-specific. You need to check SQLITE_ERRORS in your case.
So it won't help you switching to other DB easily.

For instance, UpdateCount is available, and is not specific - it is a count, whatever DB it runs on.
But the error code is highly dependent on the DB.

edwinsn · 2022-11-22 11:30:52

The value returned by SQLite is of course SQLite-specific, but 'returning a result code after executing a SQL' is not.

For example, in TSQLDBOracleStatement.ExecutePrepared, StmtExecute returns an integer value.
For example, in TSQLDBFirebirdStatement, isc_dsql_execute also returns an integer value.

My point is that, ExecutePrepared, no matter what the backend db is, should return a result code, the framework don't have to consider or worry about what the value is.

edwinsn · 2022-11-22 11:49:32

ab wrote:

The problem is what you do with the returned error code.

As I said, the framework don't have to worry about how the users use the returned error code, but the framework need to return the error code after executing a SQL, doesn't it?

In my case, I want to check if my 'DELETE FROM' statement was a success or not; If SQLite returned the "busy" code, I can initiate retries. And so on. But to reiterate - the framework don't have to worry about this, but it has to return the error code, via another field, or via function call return value, or any other means you think appropriate.

ab wrote:

So it won't help you switching to other DB easily.

It does help. If I keep using ISQLDBStatement, to switch to another db all I need to do is changing the `CheckStepCode` function; But if I use TSQLRequest, I'll have to change change all places where TSQLRequest is used. I think this is very obvious.

I hope it makes sense.

Last edited by edwinsn (2022-11-22 11:50:24)

mORMot Open Source

#1 2022-11-19 07:42:00

ISQLDBStatement.Step should return Integer value instead of boolean?

#2 2022-11-19 18:56:34

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#3 2022-11-20 03:14:15

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#4 2022-11-20 07:07:21

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#5 2022-11-20 07:35:17

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#6 2022-11-21 10:00:49

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#7 2022-11-21 14:05:49

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#8 2022-11-21 14:18:21

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#9 2022-11-21 14:24:49

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#10 2022-11-21 14:35:33

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#11 2022-11-22 01:25:32

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#12 2022-11-22 01:27:30

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#13 2022-11-22 09:02:58

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#14 2022-11-22 11:30:52

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

#15 2022-11-22 11:49:32

Re: ISQLDBStatement.Step should return Integer value instead of boolean?

Board footer