Bugfix/sql param data memory leak

[Mizaro]

Fixing "Memory leak after getting [42000][ODBC Driver 17 for SQL Server] (1105) (SQLParamData) Error #702"

[mkleehammer]

I've posted a question to the issue.

[Mizaro]

I have answered and changed the code, can you please take another look?

[Mizaro]

@mkleehammer

[Mizaro]

Waiting for your response.

[Mizaro]

@mkleehammer

[Mizaro]

Reviewed my changes. Seems OK to me.

[gordthompson]

@mkleehammer – I've just had another look at this and using test code

from time import time
import psutil
import pyodbc

print(f"pyodbc {pyodbc.version}")
cnxn = pyodbc.connect("DSN=mssqlLocal", autocommit=True)
process = psutil.Process()


def print_status(msg, t0=None):
    s = f"{msg}: "
    mb = process.memory_info().vms / 1048576
    s += f"vms {mb:0.1f} MiB"
    if t0:
        s += f", {(time() - t0):0.1f} sec."
    print(s)


print_status("startup")
num_chars = 10_000_000
data = (1, "x" * num_chars, "2012-02-31")
print_status("data loaded")

table_name = "pd_test"
col_names = ["id", "txt_col", "dtm_col"]
ins_sql = f"INSERT INTO {table_name} ({','.join(col_names)}) VALUES ({','.join('?' * len(col_names))})"
for iteration in range(5):
    t0 = time()
    crsr = cnxn.cursor()
    crsr.execute(f"DROP TABLE IF EXISTS {table_name}")
    crsr.execute(
        f"CREATE TABLE {table_name} (id int, txt_col varchar(max), dtm_col datetime2)"
    )
    try:
        crsr.execute(ins_sql, data)
    except pyodbc.DataError:
        # (Feb 31 is an invalid date)
        pass
    crsr.close()
    print_status(f"iteration {iteration}", t0)

with python -m pip install git+https://github.com/mkleehammer/pyodbc I get

pyodbc 4.0.31b56
startup: vms 6.4 MiB
data loaded: vms 15.9 MiB
iteration 0: vms 35.1 MiB, 0.9 sec.
iteration 1: vms 54.2 MiB, 1.0 sec.
iteration 2: vms 73.3 MiB, 1.0 sec.
iteration 3: vms 92.4 MiB, 1.0 sec.
iteration 4: vms 111.5 MiB, 1.0 sec.

and with python -m pip install git+https://github.com/Mizaro/pyodbc@bugfix/SQLParamData_MemoryLeak I get

pyodbc 4.0.29b72+bugfix/SQLParamData_MemoryLeak
startup: vms 6.4 MiB
data loaded: vms 15.9 MiB
iteration 0: vms 16.0 MiB, 1.0 sec.
iteration 1: vms 16.0 MiB, 1.0 sec.
iteration 2: vms 16.0 MiB, 1.0 sec.
iteration 3: vms 16.0 MiB, 1.0 sec.
iteration 4: vms 16.0 MiB, 1.0 sec.

Can we merge this one now?

[Mizaro]

@gordthompson Sorry for disappearing for a while.

Is there something I can do to promote this PR?

[mkleehammer]

Was it decided that this is no longer needed because #832 fixes the leak? I'm not sure swapping the two free calls would make a difference as they are not coupled.

[gordthompson]

@mkleehammer - #832 was merged by 716572a. When I run my test code above against the current master branch the leak is still there:

pyodbc 4.0.33b8
startup: vms 37.4 MiB
data loaded: vms 47.0 MiB
iteration 0: vms 66.0 MiB, 28.9 sec.
iteration 1: vms 85.1 MiB, 18.6 sec.
iteration 2: vms 104.2 MiB, 16.7 sec.
iteration 3: vms 123.3 MiB, 18.0 sec.
iteration 4: vms 142.3 MiB, 13.1 sec.

so this fixes a different leak. (This relates to .execute() while #832 was a fast_executemany issue.)

[mkleehammer]

I ran the test above with slight modifications for Linux & Postgres, from commit 4a7c583, and I'm not seeing any leaks. I'll have to test on my Windows laptop.

I still don't see how swapping those two lines could affect anything. I'm concerned we don't actually know the cause.

Note that it could be a bug in the driver, so we do need to track those versions too.

[gordthompson]

@mkleehammer - FWIW, my test earlier today was also on Linux.

Just tested again with PostgreSQL Unicode driver. Xubuntu 20.04, Python 3.8.10, unixODBC 2.3.7.

from time import perf_counter
import psutil
import pyodbc

print(f"{pyodbc.version=}")
connection_string = (
    "Driver=PostgreSQL Unicode;"
    "Server=192.168.0.199;"
    "Database=test;"
    "UID=scott;PWD=tiger;"
)
cnxn = pyodbc.connect(connection_string, autocommit=True)
print(f"{cnxn.getinfo(pyodbc.SQL_DRIVER_NAME)=}")
print(f"{cnxn.getinfo(pyodbc.SQL_DRIVER_VER)=}")
process = psutil.Process()


def print_status(msg, t0=None):
    s = f"{msg}: "
    mb = process.memory_info().vms / 1048576
    s += f"vms {mb:0.1f} MiB"
    if t0:
        s += f", {(perf_counter() - t0):0.1f} sec."
    print(s)


print_status("startup")
num_chars = 10_000_000
data = (1, "x" * num_chars, "2012-02-31")
print_status("data loaded")

table_name = "pd_test"
col_names = ["id", "txt_col", "dtm_col"]
ins_sql = f"INSERT INTO {table_name} ({','.join(col_names)}) VALUES ({','.join('?' * len(col_names))})"
for iteration in range(5):
    t0 = perf_counter()
    crsr = cnxn.cursor()
    crsr.execute(f"DROP TABLE IF EXISTS {table_name}")
    crsr.execute(
        f"CREATE TABLE {table_name} (id int, txt_col text, dtm_col timestamp)"
    )
    try:
        crsr.execute(ins_sql, data)
    except pyodbc.DataError:
        # (Feb 31 is an invalid date)
        pass
    crsr.close()
    print_status(f"iteration {iteration}")

With current master (e67ad89):

pyodbc.version='4.0.33b26'
cnxn.getinfo(pyodbc.SQL_DRIVER_NAME)='psqlodbcw.so'
cnxn.getinfo(pyodbc.SQL_DRIVER_VER)='12.01.0000'
startup: vms 43.9 MiB
data loaded: vms 53.4 MiB
iteration 0: vms 88.5 MiB
iteration 1: vms 133.1 MiB
iteration 2: vms 152.1 MiB
iteration 3: vms 171.2 MiB
iteration 4: vms 190.3 MiB

With Mizaro's patch (850a8dd):

pyodbc.version='4.0.31b63+bugfix/SQLParamData_MemoryLeak'
cnxn.getinfo(pyodbc.SQL_DRIVER_NAME)='psqlodbcw.so'
cnxn.getinfo(pyodbc.SQL_DRIVER_VER)='12.01.0000'
startup: vms 43.9 MiB
data loaded: vms 53.4 MiB
iteration 0: vms 69.4 MiB
iteration 1: vms 69.4 MiB
iteration 2: vms 69.4 MiB
iteration 3: vms 69.4 MiB
iteration 4: vms 69.4 MiB

[Mizaro]

Was it decided that this is no longer needed because #832 fixes the leak? I'm not sure swapping the two free calls would make a difference as they are not coupled.

@mkleehammer This is a different issue, that maybe only looks alike.

@gordthompson Thanks for retesting it! I appreciate it.

I ran the test above with slight modifications for Linux & Postgres, from commit 4a7c583, and I'm not seeing any leaks. I'll have to test on my Windows laptop.

I still don't see how swapping those two lines could affect anything. I'm concerned we don't actually know the cause.

Note that it could be a bug in the driver, so we do need to track those versions too.

About how swapping those two lines can help.

FreeParameterInfo sets cur->paramcount to zero, while FreeParameterData calls FreeInfos with cur->paramcount.
As far as I remember, when swapping the lines, cur->paramInfos are being freed without a memory leak.

We can also see my explanation here #702 (comment),

Variable cur->paramInfos that is passed to ParamInfo* a is not null and holds an object in memory.

[Mizaro]

@mkleehammer, @gordthompson Is there a plan to merge this?

[gordthompson]

Looks good to me.