Always dispose of engines in create_database and drop_database

[ashanbrown]

To ensure timely cleanup of resources, dispose of any engines after using them.

This need surfaced because psyopg3 reports a warning later if a connection if gc'd, making it desirable to dispose of the engine (and its connection pool) even if the command fails.

This pattern is already in place for database_exists, so I'm just suggesting we extend it for create_engine and drop_database.

[kurtmckee]

@ashanbrown Thanks for submitting this.

Please add a test that demonstrates that this issue both exists and is fixed by this change. Thanks!

[kurtmckee]

This kicks off a try/except block that is larger than it should be.

I recommend modifying this so that the conditionals exclusively determine what the text variable should be.

text = ''
if dialect_name == 'postgresql':
    if not template:
        template = 'template1'
    text = "CREATE DATABASE {} CHARACTER SET = '{}'".format(
        quote(conn, database), encoding
    )
elif dialect_name == 'mysql':
    ...
    text = ...
elif dialect_name == 'sqlite' and database != ':memory:':
    if database:
        text = 'CREATE TABLE DB(id int); DROP TABLE DB;'
else:
    text = f'CREATE DATABASE {quote(conn, database)}'

and then reduce the try/except block to something like this, which accommodates the possibility that text is an empty string.

if text:
    try:
        with engine.begin() as conn:
            conn.execute(sa.text(text))
    finally:
        engine.dispose()

(It's possible that engine.dispose() still needs to be run, so please check whether the if text: conditional should actually be inside the try block.)

[ashanbrown]

At least one of the examples (sqlite), requires multiple executing multiple text values, so this could possibly get a little messy. If we just had a context manager provide an engine that is always disposed, would that resolve your concern? Then we could also clean up duplicate code between create_database and drop_database.

[kurtmckee]

I think I addressed that by putting both commands into the same line. I don't anticipate it being messy, but could be wrong.

elif dialect_name == 'sqlite' and database != ':memory:':
    if database:
        text = 'CREATE TABLE DB(id int); DROP TABLE DB;'

[ashanbrown]

We could also mock the dispose method if this doesn't seem sufficient.