mORMot Open Source

Eugene Ilyin

Member

From: milky_way/orion_arm/sun/earth

Registered: 2016-03-27

Posts: 132

Website

TSQLStatementCached.Prepare

Hi, I found an interesting behavior of SQL statements cache. This exception sometimes hides the root course of the problem add provides some difficulties in investigation on what is going wrong in the application logic.

Source fragment from SynSQLite3.pas

TSQLStatementCached.Prepare

    ...
    if added then begin
      StatementSQL := GenericSQL;
      Statement.Prepare(DB,GenericSQL);                <<< [1]
      Timer := TSynMonitor.Create;                     <<< [2]
      if WasPrepared<>nil then
        WasPrepared^ := true;
    end else begin
      if Statement.Request<>0 then
        Statement.Reset;
      if WasPrepared<>nil then
        WasPrepared^ := false;
    end;
    if ExecutionTimer<>nil then begin
      Timer.ProcessStartTask;                          <<< [3]
      ExecutionTimer^ := @Timer.InternalTimer;
      if ExecutionMonitor<>nil then
        ExecutionMonitor^ := Timer;
    end;
    ...

When the code in line [1] is failed during the .Prepare processing (some issues in GenericSQL string for example: wrong parameters or incorrect or deprecated field name, memory issues, etc.) the Timer in line [2] is not created but the GenericSQL string already added into the SQL cache.
As the result we create a situation when GenericSQL is added but Timer=nil.

Next time during the separate request (which can be very far in logs from the first one) when we run this code fragment again the added=true and lines [2] is not executed so we directly go to line [3] where we have access violation exception because Timer=nil.

My suggestions how to fix it:

1. The easiest one: Switch line [1] and line [2] each other - this requires to be ensure that we do not create timer twice (to prevent memory leaks) if .Prepare failed;

2. More complex: Redesign this code with cache involvement (for example remove from cache added GenericSQL string if .Prepare call failed (as I understand .WasPrepared is a try to monitor such situation but it not cover described variant) or call Prepare first and add GenericSQL string into the cache only when we run .Prepare without exceptions);

3. Smells bad variant: Check Timer for nil in line [3];

4. The simplest: Do nothing and allows to exists cache records with Timer=nil because it is programmer duty always ensure that .Prepare will be run without exceptions on server all the time.

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,655

Website

Re: TSQLStatementCached.Prepare

I've implemented something in-between...
Timer is checked for nil and an exception is raised if the SQL is incorrect.

See https://synopse.info/fossil/info/df27090da9