Sitelet https://github.com/python/cpython/issues/89121
Skip to content

[sqlite3] only reset statements when needed #89121

Closed
#30379
Closed
[sqlite3] only reset statements when needed#89121
#30379
Labels
extension-modulesC modules in the Modules dirtopic-sqlite3

Description

@erlend-aasland
erlend-aasland
opened on Aug 19, 2021
BPO 44958
Nosy @berkerpeksag, @serhiy-storchaka, @pablogsal, @miss-islington, @erlend-aasland, @Fidget-Spinner
PRs
  • bpo-44958: Only reset sqlite3 statements when needed #27844
  • bpo-44958: Fix ref. leak introduced in GH-27844 #28490
  • bpo-44958: Revert GH-27844 #28574
  • gh-89121: Keep the number of pending SQLite statements to a minimum #30379
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = None
created_at = <Date 2021-08-19.20:05:09.432>
labels = ['extension-modules']
title = '[sqlite3] only reset statements when needed'
updated_at = <Date 2022-01-03.23:42:52.964>
user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2022-01-03.23:42:52.964>
actor = 'erlendaasland'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2021-08-19.20:05:09.432>
creator = 'erlendaasland'
dependencies = []
files = []
hgrepos = []
issue_num = 44958
keywords = ['patch']
message_count = 14.0
messages = ['399939', '399984', '401186', '402312', '402317', '402319', '402402', '402423', '402424', '402425', '402427', '402490', '402677', '402683']
nosy_count = 6.0
nosy_names = ['berker.peksag', 'serhiy.storchaka', 'pablogsal', 'miss-islington', 'erlendaasland', 'kj']
pr_nums = ['27844', '28490', '28574', '30379']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue44958'
versions = []

    Activity

    1. erlend-aasland
      added
      extension-modulesC modules in the Modules dir
      on Aug 19, 2021

    2. erlend-aasland commented on Aug 20, 2021

      @erlend-aasland
      erlend-aasland
      on Aug 20, 2021
      ContributorAuthor

      Ref. Serhiy's msg387858 in bpo-43350:
      "Maybe the code could be rewritten in more explicit way and call pysqlite_statement_reset() only when it is necessary [...]"

      Currently, we try to reset statements in all "statement exit" routes. IMO, it would be cleaner to just reset statements when we really need to:

      1. before the first sqlite3_step() every time we (re)execute a query
      2. at cursor exit, if there's an active statement
        (3. in pysqlite_do_all_statements() ... see bpo-44092)

      This will make the code easier to follow, and it will minimise the number of resets. The current patch is pretty small: 7 insertions(+), 33 deletions(-)

      Pro:

      • less lines of code, less maintenance
      • cleaner exit paths
      • optimise SQLite API usage
        Con:
      • code churn

      If this is accepted, PR 25984 of bpo-44073 will be easier to land and review :)

    3. erlend-aasland commented on Aug 20, 2021

      @erlend-aasland
      erlend-aasland
      on Aug 20, 2021
      ContributorAuthor

      I did a quick count of sqlite3_reset()s in the sqlite3 test suite:

      Since we never call sqlite3_reset() with a NULL pointer, all sqlite3_reset() calls we execute hold the SQLite db mutex; reducing the number of sqlite3_reset() calls reduces the number of times we need to hold the mutex.

    4. erlend-aasland commented on Sep 6, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 6, 2021
      ContributorAuthor

      In msg399939, item 2 lacks one more "reset path":

      1. at cursor exit, if there's an active statement

      Rewording this to:

      1. when a statement is removed from a cursor; that is either at cursor dealloc, or when the current statement is replaced.

    5. pablogsal commented on Sep 21, 2021

      @pablogsal
      pablogsal
      on Sep 21, 2021
      Member

      New changeset 050d103 by Erlend Egeberg Aasland in branch 'main':
      bpo-44958: Only reset sqlite3 statements when needed (GH-27844)
      050d103

    6. miss-islington commented on Sep 21, 2021

      @miss-islington
      miss-islington
      on Sep 21, 2021
      Contributor

      New changeset 3e3ff09 by Erlend Egeberg Aasland in branch 'main':
      bpo-44958: Fix ref. leak introduced in #72031 (GH-28490)
      3e3ff09

    7. erlend-aasland commented on Sep 21, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 21, 2021
      ContributorAuthor

      Now that pysqlite_statement_reset() is only used in cursor.c, I suggest to move it to cursor.c and make it a static function.

    8. erlend-aasland commented on Sep 21, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 21, 2021
      ContributorAuthor

      Now that pysqlite_statement_reset() is only used in cursor.c, I suggest to move it to cursor.c and make it a static function.

      I'll wait until PR 25984 is merged before opening a PR for relocating pysqlite_statement_reset().

    9. Fidget-Spinner commented on Sep 22, 2021

      @Fidget-Spinner
      Fidget-Spinner
      on Sep 22, 2021
      Member

      Erlend, I suspect that 050d103 may have introduced a perf regression in pyperformance's sqlite_synth benchmark:
      https://speed.python.org/timeline/?exe=12&base=&ben=sqlite_synth&env=1&revs=50&equid=off&quarts=on&extr=on
      The benchmark code is here https://github.com/python/pyperformance/blob/main/pyperformance/benchmarks/bm_sqlite_synth.py.

    10. erlend-aasland commented on Sep 22, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 22, 2021
      ContributorAuthor

      Ouch, that's quite a regression! Thanks for the heads up! I'll have a look at it right away.

    11. erlend-aasland commented on Sep 22, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 22, 2021
      ContributorAuthor

      I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations). Are you able to reproduce, Ken?

    12. erlend-aasland commented on Sep 22, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 22, 2021
      ContributorAuthor

      I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations) [...]

      Correction: I _am_ able to reproduce this.

    13. erlend-aasland commented on Sep 23, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 23, 2021
      ContributorAuthor

      Explicitly resetting statements when we're done with them removes the performance regression; SQLite works more efficient when we keep the number of non-reset statements low.

    14. erlend-aasland commented on Sep 26, 2021

      @erlend-aasland
      erlend-aasland
      on Sep 26, 2021
      ContributorAuthor

      I'll revert PR 27844 for now (except the tests).

      Since SQLite works better when we keep the number of non-reset statements to a minimum, we need to ensure that we reset statements when we're done with them (sqlite3_step() returns SQLITE_DONE or an error). Before doing such a change, we should clean up _pysqlite_query_execute() so we don't need to sprinkle that function with pysqlite_statement_reset's. I plan to do this before attempting to clean up reset usage again.

    15. pablogsal commented on Sep 26, 2021

      @pablogsal
      pablogsal
      on Sep 26, 2021
      Member

      New changeset 7b88f63 by Erlend Egeberg Aasland in branch 'main':
      bpo-44958: Revert #72031 (GH-28574)
      7b88f63

    16. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    17. erlend-aasland
      added
      topic-sqlite3
      on May 16, 2022
    18. erlend-aasland
      added this to sqlite3 issueson May 21, 2022
    19. erlend-aasland
      moved this to In Progress in sqlite3 issueson May 21, 2022
    20. erlend-aasland
      closed this as completedin #30379on Jun 23, 2022
    21. Repository owner moved this from In Progress to Done in sqlite3 issueson Jun 23, 2022
    22. erlend-aasland
      added a commit that references this issue on Jun 23, 2022

      gh-89121: Keep the number of pending SQLite statements to a minimum (#…

      8928531
    23. serhiy-storchaka
      added a commit that references this issue on Jun 26, 2022

      Revert 30935 defer bpo45162 to 312 (#94285)

      1ffde3e
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      extension-modulesC modules in the Modules dirtopic-sqlite3

      Projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions