Skip to content

Conversation

@gargsaumya
Copy link
Contributor

@gargsaumya gargsaumya commented May 29, 2025

This pull request introduces connection pooling to improve database connection management in the mssql_python project. Key changes include the addition of a ConnectionPool class, updates to the Connection class to support pooling, and modifications to the Python bindings to expose pooling functionality.

Connection Pooling Implementation:

  • New ConnectionPool and ConnectionPoolManager classes: Added ConnectionPool for managing reusable database connections and ConnectionPoolManager as a singleton to manage multiple pools. These classes handle connection acquisition, release, and idle timeout logic. (mssql_python/pybind/connection/connection_pool.cpp - [1] mssql_python/pybind/connection/connection_pool.h - [2]

Updates to Connection Class:

  • Support for connection pooling: Refactored Connection class to include attributes and methods for pooling, such as _lastUsed, isAlive(), and reset(). Added ConnectionHandle as a wrapper for pooled connections. (mssql_python/pybind/connection/connection.cpp - [1] [2] [3] [4] [5] mssql_python/pybind/connection/connection.h - [6] [7]

Python Bindings:

  • Exposed pooling functionality: Updated Python bindings to replace the Connection class with ConnectionHandle and added a global enable_pooling function for configuring pooling parameters. (mssql_python/pybind/ddbc_bindings.cpp - [1] [2]

Build System:

Header File Updates:

  • Included new headers: Added connection_pool.h to relevant files for access to pooling functionality. (mssql_python/pybind/connection/connection.cpp - [1] mssql_python/pybind/ddbc_bindings.cpp - [2]

Checklist

  • Tests Passed (if applicable)

Copilot AI review requested due to automatic review settings May 29, 2025 20:34
@gargsaumya gargsaumya changed the title Saumya/pool c++ FEAT: C++ support for pooling May 29, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Refactors database connection and cursor management by replacing low-level ODBC handle code with a high-level ddbc_bindings.Connection, introduces a connection pool, and streamlines transaction logic.

  • Switches to ddbc_bindings.Connection and ConnectionPoolManager to manage connections and pooling.
  • Removes legacy ODBC wrapper functions and simplifies cursor and transaction methods.
  • Updates build configuration and tests to align with the new abstraction.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_005_exceptions.py Adjust expected exception type and message
mssql_python/pybind/ddbc_bindings.h/.cpp Added pybind11 includes, cleaned up wrappers, and implemented SqlHandle destructor
mssql_python/pybind/connection/connection_pool.h/.cpp Added ConnectionPool and ConnectionPoolManager
mssql_python/pybind/connection/connection.h/.cpp Updated Connection and ConnectionHandle to support pooling and attribute logic
mssql_python/pybind/CMakeLists.txt Included connection_pool.cpp in the build
mssql_python/cursor.py Simplified statement handle allocation and removal of closed-check
mssql_python/connection.py Replaced manual handle management with ddbc_bindings.Connection
Comments suppressed due to low confidence (2)

tests/test_005_exceptions.py:128

  • [nitpick] Matching the full error message is brittle. Consider asserting substring or using a regex to validate key parts (e.g. "keyword supplied").
with pytest.raises(RuntimeError) as excinfo:

mssql_python/cursor.py:425

  • _reset_cursor frees hstmt but does not reallocate a new statement handle. Consider calling self._initialize_cursor() after free() to restore hstmt.
self.hstmt.free()

@microsoft microsoft deleted a comment from Copilot AI May 29, 2025
@gargsaumya gargsaumya force-pushed the saumya/pool-c++ branch 3 times, most recently from 37011f0 to d742791 Compare May 30, 2025 07:39
sumitmsft
sumitmsft previously approved these changes May 30, 2025
Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

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

I have a few comments. Some of them can be put into backlog for future improvements. A couple of them can be thought to get fixed right away.

@gargsaumya gargsaumya requested a review from sumitmsft June 1, 2025 06:08
@gargsaumya gargsaumya merged commit 4409af5 into main Jun 2, 2025
4 checks passed
@gargsaumya gargsaumya mentioned this pull request Jun 2, 2025
7 tasks
Sign up for free to 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.

4 participants