BUG: Allow read_sql to work with chunksize.
State: closed

- [X] closes #50199
- [X] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [X] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [ ] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.

This fixes a bug which I introduced in #49531.  That PR refactored code so that `SQLDatabase` only accepts a sqlalchemy `Connection` and not an `Engine`.  I used a context manager to handle cleanup.  However, this can break `read_sql` with `chunksize`, because the caller might be iterating over the chunks after the connection is closed.  I added tests with chunksize, and I use `NullPool` with `create_engine`, because this is more likely to cause the connection to close.  I also created a new string connectable to test with.

Labels:
name: IO SQL
description: to_sql, read_sql, read_sql_query

Comments:
@mroeschke , an alternative to this PR, which would also address the bug, would be to stop allowing `str` and `sqlalchemy.engine.Engine` types to be passed as connection objects to any sql methods.  It looks like support for `sqlalchemy.engine.Connection` was added in response to #7877, and support for `str` was added in response to #10654.  I personally prefer to pass a `sqlalchemy.engine.Connection` so that I have more control, and this would also simplify the code in `pandas.io.sql`.
> would be to stop allowing str and sqlalchemy.engine.Engine types to be passed as connection objects to any sql methods.

Unfortunately, I don't think this would be appropriate since

1. This would need a full major release (2.0 -> 3.0) to deprecate these input types
2. This would be removing useful functionality a break a lot of downstream users
Ok, that makes sense.  It might be that I need to add `__enter__` and `__exit__` methods to `SQLDatabase` and create engines and connections inside of the class.  They would either be cleaned up in the `__exit__` method or within the generator returned by `read_sql` if `chunksize is not None`. The `@contextmanager` which I applied to `pandasSQL_builder` in #49531 would be absorbed into `SQLDatabase`, and I wouldn't need the `_read_helper` method.
@mroeschke I created an `ExitStack` to handle commiting the transaction or closing the `Connection`, if applicable.  In a later PR, it will also dispose of the `Engine` if the user passes a URI string.  `ExitStack.close` either gets called in `SQLDatabase.__exit__` or `_cleanup_after_generator`, depending on whether `chunksize is None`.
@mroeschke , do the changes I made look ok?  I also updated my original PR, #48576 , so that the `Engine` disposal is also carried out by the `ExitStack` if necessary.

> @mroeschke I created an `ExitStack` to handle commiting the transaction or closing the `Connection`, if applicable. In a later PR, it will also dispose of the `Engine` if the user passes a URI string. `ExitStack.close` either gets called in `SQLDatabase.__exit__` or `_cleanup_after_generator`, depending on whether `chunksize is None`.

cc @mroeschke 

Could you have a look? I think this is ok as is
Thanks @cdcadman sorry for the delay here
Thanks @mroeschke and I agree that would have been better to put `with exit_stack:` inside of `_query_iterator`.
Can you create a small pr for it then? Such things are easy to review and can be merged without much discussion