#688 SQL prepared statements not always re-used

Reporter MattJ
Owner Waqas
Created
Updated
Stars ★★★ (4)  
Tags
  • Type-Defect
  • Status-Accepted
  • Priority-Medium
  • Milestone-0.10
  1. MattJ on

    ** This report needs more confirmation through a proper test setup. ** Prosody can create more prepared statements than strictly necessary for some kinds of queries, which can lead to unnecessary resource consumption (i.e. garbage collecting idle prepared statements that could be reused). 0.10 has improved a number of SQL-related things, and I feel strongly that this fix should also go into the release, or at least some testing to assess the real-world impact.

  2. MattJ on

    Changes
    • owner MattJ
    • tags Priority-High
  3. MattJ on

    Changes
    • tags Milestone-0.10
  4. Zash on

    Changes
    • tags Status-Accepted
  5. Waqas on

    Patch being tested: diff --git a/sql.lua b/sql2.lua index 1574991..61d6af4 100644 --- a/sql.lua +++ b/sql2.lua @@ -175,7 +175,11 @@ function engine:execute_query(sql, ...) sql = self:prepquery(sql); local stmt = assert(self.conn:prepare(sql)); assert(stmt:execute(...)); - return stmt:rows(); + local result = {}; + for row in stmt:rows() do result[#result + 1] = row; end + stmt:close(); + local i = 0; + return function() i=i+1; return result[i]; end; end function engine:execute_update(sql, ...) sql = self:prepquery(sql);

    Changes
    • owner MattJ Waqas
  6. Waqas on

    The above patch reads prepared statements eagerly and closes them explicitly. This works around the scenario where prepared statements are being created faster than the GC can clean them, resulting in hitting the database's prepared statement limit.

  7. Zash on

    Doesn't that fix #391, while bumping statement reuse to a future branch?

  8. Zash on

    Changes
    • tags Patch
  9. Waqas on

    Zash, yes, as we had decided to simply solve that piece of the two related issues. Let's re-tag that one for 0.10, and push this one out.

  10. MattJ on

    Dropping the priority on this issue in favour of #391 (which is what the above patch actually fixes)

    Changes
    • tags Priority-Medium
  11. MattJ on

    Changes
    • tags Patch

New comment