Edgewall Software

Opened 6 years ago

Last modified 3 months ago

#12496 closed enhancement

Include PostgreSQL version in environment_info table — at Version 14

Reported by: Ryan J Ollos Owned by:
Priority: normal Milestone: 1.3.1
Component: database backend Version:
Severity: normal Keywords: postgres
Cc: Branch:
Release Notes:
  • PostgreSQL version is shown in System Information table.
  • The minimum PostgreSQL version is now 9.1.0.
API Changes:
  • Added get_system_info to IDatabaseConnector interface.
  • DatabaseManager implements ISystemInfoProvider to return the database version. MySQLConnector, PostgreSQLConnector and SQLiteConnector no longer implement ISystemInfoProvider.
  • Moved IEnvironmentSetupParticipant and ISystemInfoProvider from trac.env to trac.api.
Internal Changes:

Description

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

Change History (16)

by Ryan J Ollos, 6 years ago

by Ryan J Ollos, 6 years ago

comment:1 by Ryan J Ollos, 6 years ago

Milestone: next-stable-1.0.x1.2
Owner: set to Ryan J Ollos
Status: newassigned

comment:2 by Ryan J Ollos, 6 years ago

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.

in reply to:  2 comment:3 by Jun Omae, 6 years ago

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)

Last edited 6 years ago by Jun Omae (previous) (diff)

comment:4 by Ryan J Ollos, 6 years ago

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

comment:5 by Ryan J Ollos, 6 years ago

Milestone: 1.21.3.1

in reply to:  4 comment:6 by Jun Omae, 6 years ago

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.

comment:7 by Ryan J Ollos, 6 years ago

Milestone: 1.3.1next-dev-1.3.x
Owner: Ryan J Ollos removed
Status: assignednew

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

comment:8 by Jun Omae, 6 years ago

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).

comment:9 by Ryan J Ollos, 6 years ago

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

comment:10 by Ryan J Ollos, 6 years ago

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.

comment:11 by Ryan J Ollos, 6 years ago

Milestone: next-dev-1.3.x1.3.1

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.

in reply to:  11 comment:12 by Ryan J Ollos, 6 years ago

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__.

comment:13 by Jun Omae, 6 years ago

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.

comment:14 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

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.

Note: See TracTickets for help on using tickets.