Add lazy evaluation of server_version_info

[mdesmet]

Description

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

[aalbu]

That's cool. Have you checked whether the getter is ever called in the process of creating a connection/running some SQL/closing the connection? It seemed to me like the property is never accessed, but I'm not positive.

[hashhar]

This is something that the SQLA engine interface provides so user scripts/programs can make use of it. Core itself doesn't use it other than for populating a server_version_info attribute.

Some dialects also use it internally for version dependent logic, an example is Oracle - https://github.com/zzzeek/sqlalchemy/blob/671647c8574bd6ff4c39fe503a2b1958a802772d/lib/sqlalchemy/dialects/oracle/base.py#L1487

[hashhar]

How much does this really help, it's only called once per connection. So only if someone is using NullPool or creating new engine everytime.

[mdesmet]

Well, it is called once per connection and not used in 99.999% of the cases, so making it lazily evaluated makes definitely a difference as it removes these series of requests from EVERY sqlalchemy usage.

[hashhar]

tests are failing

[hashhar]

LGTM % comment.

[hashhar]

Well, it is called once per connection and not used in 99.999% of the cases, so making it lazily evaluated makes definitely a difference as it removes these series of requests from EVERY sqlalchemy usage.

I was thinking in terms of number of requests.
This function isn't executed per-query, so it only reduces 1 call for entire application/connection (which is generally much lower than number of queries).

i.e. for someone who uses only a few connections there is no benefit of this lazy evaluation.

[nineinchnick]

Why not use the decorator @property?

[mdesmet]

Because server_version_info is not in our codebase but in sqlalchemy's, so we need to resort to this type of instantiation.

[nineinchnick]

I mean why use the property() function instead of a decorator.

[mdesmet]

Have a look at sqlalchemy's code. Simply adding the property wouldn't work as it would be overwritten.

[nineinchnick]

Thanks, I get it now. But it seems we're doing a hack here where we override the server info property with one that has a getter for lazy evaluation, but a noop setter. Then we return a None, which sqlalchemy tries to assign to that prop, but that'll get ignored because go the setter. Is it worth adding a comment explaining this, if I got it right?

[nineinchnick]

When working with a server that's geographically far away, or over a bad network connection, the overhead of the extra query can be significant, especially when executing short/simple queries. IMO, it is very valuable to avoid this unnecessary query.

[nineinchnick]

Note that I tested today that getting the server version info is done only once after creating the engine, even if connections are not reused (using a NullPool and closing the connection after every query).