database.read and database.execute_sql should not return the session to the pool till the stream is closed

[vkedia]

Cloud Spanner API says that at any point in time there can be at most one active transaction. But the way database.read and database.execute_query have been implemented, they return the session to the pool immediately. Now that session can be reused for doing something else even before the stream returned by previous database.read has completed. This needs to be fixed so that the sessions is returned to the pool only after the iterator has been completely consumed or closed.
This needs to be done before Beta.

[vkedia]

cc @lukesneeringer @bjwatson

[bjwatson]

Thanks @vkedia. At a 100K' level, this does sound like a significant usability issue. I'm okay with this being Beta blocking, unless @lukesneeringer or @tseaver have an opinion to the contrary.

[vkedia]

@bjwatson This also might be a breaking change depending on how this resolved. For eg the easiest resolution would be to just remove these methods since one can always do the same thing by using a Snapshot.

[bjwatson]

@vkedia That sounds like an easy fix, but definitely surface breaking.

[tseaver]

I don't think using Database.snapshot solves the problem. Using the current methods:

results = database.execute_sql(SQL)  # returns StreamedResultSet

versus using a snapshot:

with database.snapshot() as snapshot:
    results = snapshot.execute_sql(SQL) # returns StreamedResultSet

The 'StreamedResultSet' we return wraps a low-level GAPIC entity with no existing / obvious link to our session. Given that the user may not iterate to the end (or at all), we would likely have to had some machinery for a "deferred return to pool" and trigger that from StreamedResultSet.__del__.

The other option would be to require that user applications complete all iteration over the StreamedResultSet inside the with database.snapshot() as snapshot: block.

[vkedia]

It seems fair to me to expect that user will not access the results object outside of the context block. We can possibly do something to invalidate the iterators when the snapshot is checked back in.

[tseaver]

Via #3787.