Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Sep 8, 2021

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Nov 1, 2021

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

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Nov 2, 2021

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

@erlend-aasland
Copy link
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
ContributorAuthor

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

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

@JelleZijlstraJelleZijlstra left a comment

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

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>
Copy link
Member

@JelleZijlstraJelleZijlstra left a comment

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
ContributorAuthor

Thanks, @JelleZijlstra!

@kumaraditya303
Copy link
Contributor

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

@erlend-aaslanderlend-aasland deleted the sqlite-bound-trace branch March 9, 2022 08:22
@erlend-aasland
Copy link
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