Skip to content

sqlite3 dml statement detection does not account for CTEs #81040

Closed
#94940
Closed
sqlite3 dml statement detection does not account for CTEs#81040
#94940
Labels
extension-modulesC modules in the Modules dirtopic-sqlite3type-bugAn unexpected behavior, bug, or error

Description

@coleifer
coleifer
mannequin
opened on May 8, 2019
BPO 36859
Nosy @berkerpeksag, @animalize, @tirkarthi, @coleifer, @erlend-aasland
PRs
  • bpo-36859: Use sqlite3_stmt_readonly API when possible to determine if statement is DML.K #13216
  • [WIP] gh-81040: use sqlite3_stmt_readonly() to detect DML statements #24492
    		•
    Files
  • 36859.patch
  • 36859-2.patch: Working patch that retains backwards-compatibility.
    		•

    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 2019-05-08.23:08:24.384>
labels = ['3.8', 'type-bug', 'library']
title = 'sqlite3 dml statement detection does not account for CTEs'
updated_at = <Date 2021-08-11.21:02:17.366>
user = 'https://github.com/coleifer'

    bugs.python.org fields:

    activity = <Date 2021-08-11.21:02:17.366>
actor = 'erlendaasland'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2019-05-08.23:08:24.384>
creator = 'coleifer'
dependencies = []
files = ['48319', '48320']
hgrepos = []
issue_num = 36859
keywords = ['patch']
message_count = 9.0
messages = ['341949', '341956', '341958', '341961', '341962', '386713', '386716', '386721', '399418']
nosy_count = 5.0
nosy_names = ['berker.peksag', 'malin', 'xtreak', 'coleifer', 'erlendaasland']
pr_nums = ['13216', '24492']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue36859'
versions = ['Python 3.8']

    Activity

    1. coleifer commented on May 8, 2019

      @coleifer
      coleifermannequin
      on May 8, 2019
      MannequinAuthor

      In statement.c, there is some logic which detects whether or not an incoming statement is a DML-type. The logic, as of 2019-05-08, I am referring to is here:

      cpython/Modules/_sqlite/statement.c

      Lines 78 to 93 in fc662ac

      self->is_dml = 0;
      for (p = sql_cstr; *p != 0; p++) {
      switch (*p) {
      case ' ':
      case '\r':
      case '\n':
      case '\t':
      continue;
      }
      self->is_dml = (PyOS_strnicmp(p, "insert", 6) == 0)
      || (PyOS_strnicmp(p, "update", 6) == 0)
      || (PyOS_strnicmp(p, "delete", 6) == 0)
      || (PyOS_strnicmp(p, "replace", 7) == 0);
      break;
      }

      To demonstrate the bug:

      import sqlite3

conn = sqlite3.connect(':memory:')
conn.execute('create table kv ("key" text primary key, "value" integer)')
conn.execute('insert into kv (key, value) values (?, ?), (?, ?)',
             ('k1', 1, 'k2', 2))

assert conn.in_transaction  # Yes we are in a transaction.
conn.commit()
assert not conn.in_transaction  # Not anymore, as expected.

rc = conn.execute(
    'with c(k, v) as (select key, value + 10 from kv) '
    'update kv set value=(select v from c where k=kv.key)')

print(rc.rowcount)  # Should be 2, prints "-1".
#assert conn.in_transaction  # !!! Fails.

curs = conn.execute('select * from kv order by key;')
print(curs.fetchall())  # [('k1', 11), ('k2', 12)]
    2. coleifer
      added
      stdlibStandard Library Python modules in the Lib/ directory
      type-bugAn unexpected behavior, bug, or error
      on May 8, 2019

    3. coleifer commented on May 9, 2019

      @coleifer
      coleifermannequin
      on May 9, 2019
      MannequinAuthor

      Sqlite since 3.7.11 provides sqlite3_stmt_readonly() API for determining if a prepared statement will affect the database. I made the change, removing the SQL scanning code and replacing it with:

      self->is_dml = !sqlite3_stmt_readonly(self->st);

      But then I see a number of test failures, mostly related to the fact that table-creation is now treated as "is_dml" with the above change.

      I don't know if the above API is going to be a workable path forward, since it seems like DML statements *not* automatically starting a transaction is a behavior a lot of people may have come to depend on (whether or not it is correct).

      I've attached a patch just-in-case anyone's interested.

    4. coleifer commented on May 9, 2019

      @coleifer
      coleifermannequin
      on May 9, 2019
      MannequinAuthor

      Oh, one more thing that is actually quite important -- since BEGIN IMMEDIATE and BEGIN EXCLUSIVE "modify" the database, these statements (intended to begin a transaction) are treated as "is_dml" when using the sqlite3_stmt_readonly API.

    5. coleifer commented on May 9, 2019

      @coleifer
      coleifermannequin
      on May 9, 2019
      MannequinAuthor

      I've got a patch working now that:

      • retains complete backwards-compatibility for DDL (as well as BEGIN EXCLUSIVE/IMMEDIATE) -- tests are passing locally.
      • retains previous behavior for old sqlite that do not have the sqlite3_stmt_readonly API.

      I think this should be good-to-go and I've in fact merged a similar patch on my own standalone pysqlite3 package.

      Also I will add that the little 'test script' I provided is working as-expected with this patch applied.

    6. tirkarthi commented on May 9, 2019

      @tirkarthi
      tirkarthi
      on May 9, 2019
      Member

      CPython now accepts PRs on GitHub. Please try raising a PR as per devuguide : https://devguide.python.org/

    7. tirkarthi
      added
      3.8 (EOL)end of life
      on May 9, 2019

    8. erlend-aasland commented on Feb 9, 2021

      @erlend-aasland
      erlend-aasland
      on Feb 9, 2021
      Contributor

      I believe #13216 would be an improvement. I see that your original branch is unavailable, Charles; would you mind if I cherry-picked it and rebased it onto master? The sqlite3 module now requires SQLite >= 3.7.15 which simplifies the change a lot.

    9. coleifer commented on Feb 9, 2021

      @coleifer
      coleifermannequin
      on Feb 9, 2021
      MannequinAuthor

      Yeah, go for it erlendaasland - I abandoned all hope of getting it merged, and have just been maintaining my own pysqlite3 which simplifies my life greatly.

    10. erlend-aasland commented on Feb 9, 2021

      @erlend-aasland
      erlend-aasland
      on Feb 9, 2021
      Contributor

      Thanks, Charles. I'll give it a shot and see if get can provoke a response :)

    11. erlend-aasland commented on Aug 11, 2021

      @erlend-aasland
      erlend-aasland
      on Aug 11, 2021
      Contributor

      See also gh-79579 (bpo-35398)

    12. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    13. erlend-aasland
      added
      topic-sqlite3
      and removed
      3.8 (EOL)end of life
      on May 16, 2022

    14. 6 remaining items

    15. erlend-aasland
      closed this as not plannedon Jun 21, 2022
    16. Repository owner moved this from Backwards compatibility issues to Done in sqlite3 issueson Jun 21, 2022

    17. erlend-aasland commented on Jun 21, 2022

      @erlend-aasland
      erlend-aasland
      on Jun 21, 2022
      Contributor

      I'm reopening this, because this issue materialises into two problems:

    18. erlend-aasland
      reopened this on Jun 21, 2022
    19. erlend-aasland
      moved this from Done to Backwards compatibility issues in sqlite3 issueson Jun 21, 2022

    20. erlend-aasland commented on Jul 17, 2022

      @erlend-aasland
      erlend-aasland
      on Jul 17, 2022
      Contributor

      2. rowcount is not updated as expected for CTE's.

      Suggesting to address the rowcount issue by improving the accuracy of the rowcount docs. We can then close this issue.

    21. erlend-aasland
      added a commit that references this issue on Jul 17, 2022

      pythongh-81040: Improve sqlite3.Cursor.rowcount docs

      1950829
    22. erlend-aasland
      closed this as completedin #94940on Jul 22, 2022
    23. Repository owner moved this from Backwards compatibility issues to Done in sqlite3 issueson Jul 22, 2022
    24. erlend-aasland
      added a commit that references this issue on Jul 22, 2022

      gh-81040: Improve sqlite3.Cursor.rowcount docs (#94940)

      f9b3706
    25. miss-islington
      added 2 commits that reference this issue on Jul 22, 2022

      pythongh-81040: Improve sqlite3.Cursor.rowcount docs (pythonGH-94940)

      bca4b3d

      pythongh-81040: Improve sqlite3.Cursor.rowcount docs (pythonGH-94940)

      7bcc42a

    26. erlend-aasland commented on Jul 22, 2022

      @erlend-aasland
      erlend-aasland
      on Jul 22, 2022
      Contributor

      Rowcount docs are updated in the following branches:

      Considering this as resolved.

    27. miss-islington
      added 2 commits that reference this issue on Jul 22, 2022

      gh-81040: Improve sqlite3.Cursor.rowcount docs (GH-94940)

      02f1cac

      gh-81040: Improve sqlite3.Cursor.rowcount docs (GH-94940)

      12b56e4
    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-sqlite3type-bugAn unexpected behavior, bug, or error

      Projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions