Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

Last modified 15 months ago

#12496 closed enhancement (fixed)

Include PostgreSQL version in environment_info table

Reported by: Ryan J Ollos Owned by: Jun Omae
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.

Description

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

Attachments (2)

Screen Shot 2016-05-31 at 11.52.09.png (60.5 KB ) - added by Ryan J Ollos 3 years ago.
Screen Shot 2016-05-31 at 11.52.26.png (59.0 KB ) - added by Ryan J Ollos 3 years ago.

Download all attachments as: .zip

Change History (22)

by Ryan J Ollos, 3 years ago

by Ryan J Ollos, 3 years ago

comment:1 by Ryan J Ollos, 3 years ago

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

comment:2 by Ryan J Ollos, 3 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, 3 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 3 years ago by Jun Omae (previous) (diff)

comment:4 by Ryan J Ollos, 3 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, 3 years ago

Milestone: 1.21.3.1

in reply to:  4 comment:6 by Jun Omae, 3 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, 3 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, 3 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, 3 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, 3 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, 3 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, 3 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, 3 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, 3 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.

comment:15 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

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.

comment:16 by Ryan J Ollos, 3 years ago

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.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:17 by Jun Omae, 3 years ago

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

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

comment:18 by Ryan J Ollos, 3 years ago

Resolution: fixed
Status: assignedclosed

Thanks for the fix. Committed in r15152.

comment:19 by Ryan J Ollos, 3 years ago

Owner: changed from Ryan J Ollos to Jun Omae

comment:20 by Ryan J Ollos, 15 months ago

#13032 addresses the server version for PostgreSQL ≥ 10.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.