bpo-45089: raise exceptions on error in sqlite3 trace callback

[erlend-aasland]

https://bugs.python.org/issue45089

[erlend-aasland]

I see no way to test PyUnicode_DecodeUTF8 errors: UTF8 errors (including too long strings) are handled when we create the statement.

[erlend-aasland]

@serhiy-storchaka I would like your opinion on this PR, if you don't mind.

[encukou]

I assumed exceptions in a trace function were ignored on purpose, since there's no way to inform SQLite of the error?
Is it possible to get multiple trace function failures from one sqlite3 call? If so, what happens?

It would be nice to chain the original exception. It's a bit complicated to combine it with the enable_callback_tracebacks mechanism, but you could do something like:

PyObject*type, *value, *traceback; // Clear the error indicator; get references to type, value, tracebackPyErr_Fetch(&type, &value, &traceback); pysqlite_state*state=pysqlite_get_state(NULL); if (state->enable_callback_tracebacks){// Get extra references to type, value & tracebackPy_INCREF(type); Py_INCREF(value); Py_INCREF(traceback); // Restore the error indicator (losing the extra references)PyErr_Restore(type, value, traceback); // Print the error (clearing the error indicator)PyErr_Print()} PyErr_SetString(state->OperationalError, "trace callback raised an exception"); // Chain the original exception as __cause__ (losing our references)_PyErr_ChainExceptions(type, value, traceback);

And since since the user will get the original exception, I wonder if the printing could be skipped.

[erlend-aasland]

I assumed exceptions in a trace function were ignored on purpose, since there's no way to inform SQLite of the error?

Maybe so, but how else should we inform the user about bugs in trace functions if not through exceptions?

It would be nice to chain the original exception

Yes, good point.

[encukou]

how else should we inform the user about bugs in trace functions if not through exceptions?

AFAIK, enable_callback_tracebacks is there exactly for that. It's crude but it seems to work; doing this right won't be an easy change.

Is it possible to get multiple trace function failures from one sqlite3 call?

It is:

>>> conn.executemany('INSERT INTO foo (bar) VALUES (?)', [(1,), (2,), (3,)]) Traceback (most recent call last): File "<stdin>", line 1, in <lambda> ZeroDivisionError: division by zero Traceback (most recent call last): File "<stdin>", line 1, in <lambda> ZeroDivisionError: division by zero Traceback (most recent call last): File "<stdin>", line 1, in <lambda> ZeroDivisionError: division by zero <sqlite3.Cursor object at 0x7faea47f8f80> 

Without the ability to tell SQLite to abort on trace failure, I doubt we can do much better than the current behavior. Getting an exception and having the data inserted seems quite irregular.

[erlend-aasland]

AFAIK, enable_callback_tracebacks is there exactly for that.

Well, it kinda works, but you won't get an exception. However, getting an exception and having the database modified is worse.

I doubt we can do much better than the current behavior.

I do agree.