Invocation of stored procedure with empty TVP dumps core #772
Comments
#!/usr/bin/env python3
# Repro case
from argparse import ArgumentParser
from pyodbc import connect
CLEANUP = (
"DROP PROCEDURE save_dictionary_term",
"DROP TYPE alias_type",
"DROP TABLE dictionary_term_alias",
"DROP TABLE dictionary_term",
)
parser = ArgumentParser()
parser.add_argument("conn")
opts = parser.parse_args()
conn = connect(opts.conn)
cursor = conn.cursor()
# Clean up from previous runs.
for statement in CLEANUP:
try:
cursor.execute(statement)
conn.commit()
except Exception:
pass
# Create the database objects.
cursor.execute("""\
CREATE TABLE dictionary_term (
term_id INTEGER NOT NULL,
term_name NVARCHAR(1000) NOT NULL
)""")
conn.commit()
cursor.execute("""\
CREATE TABLE dictionary_term_alias (
term_id INTEGER NOT NULL,
other_name NVARCHAR(1000) NOT NULL,
other_name_type NVARCHAR(30) NOT NULL
)""")
conn.commit()
cursor.execute("""\
CREATE TYPE alias_type AS TABLE (
term_id INTEGER NULL,
other_name NVARCHAR(1000) NULL,
other_name_type NVARCHAR(30) NULL
)""")
conn.commit()
cursor.execute("""\
CREATE PROCEDURE save_dictionary_term (
@tid INTEGER,
@tname NVARCHAR(1000),
@aliases alias_type READONLY
)
AS
BEGIN
INSERT INTO dictionary_term (term_id, term_name) VALUES (@tid, @tname)
INSERT INTO dictionary_term_alias (term_id, other_name, other_name_type)
SELECT term_id, other_name, other_name_type FROM @aliases
END""")
conn.commit()
# Invoke the stored procedure with a non-empty table parameter argument.
aliases = [
[42, "pullet", "synonym"],
[42, "capon", "synonym"],
[42, "fowl", "broader"],
]
command = "EXECUTE save_dictionary_term @tid=?, @tname=?, @aliases=?"
args = 42, "chicken", aliases
cursor.execute(command, args)
conn.commit()
print(cursor.execute("SELECT * FROM dictionary_term").fetchall())
print(cursor.execute("SELECT * FROM dictionary_term_alias").fetchall())
# Invoke the stored procedure with an empty table parameter argument.
args = 76, "trombones", []
cursor.execute(command, args) # <-- will crash the interpreter here
conn.commit()
print(cursor.execute("SELECT * FROM dictionary_term").fetchall())
print(cursor.execute("SELECT * FROM dictionary_term_alias").fetchall()) |
|
Could you also provide an ODBC trace? |
|
No problem (credentials redacted). odbctrace.log |
|
Doesn't look as if the problem is in a lower layer. I can successfully invoke stored procedures with empty table parameters from C++ or C# using the same driver. |
|
I see what's happening. The code in |
|
There's a possibility that I'll try to come up with a PR. The factors which argue against doing that are
Partly I suppose my ultimate decision will depend on how much encouragement and support I get from the maintainers for giving it a shot. One thing I'll ask up front is about the apparent contradiction I see between this comment in // Items specific to databases.
//
// Obviously we'd like to minimize this, but if they are needed this file isolates them. I'd like for there to be a
// single build of pyodbc on each platform and not have a bunch of defines for supporting different databases.And things like this in #ifndef SQL_SS_TABLE
#define SQL_SS_TABLE -153
#endif
#ifndef SQL_SOPT_SS_PARAM_FOCUS
#define SQL_SOPT_SS_PARAM_FOCUS 1236
#endif
#ifndef SQL_CA_SS_TYPE_NAME
#define SQL_CA_SS_TYPE_NAME 1227
#endifIf additional defined constants are needed for the fix (seems likely) where should they go? Is one of those two headers preferred over the other for this purpose? Directly in Thanks! |
|
Here's another problem, closely related to the bug originally reported. On line 1437 ff. of Py_ssize_t i = PySequence_Size(info.pObject) - info.ColumnSize;
Py_ssize_t ncols = 0;
while (i < PySequence_Size(info.pObject))
{
PyObject *row = PySequence_GetItem(info.pObject, i);doesn't check the return value from the first invocation of Note that this second bug means that the comment in the test immediately below this loop if (!ncols)
{
// TVP has no columns --- is null
info.nested = 0;
}is wrong. If we get to that code before crashing then the tvp will not have been |
Unfortunately due to how TVPs are implemented at the ODBC driver level, it is not possible to determine the correct binding types via the usual SQLDescribeParam method if no data is provided to detect. However, it should be possible to avoid the crash by adding an extra condition to the loop: |
|
I tried that, but that just defers the core dump. The ODBC documentation is not as helpful as I would like for this area, but we should be able to get the data source to provide the information needed for the calls to |
|
Anyway, back to the ODBC API. If I can get the back end to cough up the name of the TVP type, I think we'll be home free. I'm looking for the right field to request in a |
|
Possibly somewhat related comment here. |
|
Bingo! |
|
Here's a first cut at a fix. The repro case runs successfully with it, though there's more to do (threading stuff, unit test, etc.), but I'm reluctant to go much further toward rolling a PR without some feedback. index b12dce9..f74ff0c 100644
--- a/src/cursor.h
+++ b/src/cursor.h
@@ -68,6 +68,9 @@ struct ParamInfo
struct ParamInfo *nested;
SQLLEN curTvpRow;
+ // For TVPs, the name of the table type.
+ SQLWCHAR* type_name;
+
// Optional data. If used, ParameterValuePtr will point into this.
union
{
diff --git a/src/params.cpp b/src/params.cpp
index 20548c2..293ffab 100644
--- a/src/params.cpp
+++ b/src/params.cpp
@@ -606,6 +606,8 @@ static void FreeInfos(ParamInfo* a, Py_ssize_t count)
pyodbc_free(a[i].ParameterValuePtr);
if (a[i].ParameterType == SQL_SS_TABLE && a[i].nested)
FreeInfos(a[i].nested, a[i].maxlength);
+ if (a[i].type_name)
+ pyodbc_free(a[i].type_name);
Py_XDECREF(a[i].pObject);
}
pyodbc_free(a);
@@ -1200,6 +1202,29 @@ static bool GetTableInfo(Cursor *cur, Py_ssize_t index, PyObject* param, ParamIn
SQLDescribeParam(cur->hstmt, index + 1, &tvptype, 0, 0, 0);
}
+ if (!nrows)
+ {
+ // With no table rows to examine, we'll need the TPV's type. Note that we don't
+ // really need the values provided by SQLDescribeParam, but without invoking
+ // that function, the next call (to get the table type name) will fail for
+ // some reason. Errors will be detected later (when the needed type_name
+ // pointer is found to be NULL).
+ SQLHANDLE ipd;
+ SQLWCHAR name[257] = {0};
+ SQLINTEGER len = 0;
+ SQLULEN parm_size;
+ SQLSMALLINT decdigits, nulls, datatype;
+ SQLGetStmtAttr(cur->hstmt, SQL_ATTR_IMP_PARAM_DESC, &ipd, SQL_IS_POINTER, &len);
+ SQLDescribeParam(cur->hstmt, index + 1, &datatype, &parm_size, &decdigits, &nulls);
+ SQLRETURN rc = SQLGetDescFieldW(ipd, index + 1, SQL_CA_SS_TYPE_NAME, name, sizeof(name), &len);
+ if (SQL_SUCCEEDED(rc))
+ {
+ info.type_name = (SQLWCHAR*)pyodbc_malloc(len + sizeof(SQLWCHAR));
+ memcpy(info.type_name, name, len);
+ info.type_name[len / sizeof(SQLWCHAR)] = 0;
+ }
+ }
+
info.pObject = param;
Py_INCREF(param);
info.ValueType = SQL_C_BINARY;
@@ -1434,8 +1459,14 @@ bool BindParameter(Cursor* cur, Py_ssize_t index, ParamInfo& info)
return false;
}
- Py_ssize_t i = PySequence_Size(info.pObject) - info.ColumnSize;
+ // Make sure the user didn't pass None for the TVP.
+ Py_ssize_t i = PySequence_Size(info.pObject);
+ if (i < 0)
+ {
+ RaiseErrorV(0, ProgrammingError, "A TVP must be a Sequence.");
+ }
Py_ssize_t ncols = 0;
+ i -= info.ColumnSize;
while (i < PySequence_Size(info.pObject))
{
PyObject *row = PySequence_GetItem(info.pObject, i);
@@ -1455,12 +1486,7 @@ bool BindParameter(Cursor* cur, Py_ssize_t index, ParamInfo& info)
ncols = PySequence_Size(row);
i++;
}
- if (!ncols)
- {
- // TVP has no columns --- is null
- info.nested = 0;
- }
- else
+ if (ncols)
{
PyObject *row = PySequence_GetItem(info.pObject, PySequence_Size(info.pObject) - info.ColumnSize);
Py_XDECREF(row);
@@ -1498,6 +1524,64 @@ bool BindParameter(Cursor* cur, Py_ssize_t index, ParamInfo& info)
}
}
+ else
+ {
+ // An empty sequence was passed for the TVP, so we can't use the data to figure even how
+ // many columns the TVP has. Get some help from the server.
+ SQLHANDLE chstmt;
+ ParamInfo* column = 0;
+
+ if (!info.type_name)
+ {
+ RaiseErrorV(0, ProgrammingError, "Unable to find TVP type name.");
+ return false;
+ }
+ ret = SQLAllocHandle(SQL_HANDLE_STMT, GetConnection(cur)->hdbc, &chstmt);
+ if (!SQL_SUCCEEDED(ret))
+ {
+ RaiseErrorFromHandle(cur->cnxn, "SQLAllocHandle", GetConnection(cur)->hdbc, cur->hstmt);
+ return false;
+ }
+ ret = SQLSetStmtAttr(chstmt, SQL_SOPT_SS_NAME_SCOPE, (SQLPOINTER)SQL_SS_NAME_SCOPE_TABLE_TYPE,
+ SQL_IS_UINTEGER);
+ if (!SQL_SUCCEEDED(ret))
+ {
+ RaiseErrorFromHandle(cur->cnxn, "SQLSetStmtAttr", GetConnection(cur)->hdbc, cur->hstmt);
+ return false;
+ }
+ ret = SQLColumnsW(chstmt, NULL, 0, NULL, 0, info.type_name, SQL_NTS, NULL, 0);
+ if (!SQL_SUCCEEDED(ret))
+ {
+ RaiseErrorFromHandle(cur->cnxn, "SQLColumnsW", GetConnection(cur)->hdbc, cur->hstmt);
+ return false;
+ }
+ while (SQL_SUCCEEDED(SQLFetch(chstmt)))
+ {
+ if (ncols++)
+ {
+ pyodbc_realloc((BYTE**)&info.nested, ncols * sizeof(ParamInfo));
+ column = info.nested + ncols - 1;
+ }
+ else
+ column = info.nested = (ParamInfo*)pyodbc_malloc(ncols * sizeof(ParamInfo));
+ memset((void*)column, 0, sizeof(ParamInfo));
+ column->ValueType = SQL_C_DEFAULT;
+ column->ParameterType = SQL_VARCHAR;
+ column->StrLen_or_Ind = SQL_DATA_AT_EXEC;
+ }
+ for (i = 0; i < ncols; ++i) {
+ // TODO Replace hard-coded type information.
+ ret = SQLBindParameter(cur->hstmt, (SQLUSMALLINT)(i + 1), SQL_PARAM_INPUT,
+ SQL_C_DEFAULT, SQL_VARCHAR, 0, 0, info.nested + i, 0,
+ &info.nested[i].StrLen_or_Ind);
+ if (!SQL_SUCCEEDED(ret))
+ {
+ RaiseErrorFromHandle(cur->cnxn, "SQLBindParameter", GetConnection(cur)->hdbc, cur->hstmt);
+ return false;
+ }
+ }
+ }
+
ret = SQLSetStmtAttr(cur->hstmt, SQL_SOPT_SS_PARAM_FOCUS, 0, SQL_IS_INTEGER);
if (!SQL_SUCCEEDED(ret))
{
diff --git a/src/pyodbc.h b/src/pyodbc.h
index 6bef3e6..55f7af4 100644
--- a/src/pyodbc.h
+++ b/src/pyodbc.h
@@ -82,6 +82,14 @@ typedef int Py_ssize_t;
#define SQL_CA_SS_TYPE_NAME 1227
#endif
+#ifndef SQL_SOPT_SS_NAME_SCOPE
+#define SQL_SOPT_SS_NAME_SCOPE 1237
+#endif
+
+#ifndef SQL_SS_NAME_SCOPE_TABLE_TYPE
+#define SQL_SS_NAME_SCOPE_TABLE_TYPE 1
+#endif
+
inline bool IsSet(DWORD grf, DWORD flags)
{
return (grf & flags) == flags; |
|
As expected, there is a performance penalty when we ask the back end for help in figuring out how many columns an empty TVP has. To measure, I moved my testing to a development configuration in which the database server and client are not on the same machine. It took five seconds to invoke the stored procedure 1,000 times with two rows in the TVP. Also five seconds with one row per call. When the calls use an empty TVP the 1,000 invocations take seven seconds. So in our use case (see earlier comment above), you'd be hard pressed to measure the penalty, and even in the degenerate case in which most (or all) of the TVPs are empty, it would be hard to argue that an exorbitant price is being exacted for avoiding crashing the interpreter. I mean, it's nice to have a performant database module, but there's only so much you should be expected to do to save developers from their own misguided application designs, right? 😉 |
|
While we've got this part of the code under the microscope (and while I'm waiting for some feedback), can I ask what's going on in this code in if (!nskip)
{
// Need to describe in order to fill in IPD with the TVP's type name, because user has not provided it
SQLSMALLINT tvptype;
SQLDescribeParam(cur->hstmt, index + 1, &tvptype, 0, 0, 0);
}Looks like a mistake (on several fronts), but I'm open to other explanations. [Hint: |
|
I don't think such complexity is necessary, since the default value of a TVP is empty and so it should be possible to just skip binding it completely; the fact that skipping the first loop with an additional condition will cause a crash later just means that other place should also skip.
The comment there explains why, but you noticed this behaviour yourself when you wrote this comment in your proposed change:
|
|
I'll take your word for it that just calling In any case, I'm at a dead end. I tried your suggestion of adding the extra test to the 1 According to Microsoft's docs, "After binding the table-valued parameter, the application must then bind each table-valued parameter column." |
Without this patch, passing a TVP with no rows will crash the Python interpreter. Solution from v-chojas. See #772.
|
Should have been fixed by PR #779 . Feel free to re-open if still an issue. |
Environment
Issue
The text was updated successfully, but these errors were encountered: