Skip to content

bpo-45138: Expand traced SQL statements in sqlite3 trace callback - #28240

Merged
JelleZijlstra merged 28 commits into
python:mainfrom
erlend-aasland:sqlite-bound-trace
Mar 9, 2022
Merged

bpo-45138: Expand traced SQL statements in sqlite3 trace callback#28240
JelleZijlstra merged 28 commits into
python:mainfrom
erlend-aasland:sqlite-bound-trace

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

Comment threadModules/_sqlite/connection.c Outdated
@erlend-aasland

erlend-aasland commented Nov 1, 2021

Copy link
Copy Markdown
ContributorAuthor

I'll resolve the conflicting files after GH-29327 has been merged. Fixed.

@erlend-aasland

erlend-aasland commented Nov 2, 2021

Copy link
Copy Markdown
ContributorAuthor

Note to self: after GH-29356 has landed, add test for too long SQL statements. Done

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Coverage for trace_callback() is now pretty good.

Details
 1056| |static int
1057| |trace_callback(unsigned int type, void *ctx, void *stmt, void *sql)
1058| |#else
1059| |static void
1060| |trace_callback(void *ctx, const char *sql)
1061| |#endif
1062| 20|{
1063| 20|#ifdef HAVE_TRACE_V2
1064| 20| if (type != SQLITE_TRACE_STMT) {
1065| 0| return 0;
1066| 0| }
1067| 20|#endif
1068| |
1069| 20| PyGILState_STATE gilstate = PyGILState_Ensure();
1070| |
1071| 20| assert(ctx != NULL);
1072| 20| PyObject *py_statement = NULL;
1073| 20|#ifdef HAVE_TRACE_V2
1074| 20| assert(stmt != NULL);
1075| 20| const char *expanded_sql = sqlite3_expanded_sql((sqlite3_stmt *)stmt);
1076| 20| if (expanded_sql == NULL) {
1077| 2| sqlite3 *db = sqlite3_db_handle((sqlite3_stmt *)stmt);
1078| 2| if (sqlite3_errcode(db) == SQLITE_NOMEM) {
1079| 0| (void)PyErr_NoMemory();
1080| 0| goto exit;
1081| 0| }
1082| |
1083| 2| pysqlite_state *state = ((callback_context *)ctx)->state;
1084| 2| assert(state != NULL);
1085| 2| PyErr_SetString(state->DataError,
1086| 2| "Expanded SQL string exceeds the maximum string "
1087| 2| "length");
1088| |
1089| | // Fall back to unexpanded sql
1090| 2| py_statement = PyUnicode_FromString((const char *)sql);
1091| 2| }
1092| 18| else {
1093| 18| py_statement = PyUnicode_FromString(expanded_sql);
1094| 18| sqlite3_free((void *)expanded_sql);
1095| 18| }
1096| |#else
1097| | py_statement = PyUnicode_FromString(sql);
1098| |#endif
1099| 20| if (py_statement) {
1100| 20| PyObject *exc, *val, *tb;
1101| 20| PyErr_Fetch(&exc, &val, &tb);
1102| |
1103| 20| PyObject *callable = ((callback_context *)ctx)->callable;
1104| 20| PyObject *ret = PyObject_CallOneArg(callable, py_statement);
1105| 20| Py_DECREF(py_statement);
1106| 20| Py_XDECREF(ret);
1107| |
1108| 20| _PyErr_ChainExceptions(exc, val, tb);
1109| 20| }
1110| |
1111| 20| if (PyErr_Occurred()) {
1112| 4| print_or_clear_traceback((callback_context *)ctx);
1113| 4| }
1114| |
1115| 20|exit:
1116| 20| PyGILState_Release(gilstate);
1117| 20|#ifdef HAVE_TRACE_V2
1118| 20| return 0;
1119| 20|#endif
1120| 20|}

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

I believe I've addressed everything here, @pablogsal. Let me know if you want further changes.

@erlend-aasland
erlend-aasland requested review from JelleZijlstra and removed request for corona10March 2, 2022 08:30
@JelleZijlstraJelleZijlstra self-assigned this Mar 2, 2022

@JelleZijlstraJelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, a few small comments

Comment threadLib/test/test_sqlite3/test_hooks.py Outdated
Comment threadLib/test/test_sqlite3/test_hooks.py Outdated
Comment threadLib/test/test_sqlite3/test_hooks.py Outdated
Comment threadModules/_sqlite/connection.c
Erlend Egeberg Aaslandand others added 3 commits March 3, 2022 09:27
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>

@JelleZijlstraJelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! @gvanrossum I'm planning to merge this change, another SQLite enhancement by Erlend.

@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

Thanks, @JelleZijlstra!

@kumaraditya303

Copy link
Copy Markdown
Contributor

This change broke several buildbots See https://buildbot.python.org/all/#/builders/58/builds/1850

@erlend-aasland
erlend-aasland deleted the sqlite-bound-trace branch March 9, 2022 08:22
@erlend-aasland

Copy link
Copy Markdown
ContributorAuthor

I'm aware, and I'm coming up with a fix for that test.

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Mar 9, 2022
miss-islington pushed a commit that referenced this pull request Mar 9, 2022
This reverts commit d177751.
Automerge-Triggered-By: GH:JelleZijlstra
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@erlend-aasland@kumaraditya303@JelleZijlstra@pablogsal@the-knights-who-say-ni@bedevere-bot