Question about SQL specifically nrows

Posted by Teclab85 on Tue 10 May 2016 01:13 AM — 19 posts, 73,104 views.

#0
So according to the gammon api pages db:exec should not be broken out of because it will lock the db. I currently do, because I didn't read that part before when I made a script, so can someone explain what this is actually doing? My script seems to work just fine.

I am willing to rewrite the parts required if it is truly doing something malicious or if it is just a huge time sink, but other than that I just need to know if it is not worth the rewrite.
Australia Forum Administrator #1
Where does it say that exactly? Can you quote it?
#2
at http://www.gammon.com.au/scripts/doc.php?lua=db:nrows


Last line before the See also
"Note: You should let the iterator finish (that is, get to the end of the selected rows) otherwise the database remains locked. So, do not "break" out of the for loop."
USA Global Moderator #3
See: http://www.mushclient.com/forum/bbshowpost.php?bbsubject_id=11423
#4
Fiendish, that didn't really answer the question, it just reinforced that it should not be done. Could you elaborate a little more?

Is returning out of a function similar to just breaking out of the loop? As in, if found I return from the loop that is iterating over the SQL call.
Amended on Tue 10 May 2016 01:43 PM by Teclab85
Australia Forum Administrator #5
A Lua iterator is designed to be called repeatedly inside a "for" loop. The function nrows returns such an iterator, after doing an SQLite3 "prepare" (to set up for doing a SELECT or similar). After doing a "prepare" you are supposed to "finalize" to clean up the prepared statement, unlock the database, and free resources.

What happens in this iterator is that, when it reaches the end of the requested set of data (that is, the attempt to get more data returns SQLITE_DONE) it does the cleanup, and then returns nil, which exits the for loop.

If you break out of the loop (ie. using break or return) then that code is not executed and the database remains locked.

Your options are:

  • Don't break out of the loop, but let it finish, ignoring the other rows you don't want.
  • Make the query such that it exits itself when your desired condition is reached. For example, if you want to stop after a certain item is found, make that part of the WHERE clause.
  • Do the prepare / execute / finalize yourself (see below).


http://www.gammon.com.au/scripts/doc.php?general=lua_sqlite3

You can:




Example:


require "tprint"

-- make in-memory database
db = sqlite3.open_memory()

-- make a table for testing
db:exec  [[
          CREATE TABLE mobs (name, class, hp);
          INSERT INTO mobs VALUES("Naga", "mage", 666);
          INSERT INTO mobs VALUES("Wolf", "beast", 42);
          INSERT INTO mobs VALUES("Guard", "warrior", 100);
       ]]

-- prepare a SELECT statement
local stmt = db:prepare ("SELECT * FROM mobs")

-- loop until we get everything
while true do

  local result = stmt:step ()

  -- exit loop if done
  if result == sqlite3.DONE then
    break
  end -- if done

  -- should have ROW result
  assert (result == sqlite3.ROW, "Row not found")

  -- get all values into a table
  local row = stmt:get_named_values()

  -- display them
  print (string.rep ("-", 20))
  tprint (row)
  
end -- while

-- done with this statement
stmt:finalize ()

-- finished with database
db:close ()


Now in this example, since we are handling the finalize ourselves, we can break out of that loop if we want to, because we will then execute the stmt:finalize () anyway. If you choose to do a return then you need to do the finalize before returning.
Amended on Tue 10 May 2016 11:04 PM by Nick Gammon
Australia Forum Administrator #6
To trim it down to the minimum, instead of this:


for row in db:nrows("SELECT * FROM mobs") do
  tprint (row)
end


Do this:


local stmt = db:prepare ("SELECT * FROM mobs")
local result = stmt:step ()  -- first read
while result == sqlite3.ROW do
  local row = stmt:get_named_values()
  tprint (row)
  result = stmt:step ()  -- next read
end -- while
stmt:finalize ()


It's slightly lengthier, but you have more control.
Amended on Tue 10 May 2016 11:03 PM by Nick Gammon
USA Global Moderator #7
Teclab85 said:

Fiendish, that didn't really answer the question, it just reinforced that it should not be done. Could you elaborate a little more?

I think it does sort of answer with the part that says "You can test this by doing something like...", though obviously it's not as good of an explanation as what Nick has here because it was just a report. Perhaps what is missing is a real life implication of this problem?

See here then also:
https://github.com/fiendish/aardwolfclientpackage/issues/165
(It says posted by me because it's a conversion from the old defunct googlecode repository. It's actually reported by another user.)

Quote:

Is returning out of a function similar to just breaking out of the loop? As in, if found I return from the loop that is iterating over the SQL call.
Yes. Same thing.
Amended on Tue 10 May 2016 11:35 PM by Fiendish
Australia Forum Administrator #8
This technique could be usefully used when you know you will only have a single row returned. For example:


stmt = db:prepare ("SELECT COUNT(*) FROM mobs")
stmt:step ()  -- get first (and only) row
count = stmt:get_value (0)  -- get first column
stmt:finalize ()

print ("Count =", count)


In this case we know that a count will only ever be a single row, so this saves having a fake "for" loop, where we know in advance it will only ever have a single iteration.
Australia Forum Administrator #9
Another example shows how you can get a single result from a SELECT, where you may or may not get any data:


stmt = db:prepare ("SELECT * FROM mobs WHERE name = 'Naga' ")
result = stmt:step ()
if result == sqlite3.ROW then
  tprint (stmt:get_named_values())
else
  print ("Not found")
end -- if
stmt:finalize ()
#10
Well I am trying to convert my stuff over to not breaking out of the loops, its a little hard because several tings use nested for loops to iterate through several different dbs.

I want to thank Nick and Fiendish for answering my question and helping me resolve it.


Quick note, by doing as I am supposed to and not breaking out of the for loops by whole script runs about 5 seconds slower and locks the mud up while its thinking.
Amended on Wed 11 May 2016 02:14 PM by Teclab85
USA Global Moderator #11
Teclab85 said:

Quick note, by doing as I am supposed to and not breaking out of the for loops by whole script runs about 5 seconds slower and locks the mud up while its thinking.

Whaaaat? What are you doing?
Amended on Wed 11 May 2016 07:25 PM by Fiendish
Australia Forum Administrator #12
You can break out of the loops by using the techniques I described above. Five seconds sounds like a long time, however.
#13
Fiendish I meant my client.

I will post the code snippits when my son goes down and see if I am just royally messing up with this or missing something.
#14
So this is a little snippit of the old code.

local loc = cp_mobs[tableNum].location
local mob_table_name= ''
local rows_counter= 1
local rows_counter_check= 1
 res, gmcparg = CallPlugin("3e7dedbe37e44942dd46d264","gmcpval","char")
 luastmt = "gmcpdata = " .. gmcparg 
 assert (loadstring (luastmt or "")) ()
 level = tonumber(gmcpdata.status.level) 
 min_level = level - 11 
 max_level = level + 11 
 if (min_level<0)then min_level=0 end
  if dbA:isopen() then
    query1 = string.format("select rooms.uid,"..
      " areas.name as areaName,"..
      " rooms.name as roomName"..
      " from rooms, areas"..
      " where areas.uid = rooms.area and rooms.name = %s",fixsql(cp_mobs[tableNum].location))
    for rows in dbA:nrows(query1) do
      rows_counter = rows_counter+ 1
    end--for
    for rows in dbA:nrows(query1) do
      rows_counter_check = rows_counter_check+1
      if dbkt:isopen() then
        query = string.format("select room_id, COUNT(*) as timeskilled,"..
          " name from mobkills "..
          "where room_id = %s "..
          " group by room_id, name "..
          "ORDER by timeskilled asc ", fixsql(rows.uid))
        for a in dbkt:nrows(query) do   
          roomTemp= a.room_id
          if string.lower(a.name) == string.lower(name) then
            roomNumber= roomTemp
            mob_table_name = a.name
            break       
          end --if
        end--for
      end--if
      for a, v in ipairs(areaLevel) do
        if rows.areaName == v.name then
          if v.minLevel ~=nil and v.maxLevel ~=nil then
            if v.minLevel > max_level or v.maxLevel<min_level then 
              rows_counter= rows_counter-1
              rows_counter_check = rows_counter_check-1
              break
            else
              if room_num_table ~= nil and #room_num_table > 0 then
                for i, v in ipairs (room_num_table) do
                  if (string.lower(v[2]) == string.lower(nameHolder) or v[1]== rows.uid and tableNum == tableNumHolder) then
                    return
                  end--if
                end--for
              end--if
                if mob_table_name~= '' then
                  makeTable(roomNumber, cp_mobs[tableNum].name, cp_mobs[tableNum].mobdead, true)
                  mob_index= 1
                  return
                end--if for the commented section below
                 makeTable2(tonumber(rows.uid), cp_mobs[tableNum].name, cp_mobs[tableNum].mobdead, false)
                 mob_index= 1
            end--if
          end--if
        end--if
      end--for
 else")
    end--for
  end--if
  if rows_counter_check == rows_counter then
    makeTable(-1, cp_mobs[tableNum].name, cp_mobs[tableNum].mobdead, false)
 
  end--if
 
  tableNumHolder= tableNum
end 


This is what I turned it into.
Amended on Thu 12 May 2016 12:14 AM by Teclab85
#15

function getRoomIdRoomCP(name, nameHolder, tableNum)
local loc = cp_mobs[tableNum].location
local mob_table_name= ''
local rows_counter= 1
local rows_counter_check= 1
local inTable = false
local inLevel = true
res, gmcparg = CallPlugin("3e7dedbe37e44942dd46d264","gmcpval","char")
luastmt = "gmcpdata = " .. gmcparg
assert (loadstring (luastmt or "")) ()
level = tonumber(gmcpdata.status.level) 
min_level = level - 11 
max_level = level + 11 
if (min_level<0)then min_level=0 end
if not dbkt:isopen() or not dbA:isopen() then
  return
end--if

query1 = string.format("select rooms.uid,"..
  " areas.name as areaName,"..
  " rooms.name as roomName"..
  " from rooms, areas"..
  " where areas.uid = rooms.area and rooms.name = %s",fixsql(cp_mobs[tableNum].location))
for rows in dbA:nrows(query1) do
  rows_counter = rows_counter+ 1
end--for
DebugNote(rows_counter)
for rows in dbA:nrows(query1) do
  rows_counter_check = rows_counter_check+1
  query = string.format("select room_id, COUNT(*) as timeskilled,"..
    " name from mobkills "..
    "where room_id = %s "..
    "and lower(name) = %s"..
    " group by room_id, name "..
    "ORDER by timeskilled asc ", fixsql(rows.uid), fixsql(string.lower(name)))
  for a in dbkt:nrows(query) do     
    roomTemp= a.room_id
    if string.lower(a.name) == string.lower(name) then
      roomNumber= roomTemp
      mob_table_name = a.name      
    end --if
  end--for
      if areaLevel[rows.areaName].minLevel <= max_level and areaLevel[rows.areaName].maxLevel>=min_level then 
        inLevel = true
        if room_num_table ~= nil and #room_num_table > 0 then
          for i, v in ipairs (room_num_table) do
            DebugNote(v[2])
            DebugNote(nameHolder)
            DebugNote(tableNum)
            DebugNote(tableNumHolder)
            if ((string.lower(v[2]) == string.lower(nameHolder) or v[1] == rows.uid) and tableNum == tableNumHolder) then
              inTable = true
            end--if
          end--for
        end--if
        if mob_table_name~= '' and inTable == false then
          makeTable(roomNumber, cp_mobs[tableNum].name, cp_mobs[tableNum].mobdead, true)
          mob_index= 1
          DebugNote("first if part")
          rows_counter= rows_counter-1
        rows_counter_check = rows_counter_check-1
        elseif inTable == false then
          makeTable2(tonumber(rows.uid), cp_mobs[tableNum].name, cp_mobs[tableNum].mobdead, false)
          mob_index= 1
          DebugNote("Second if part")
          DebugNote(rows.uid.. " " .. cp_mobs[tableNum].name)
          rows_counter= rows_counter-1
        rows_counter_check = rows_counter_check-1
        end--if
      else
        rows_counter= rows_counter-1
        rows_counter_check = rows_counter_check-1
        inlevel = false
      end--if
end--for
  
  if rows_counter_check == rows_counter and inTable== false and inlevel == true then
    makeTable(-1, cp_mobs[tableNum].name, cp_mobs[tableNum].mobdead, false)
    print ('-1 becuase check = counter')
  end--if
  tableNumHolder= tableNum
end


So the issue is that with this it takes much longer, 5 or so seconds. I didn't see any actual issues with the old code, aside from some stuff that I needed to do better like the area checker that needed to be not a for loop as the array would work to use it.

I realize this is dirty looking and probably not efficient. This was my first plugin that I ever wrote and I have been slowly converting it over to a decent program for two years now in my spare time.
Amended on Thu 12 May 2016 12:15 AM by Teclab85
Australia Forum Administrator #16
For a start, this is very inefficient:


query1 = string.format("select rooms.uid,"..
  " areas.name as areaName,"..
  " rooms.name as roomName"..
  " from rooms, areas"..
  " where areas.uid = rooms.area and rooms.name = %s",fixsql(cp_mobs[tableNum].location))
for rows in dbA:nrows(query1) do
  rows_counter = rows_counter+ 1
end--for


If you are just counting things that match a criteria, get a count:


query1 = string.format("SELECT COUNT(*) AS counter " ..
  " FROM rooms, areas"..
  " WHERE areas.uid = rooms.area AND rooms.name = %s",fixsql(cp_mobs[tableNum].location))
for rows in dbA:nrows(query1) do
  rows_counter = rows.counter
end--for


That returns a single row, which is the count of how many records match the "where" clause.




And here:


        query = string.format("select room_id, COUNT(*) as timeskilled,"..
          " name from mobkills "..
          "where room_id = %s "..
          " group by room_id, name "..
          "ORDER by timeskilled asc ", fixsql(rows.uid))
        for a in dbkt:nrows(query) do   
          roomTemp= a.room_id
          if string.lower(a.name) == string.lower(name) then
            roomNumber= roomTemp
            mob_table_name = a.name
            break       
          end --if
        end--for


You are looking for a certain name, why not make that part of the select?


where room_id = %s AND name = %s


Now you don't need to break out of the loop.
Amended on Thu 12 May 2016 03:56 AM by Nick Gammon
#17
As I don't have much time to work on this project it is going slowly. I did convert the stupid thing I did with cycling through a whole table to get the count over to just using Count(*) and reading in the counter. Which by the way thank you, that was a terrible rookie mistake.

On to the rest.

I did add in searching for the where name = %s (name of mob) but that actually doesn't work as well as you would think, still returns 1-150 entries, helpful but not perfect yet. As it is down from 200-500 entries.

The whole conversion is still much slower than breaking out of the calls.

I am more interested in why I am not seeing the lockups or any errors for breaking or returning out of these calls.

I am thinking it might have something to do with the fact that I am using collectgarbage("collect") Before and after the program does what it is supposed to do. This would account for at least keeping the memory down, but not for locks on the .db files that I use.

I am calling into two different .db files to do one search and one of them is the Aardwolf.db file which if that was locked at any given time you would think the mapper would not work.


I just don't know. I am trying to do this the right way, but it is killing performance. I know you guys have given me a tremendous amount of resources to rectify this, but it just seems to keep slowing the program down.
Australia Forum Administrator #18
The method I suggested previously for breaking out of a loop after doing a select should work. After all you were doing that anyway. I don't know why it would suddenly add 5 seconds to the search.

500 records isn't much. Something else odd is going on.