Include PostgreSQL version in environment_info table

[Ryan J Ollos]

The MySQL client and server version is shown if using MySQL, but the Postgres version is not shown if using PostgreSQL.

[Ryan J Ollos]

I had an unexpected finding. The first database interaction in a test case is through the TransactionContextManager: branches/1.2-stable/trac/test.py@15048:389,405#L388​. It seems IDatabaseConnector.get_connection isn't called when a TransactionContextManager is put in the pool.

I'm not sure if that makes sense. Possible changes in log:rjollos.git:t12496_postgresql_version_in_environment_info.

Note: assertRegexpMatches is only available in Python 2.7 and the test is only intended to be committed to the trunk. Also, I haven't run tests on all platforms.

[Jun Omae]

Replying to Ryan J Ollos:

I'm not sure if that makes sense. Possible changes in log:rjollos.git:t12496_postgresql_version_in_environment_info.

I don't think versions of libpq and pg_dump would be same. It is possible that a user installs multiple versions of postgresql client, install to directory which isn't in PATH (e.g. /usr/local/postgresql-9.x, using only LD_LIBRARY_PATH).

I consider we should use psycopg2.libpq_version (since psycopg 2.7) or PQlibVersion() in libpq via ctypes: [94d79aa7d/jomae.git] (untested with Windows)

[Ryan J Ollos]

Sounds good. Do you have any concerns about [93313f31/rjollos.git#file0]? Maybe my solution is just a hack.

[Jun Omae]

Replying to Ryan J Ollos:

Do you have any concerns about [93313f31/rjollos.git#file0]? Maybe my solution is just a hack.

I consider that is not a good idea.

• If database connection string is incorrect, get_systeminfo() always would fail.
• Database connection retrieved from connector.get_connection(**args) would be leaked. Must close it.

[Ryan J Ollos]

I don't have any better ideas to resolve this. Feel free to take over the ticket if you'd like.

[Jun Omae]

Another idea: [f85b868eb/jomae.git] (jomae.git@t12496.2_trunk)

• Add get_system_info() to IDatabaseConnector.
• Name and version of the database are returned by Environment.get_system_info() with DatabaseManager(env).get_connector().
• The name and version are returned even if database isn't connected yet ((not-connected) would be shown as database version).

[Ryan J Ollos]

That looks good. It would probably make sense to have DatabaseManager implement ISystemInfoProvider, but that appears to result in a circular import.

[Ryan J Ollos]

With the changes introduced, system_info should probably not be lazily-evaluated: trunk/trac/env.py@15058:312#L304​. We could modify the test case to test Environment.system_info rather than Environment.get_system_info. The former would be more like a functional test. I tried to make the to the test case, but ran into an issue that get_system_info from Environment is not returned from Environment.system_info. It seems to be most directly due to EnvironmentStub not being found in cmpmgr: trunk/trac/core.py@14978:82#L63​.

[Ryan J Ollos]

Additional changes in log:rjollos.git:t12496.3_trunk.

I consider the changes after and including [1ca2ba02/rjollos.git] to be experimental. While moving the interfaces from trac.env to trac.api resolves the circular import, I'm not sure yet that it's the right change to make.

[Ryan J Ollos]

Replying to Ryan J Ollos:

While moving the interfaces from trac.env to trac.api resolves the circular import, I'm not sure yet that it's the right change to make.

More generally, it looks like we'd avoid circular import problems by:

• Putting interfaces and abstract base classes in api.py, but don't put implementations in api.py, such as DatabaseManager​.
• Don't import api in __init__. This is done, for example, in trac.admin.__init__​.

[Jun Omae]

Looks good to me except two things.

[a0df6a00a/jomae.git]: I think server_version and _client_version in trac.db.postgres_backend should be a tuple of the version to be consistent. Thoughts?

[380e93192/jomae.git]: I believe a connection retrieved from connector.get_connection() in trac.db.api is leaked. The statement should be removed, otherwise call close() of the connection.

[Ryan J Ollos]

Those changes look good. I also had concerns about the string vs tuple inconsistency and I like the solution of having helper functions. I'll review the changes again at end of the week and push them.

[Ryan J Ollos]

Rebased changes in log:rjollos.git:t12496.5. I squashed and reordered the resulting changesets into an order suitable for pushing to the main repository.

[Ryan J Ollos]

Committed to trunk in [15148:15151].

It looks like builds on AppVeyor were failing even when I pushed to my GitHub fork, but I didn't notice because I'm apparently not getting notifications. I'll investigate.

[Jun Omae]

Sorry, my mistake. It's needed to use libpq instead of pq on Windows. Also, UnboundLocalError occurs if missing libpq library.

• [721b97872/jomae.git]
• ​https://ci.appveyor.com/project/jun66j5/trac/build/t12496.5.11

[Ryan J Ollos]

Thanks for the fix. Committed in r15152.

[Ryan J Ollos]

#13032 addresses the server version for PostgreSQL ≥ 10.