Execute SQL SELECT returning 0 rows shows stale data #1431

Activity

1. tlhackque commented on Jun 17, 2018

   

   tlhackque

   on Jun 17, 2018

   Author

   More actions

   I did some experimenting, and found that a simple select does what's expected. It seems that a compound select is necessary to trigger the issue.

   Here is a simple reproducer (distilled from the real case - the reproducer is valid, but somewhat silly).

   Create a database with 1 record and one field, named 'diskid' with a value of 1.

   create table CDs (diskid integer);
   insert into CDs (diskid) values (1 );
   

   Execute this SELECT statement:

   with dup ( id) as ( select diskid from CDs ) 
   select "dup".* from dup where( id=1)
   

   As expected, you get one row, like this:

      id
   1 1
   1 rows returned in 10ms from: with dup ( id) as ( select diskid from CDs ) 
   select "dup".* from dup where( id=1)
   

   Now change the 'id=1' to 'id=10'

   The results tab does NOT change.

   The status tab says:

   Query executed successfully: with dup ( id) as ( select diskid from CDs ) 
   select "dup".* from dup where( id=10000) (took 0ms)
   
2. justinclift commented on Jun 17, 2018

   

   justinclift

   on Jun 17, 2018

   Member

   More actions

   Thanks @tlhackque, yep that does sound like a bug. 😄
3. justinclift

   added

   bugConfirmed bugs or reports that are very likely to be bugs.Confirmed bugs or reports that are very likely to be bugs.

   on Jun 17, 2018
4. tlhackque commented on Jun 18, 2018

   

   tlhackque

   on Jun 18, 2018

   Author

   More actions

   If I had to guess, you probably classify statements into SELECT and non-SELECT statements. Likely looking for SELECT as the first token.

   The compound statment (beginining with WITH) is probably masking the fact that my example is a SELECT.

   So it's probably a matter of teaching your classifier more of the wonderous syntax at https://sqlite.org/lang_select.html. Fortunately, it's probably just accepting with [recursive] [cte]* in front of select when determining that a statement is a select. A full parser for SELECT would be a Project. (And you don't want to be fooled by select in an expression - e.g. within an UPDATE.)

   It may suffice to simply look for with as well as select as the first token of a command...

   Just a guess...
5. chrisjlocke commented on Jun 18, 2018

   

   chrisjlocke

   on Jun 18, 2018

   Member

   More actions

   .... and then add in comments, multi-line, whitespaces. Yes, its simple. 5 minute job....
6. tlhackque commented on Jun 18, 2018

   

   tlhackque

   on Jun 18, 2018

   Author

   More actions

   comments, multi-line, whitespaces.

   Which you already handle looking for select, right? So one might hope it's 'just' looking for either token. Otherwise, it's a bigger bug, but the essence is the same.

   Still, nothing takes 5 minutes. Not even the regression test.
7. mgrojo commented on Jun 18, 2018

   

   mgrojo

   on Jun 18, 2018

   Member

   More actions

   If I had to guess, you probably classify statements into SELECT and non-SELECT statements. Likely looking for SELECT as the first token.

   You guessed right.

   This is the revelant function:

   MainWindow::StatementType MainWindow::getQueryType(const QString& query) const
   {
       // Helper function for getting the type of a given query
   
       if(query.startsWith("SELECT", Qt::CaseInsensitive)) return SelectStatement;
       if(query.startsWith("ALTER", Qt::CaseInsensitive)) return AlterStatement;
       if(query.startsWith("DROP", Qt::CaseInsensitive)) return DropStatement;
       if(query.startsWith("ROLLBACK", Qt::CaseInsensitive)) return RollbackStatement;
       if(query.startsWith("PRAGMA", Qt::CaseInsensitive)) return PragmaStatement;
       if(query.startsWith("VACUUM", Qt::CaseInsensitive)) return VacuumStatement;
       if(query.startsWith("INSERT", Qt::CaseInsensitive)) return InsertStatement;
       if(query.startsWith("UPDATE", Qt::CaseInsensitive)) return UpdateStatement;
       if(query.startsWith("DELETE", Qt::CaseInsensitive)) return DeleteStatement;
       if(query.startsWith("CREATE", Qt::CaseInsensitive)) return CreateStatement;
       if(query.startsWith("ATTACH", Qt::CaseInsensitive)) return AttachStatement;
       if(query.startsWith("DETACH", Qt::CaseInsensitive)) return DetachStatement;
   
       return OtherStatement;
   }
   

   And then:

               // SQLite returns SQLITE_DONE when a valid SELECT statement was executed but returned no results. To run into the branch that updates
               // the status message and the table view anyway manipulate the status value here. This is also done for PRAGMA statements as they (sometimes)
               // return rows just like SELECT statements, too.
               if((query_part_type == SelectStatement || query_part_type == PragmaStatement) && sql3status == SQLITE_DONE)
                   sql3status = SQLITE_ROW;

   Adding this to getQueryType, of course, solves your issue, but this naive approach has probably other problems. In fact, a WITH statement can be followed by a SELECT, INSERT, DELETE, or UPDATE statement. Maybe someone has a better idea.

       if(query.startsWith("WITH", Qt::CaseInsensitive) return SelectStatement;
8. tlhackque commented on Jun 18, 2018

   

   tlhackque

   on Jun 18, 2018

   Author

   More actions

   My guesses are usually good - refined by those that weren't :-)

   Here's what I'd try first:

   If the query starts with WITH, you'll need to parse the optional RECURSIVE token and (required) CTE string to get to the verb. Fortunately, the CTE isn't bad. It's a name, then optional ( name,name,name,... ), then AS (select stmt).

   That last bit looks ugly - but for your purposes, you can just count ()s (well, ignoring those in comments and any quoted strings.) So, the verb will be the first token after the last (matched) right parenthesis. Note that the parentheses are required syntax, so there will be a (possibly nested) string to parse.

   In fact, you can skip parsing the column names by noting that AS is required, so after the table-name, skip everything up to AS, then skip matching parenthesis to the verb. You do need to ignore parenthesis within quoted strings though.

   It won't be 5 minutes -- but it shouldn't be a big job if you take the shortcuts.

   See the syntax diagram referenced in my earlier note.

   Still, it's not a full parser!

   Thanks for digging into this.
9. mgrojo

   added a commit that references this issue on Jun 19, 2018

   Reset SQL results table when the query has not returned data

   ...

   bf89d99
10. MKleusberg

    added a commit that references this issue on Jun 21, 2018

    Reset SQL results table when the query has not returned data

    ...

    01f0146
11. mgrojo commented on Jun 23, 2018

    

    mgrojo

    on Jun 23, 2018

    Member

    More actions

    @tlhackque Today's nightly build resets the results table with each query that is executed. This should fix the usability problem that you encountered (the results table will be our empty for your case). The underlaying problem with the CTE query is not solved for the moment.

    👍React with 👍1Reacted by Justin Clift
12. tlhackque commented on Jun 24, 2018

    

    tlhackque

    on Jun 24, 2018

    Author

    More actions

    Thanks, that's better.
13. mgrojo

    added a commit that references this issue on Oct 14, 2023

    Reset SQL results table when the query has not returned data

    ...

    ea5cc0a