View Issue Details

IDProjectCategoryView StatusLast Update
0033010FPCPackagespublic2018-01-14 10:29
ReporterOndrej PokornyAssigned ToMichael Van Canneyt 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version3.1.1Product Build 
Target Version3.2.0Fixed in Version3.1.1 
Summary0033010: [fcl-db] MSSQL: An SQL statement is always prepared even if it's been prepared already
DescriptionIn MSSQL, when the statement is prepared, the FPrepared flag is not set to true so that it has to be prepared all over again which is useless.
Steps To Reproduce1.) Apply the attached patch (but comment out the "FPrepared := True;" line).

2.) Run this program:

program DBconn;

uses
  db, mssqlconn, sqldb, sysutils;

var
  xSQL: TMSSQLConnection;
  Q: TSQLQuery;
  T: TSQLTransaction;
begin
  xSQL := TMSSQLConnection.Create(nil);
  try
    xSQL.HostName := 'your-server';
    xSQL.DatabaseName := 'your-database';
    xSQL.Connected := True;

    Q := TSQLQuery.Create(xSQL);
    T := TSQLTransaction.Create(xSQL);
    T.DataBase := xSQL;
    Q.DataBase := xSQL;
    Q.Transaction := T;

    Writeln(sLineBreak, 'Statement start here', sLineBreak);
    Q.SQL.Text := 'SELECT mydate from mytest WHERE CAST(mydate as DATE)<>:mydate';
    Q.ParamByName('mydate').AsDate := now;
    Q.Open;
    Writeln(sLineBreak, '1: ', Q.Fields[0].AsString, sLineBreak);
    Q.Close;
    Q.ParamByName('mydate').AsDate := StrToDate('17.10.2017');
    Q.Open;
    Writeln('2: ', Q.Fields[0].AsString);
  finally
    xSQL.Free;
  end;

  Readln;
end.

3.) Check the output - you'll see that the SQL is parsed several times and the statement is prepared all over again.

4.) Uncomment the "FPrepared := True;" line in the patch.

5.) See again, the needed prepare/format calls have reduced.
Additional InformationThe patch includes Writeln debug lines. Please delete them before applying to FPC.
TagsNo tags attached.
Fixed in Revision37951
FPCOldBugId
FPCTarget
Attached Files
  • mssql-prepare-01.patch (1,178 bytes)
    Index: packages/fcl-db/src/base/dsparams.inc
    ===================================================================
    --- packages/fcl-db/src/base/dsparams.inc	(revision 37897)
    +++ packages/fcl-db/src/base/dsparams.inc	(working copy)
    @@ -303,6 +303,7 @@
       tmpParam:TParam;
     
     begin
    +  Writeln('TParams.ParseSQL: ', Copy(SQL, 1, 20)); // DEBUG
       if DoCreate then Clear;
       // Parse the SQL and build ParamBinding
       ParamCount:=0;
    Index: packages/fcl-db/src/sqldb/mssql/mssqlconn.pp
    ===================================================================
    --- packages/fcl-db/src/sqldb/mssql/mssqlconn.pp	(revision 37897)
    +++ packages/fcl-db/src/sqldb/mssql/mssqlconn.pp	(working copy)
    @@ -225,10 +225,12 @@
     var
       ParamBinding : TParamBinding;
     begin
    +  Writeln('TDBLibCursor.Prepare - ParseSQL: ', Copy(Buf, 1, 20)); // DEBUG
       if assigned(AParams) and (AParams.Count > 0) then
         FQuery:=AParams.ParseSQL(Buf, false, sqEscapeSlash in FConnection.ConnOptions, sqEscapeRepeat in FConnection.ConnOptions, psSimulated, ParamBinding, FParamReplaceString)
       else
         FQuery:=Buf;
    +  FPrepared := True;
     end;
     
     function TDBLibCursor.ReplaceParams(AParams: TParams): string;
    
    mssql-prepare-01.patch (1,178 bytes)

Activities

Ondrej Pokorny

2018-01-12 11:53

developer  

mssql-prepare-01.patch (1,178 bytes)
Index: packages/fcl-db/src/base/dsparams.inc
===================================================================
--- packages/fcl-db/src/base/dsparams.inc	(revision 37897)
+++ packages/fcl-db/src/base/dsparams.inc	(working copy)
@@ -303,6 +303,7 @@
   tmpParam:TParam;
 
 begin
+  Writeln('TParams.ParseSQL: ', Copy(SQL, 1, 20)); // DEBUG
   if DoCreate then Clear;
   // Parse the SQL and build ParamBinding
   ParamCount:=0;
Index: packages/fcl-db/src/sqldb/mssql/mssqlconn.pp
===================================================================
--- packages/fcl-db/src/sqldb/mssql/mssqlconn.pp	(revision 37897)
+++ packages/fcl-db/src/sqldb/mssql/mssqlconn.pp	(working copy)
@@ -225,10 +225,12 @@
 var
   ParamBinding : TParamBinding;
 begin
+  Writeln('TDBLibCursor.Prepare - ParseSQL: ', Copy(Buf, 1, 20)); // DEBUG
   if assigned(AParams) and (AParams.Count > 0) then
     FQuery:=AParams.ParseSQL(Buf, false, sqEscapeSlash in FConnection.ConnOptions, sqEscapeRepeat in FConnection.ConnOptions, psSimulated, ParamBinding, FParamReplaceString)
   else
     FQuery:=Buf;
+  FPrepared := True;
 end;
 
 function TDBLibCursor.ReplaceParams(AParams: TParams): string;
mssql-prepare-01.patch (1,178 bytes)

LacaK

2018-01-12 15:49

developer   ~0105698

mssqlconn uses dblib to talko to sql server. dblib does not provide server-side statement preparation. So PrepareStatement only localy parses SQL and substitute parameters with parameters placeholders.

There is also "hack" in sqldb, which calls UnprepareStatement when dataset is closed and statement is NOT prepared. This allows do cleaning of "in process resultset". There is "feature" in MSSQL, that when not all rows of pending resultset are consumed then next attempt to send another sql command ends with error 20019 : Attempt to initiate a new Adaptive Server operation with results pending

So I am not sure if this patch cannot cause compatibility issues.
Probably not.

Michael Van Canneyt

2018-01-12 15:57

administrator   ~0105699

@Laco, can you explain where this 'Hack' is ? In TSQLQuery ?

Ondrej Pokorny

2018-01-12 16:25

developer   ~0105702

> There is also "hack" in sqldb, which calls UnprepareStatement when dataset is closed and statement is NOT prepared.
@Laco: yes and no. Actually it's UnPrepared anyway

1.) procedure TCustomSQLQuery.InternalClose;
-->> if not Prepared then FStatement.DoUnprepare;

2.) procedure TCustomSQLQuery.SetActive(Value: Boolean);
-->> if not Value and IsPrepared then UnPrepare;


You see once there is a check for IsPrepared and once there is a check for not IsPrepared.

----

I assume such a problem will be registered quickly and then you can revert the patch.

Michael Van Canneyt

2018-01-12 16:40

administrator   ~0105704

If so, this is wrong.

The TSQLQuery should prepare if needed, but should only unprepare if it prepared itself.

The idea is that
Q.SQL.Text:='Select Blah from MyTable where Id=:ID';
Q.Prepare;
For something in myloop do
  begin
  Q.Open;
  // Aloha data
  Q.Close;
  end;
is prepared only once, and then executed repeatedly.

Ondrej Pokorny

2018-01-12 16:50

developer   ~0105706

@Michael: exactly, this is what I wanted to look into after this patch is applied.

LacaK

2018-01-12 18:47

developer   ~0105713

As Ondrej pointed out. There seems not to be a problem adding FPrepared:=True; only in TMSSQLConnection.

But going deeper into sqldb may lead to problems. There is for example from begining comment in:

procedure TCustomSQLQuery.BeforeRefreshOpenCursor;
begin
  // This is only necessary because TIBConnection can not re-open a
  // prepared cursor. In fact this is wrong, but has never led to
  // problems because in SetActive(false) queries are always
  // unprepared. (which is also wrong, but has to be fixed later)
  if IsPrepared then with SQLConnection do
    UnPrepareStatement(Cursor);
end;

Probably if fixed sqldb, then it may lead to need update applications and use:
  SQLQuery1.Close;
  SQLQuery1.Unprepare; // cancel pending rows if there are any

On the other hand, probably what is now in TMSSQLConnection handled in UnPrepareStatement should be moved to FreeFldBuffers (as far as FreeFldBuffers is called by InternalClose and here we can cancel pending resultset if client DataSet is closed)

IMO any change in sqldb should be checked at least using dbtestframework against all supported databases.
(I can fix MSSQLConnection and run test suite to do quick check)

Michael Van Canneyt

2018-01-12 18:57

administrator   ~0105716

We definitely need to dig deeper in SQLDb

1. As said, TSQLQuery may only unprepare if it has prepared by itself.
   Otherwise the whole idea of
   "prepare once, execute many times" is simply thrown away.
   It is the whole point of having prepare, after all.

2. The BeforeRefreshOpenCursor is plain wrong.
   If Firebird cannot handle this situation,
   it needs to close and re-execute the query.

3. The MSSQL thing is separate.
   It needs to be fixed first, and then the deeper sqlDB issue must be checked.

Michael Van Canneyt

2018-01-13 10:58

administrator   ~0105730

I have applied the patch, but removed the debug statements.

For the issue if prepare/unprepare by TSQLQuery, I think it's best to create a separate bugreport, it is a separate issue after all.

Ondrej Pokorny

2018-01-14 10:29

developer   ~0105781

Thank you!

Issue History

Date Modified Username Field Change
2018-01-12 11:53 Ondrej Pokorny New Issue
2018-01-12 11:53 Ondrej Pokorny File Added: mssql-prepare-01.patch
2018-01-12 12:31 Michael Van Canneyt Assigned To => Michael Van Canneyt
2018-01-12 12:31 Michael Van Canneyt Status new => assigned
2018-01-12 15:49 LacaK Note Added: 0105698
2018-01-12 15:49 LacaK Status assigned => feedback
2018-01-12 15:57 Michael Van Canneyt Note Added: 0105699
2018-01-12 16:25 Ondrej Pokorny Note Added: 0105702
2018-01-12 16:25 Ondrej Pokorny Status feedback => assigned
2018-01-12 16:40 Michael Van Canneyt Note Added: 0105704
2018-01-12 16:50 Ondrej Pokorny Note Added: 0105706
2018-01-12 18:47 LacaK Note Added: 0105713
2018-01-12 18:57 Michael Van Canneyt Note Added: 0105716
2018-01-13 10:58 Michael Van Canneyt Fixed in Revision => 37951
2018-01-13 10:58 Michael Van Canneyt Note Added: 0105730
2018-01-13 10:58 Michael Van Canneyt Status assigned => resolved
2018-01-13 10:58 Michael Van Canneyt Fixed in Version => 3.1.1
2018-01-13 10:58 Michael Van Canneyt Resolution open => fixed
2018-01-13 10:58 Michael Van Canneyt Target Version => 3.2.0
2018-01-14 10:29 Ondrej Pokorny Note Added: 0105781
2018-01-14 10:29 Ondrej Pokorny Status resolved => closed