Context Navigation

Modify ↓

#11512 closed enhancement (fixed)

update_sequence should use the column name when selecting the max value

Reported by:	Ryan J Ollos	Owned by:	Jun Omae
Priority:	normal	Milestone:	0.12.6
Component:	database backend	Version:
Severity:	normal	Keywords:	postgesql
Cc:	Jun Omae	Branch:
Release Notes:
API Changes:	Added `drop_table` method to the database connection classes. Added `create_tables` and `drop_tables` methods to the `DatabaseManager` class (1.0.2). `update_sequence` method of the connection classes uses the column name to select the max value. `update_sequence` and `get_last_id` methods of the connection classes and `trac.test.reset_postgres_db` quote the table and column names, fixing problems with upper case characters.
Internal Changes:

Description

To support the case that the primary key column has a name other than id, update_sequence for the Postgres backend should be modified to use the specified column name when selecting the MAX value: branches/1.0-stable/trac/db/postgres_backend.py@12500:258#L255 .

The patch was proposed by Olemis Lang in bh:#708.

Attachments (0)

Change History (33)

comment:1 by Ryan J Ollos, 11 years ago

Proposed change can be found in log:rjollos.git:t11512. I'll look into adding a unit test before the change is committed.

comment:2 by Peter Suter, 11 years ago

Nice catch. Sounds good to me.

(For reference: update_sequence was introduced in #8575.)

This API is intended for plugins, so there's no usage of it in Trac. Do you know which plugins (besides Bloodhound) already use this?

Last edited 11 years ago by Peter Suter (previous) (diff)

comment:3 by Ryan J Ollos, 11 years ago

I only find one on trac-hacks, which is Jun's th:TracMigratePlugin:

$ grep -r "\.update_sequence" trachacks.git/
trachacks.git/tracmigrateplugin/0.12/tracmigrate/admin.py:                db.update_sequence(cursor, table)

comment:4 by Jun Omae, 11 years ago

get_last_id and update_sequence methods don't work for column and/or table name with upper case letters. I think we should use quote() method for sequence name.

trac/db/postgres_backend.py

diff --git a/trac/db/postgres_backend.py b/trac/db/postgres_backend.py
index 1229169..be2e38a 100644

                class PostgreSQLConnection(ConnectionWrapper):
         return '"%s"' % identifier.replace('"', '""')
     def get_last_id(self, cursor, table, column='id'):
+        cursor.execute("""SELECT CURRVAL('"%s_%s_seq"')""" % (table, column))
+        cursor.execute("SELECT CURRVAL(%s)",
+                       (self.quote(self._sequence_name(table, column)),))
         return cursor.fetchone()[0]
     def update_sequence(self, cursor, table, column='id'):
+        cursor.execute("""
+            SELECT setval('"%s_%s_seq"', (SELECT MAX(id) FROM %s))
+            """ % (table, column, table))
+        quote = self.quote
+        cursor.execute("SELECT SETVAL(%%s, (SELECT MAX(%s) FROM %s))"
+                       % (quote(column), quote(table)),
+                       (quote(self._sequence_name(table, column)),))
     def cursor(self):
         return IterableCursor(self.cnx.cursor(), self.log)
+    def _sequence_name(self, table, column):
+        return '%s_%s_seq' % (table, column)

comment:5 by Ryan J Ollos, 11 years ago

Proposed changes in rjollos.git:t11512.2. Tested with:

$ psql --version
psql (PostgreSQL) 9.1.11
$ sqlite3 --version
3.7.15.2 2013-01-09 11:53:05 c0e09560d26f0a6456be9dd3447f5311eb4f238f
$ mysql --version
mysql  Ver 14.14 Distrib 5.5.34, for debian-linux-gnu (x86_64) using readline 6.2

comment:6 by Jun Omae, 11 years ago

All unit and functional tests pass on SQLite 3.3.6, PostgreSQL 8.1.23 and MySQL 5.0.95.

comment:7 by Ryan J Ollos, 11 years ago

Thanks for testing. Not sure not why I based the rjollos.git:t11512.2 branch on the trunk. Should we target to 0.12-stable?

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

comment:8 by Jun Omae, 11 years ago

Yes, I think we should apply to 0.12-stable.

follow-up: 11 comment:9 by Ryan J Ollos, 11 years ago

Milestone:	1.0.2

I tried making the tests more robust by moving the table creation and deletion code to the test harness. The latest changes have been added to log:rjollos.git:t11512.2.

It seems that I could just call self.env.destory_db() rather than self._cleanup_tables, however I get test failures for SQLite because there doesn't seem to be a case in the method to handle SQLite: tags/trac-1.0.1/trac/test.py@:?marks=372-379#L366 . Should we the SQLite case?

comment:10 by Ryan J Ollos, 11 years ago

Milestone:	→ 0.12.6

in reply to: 9 comment:11 by Jun Omae, 11 years ago

It seems that I could just call self.env.destory_db() rather than self._cleanup_tables, however I get test failures for SQLite because there doesn't seem to be a case in the method to handle SQLite: tags/trac-1.0.1/trac/test.py@:?marks=372-379#L366 . Should we the SQLite case?

Hmm, I cannot reproduce such errors on SQLite using rjollos.git@t11512.2 with the following changes.

trac/db/tests/api.py

diff --git a/trac/db/tests/api.py b/trac/db/tests/api.py
index a77b879..69911cd 100644

                class ConnectionTestCase(unittest.TestCase):
                     db(sql)
     def tearDown(self):
         self._cleanup_tables()
+        self.env.destroy_db()
         self.env.reset_db()
     def _cleanup_tables(self):

Also, the ConnectionTestCase is very slow on PostgreSQL and MySQL if using destroy_db().

If using SQLite in tests, on-memroy SQLite database is used at tags/trac-1.0.1/trac/test.py@:175#L163 . So, I think the destory_db() doesn't handle SQLite.

I got the following on PostgreSQL 8.1.23. If PostgreSQL 8.1 or early, DROP TABLE doesn't support IF EXISTS. See http://www.postgresql.org/docs/8.1/static/sql-droptable.html.

======================================================================
ERROR: test_get_last_id (trac.db.tests.api.ConnectionTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jun66j5/src/trac/edgewall/git/trac/db/tests/api.py", line 365, in tearDown
    self._cleanup_tables()
  File "/home/jun66j5/src/trac/edgewall/git/trac/db/tests/api.py", line 371, in _cleanup_tables
    db("DROP TABLE IF EXISTS %s" % db.quote(table.name))
  File "/home/jun66j5/src/trac/edgewall/git/trac/db/util.py", line 128, in execute
    cursor.execute(query, params)
  File "/home/jun66j5/src/trac/edgewall/git/trac/db/util.py", line 63, in execute
    r = self.cursor.execute(sql)
ProgrammingError: syntax error at or near "EXISTS" at character 15

comment:12 by Ryan J Ollos, 11 years ago

I was also concerned about the speed of the test case, but if we only use it for a few test cases as opposed to hundreds, I can't see that it would be too harmful.

I'm thinking to try and expose some additional utilities from test.py for deleting tables, which would be useful for plugins as well. I'll propose some additional changes soon.

comment:13 by Jun Omae, 11 years ago

pg_get_serial_sequence() which is used in reset_postgres_db requires quoted identifiers for table name used upper case letters, as the same as nextval(). See jomae.git@t11512.3.

trac=# CREATE TABLE "HOUR" ("ID" serial);
NOTICE:  CREATE TABLE will create implicit sequence "HOUR_ID_seq" for serial column "HOUR.ID"
CREATE TABLE
trac=# \d "HOUR"
                         Table "tractest.HOUR"
 Column |  Type   |                      Modifiers
--------+---------+-----------------------------------------------------
 ID     | integer | not null default nextval('"HOUR_ID_seq"'::regclass)

trac=# SELECT pg_get_serial_sequence('HOUR', 'ID');
ERROR:  relation "hour" does not exist
trac=# SELECT pg_get_serial_sequence('"HOUR"', 'ID');
 pg_get_serial_sequence
------------------------
 tractest."HOUR_ID_seq"
(1 row)

comment:14 by Ryan J Ollos, 11 years ago

Since [1eda8f6f/jomae.git] demonstrates that some care must be taken to properly remove a table in a way that is cross-db compatible, I was thinking it could be useful to move this code to a drop_tables method of EnvironmentStub.

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

comment:15 by Jun Omae, 11 years ago

Okay. I've revised jomae.git@t11512.3 branch and added drop_table() to each database connection class.

Because DROP TABLE IF EXISTS leads abort of transaction on PostgreSQL 8.1, PostgreSQLConnection.drop_table() method checks existence of the table if PostgreSQL 8.0 and 8.1.

comment:16 by Ryan J Ollos, 11 years ago

Two minor related changes in [12673:12674], merged in [12675].

comment:17 by Ryan J Ollos, 11 years ago

One change committed to [12682] to 1.0-stable, merged in [12683]. I squashed and rebased the other changes on the trunk: log:rjollos.git:t11512.4. Jun, could you apply [345479f3/rjollos.git] onto 0.12-stable? I'm afraid I might break the changeset given the manual edits needed to rebase it onto 0.12-stable. I have an idea that could affect the other two changesets, which I'll post in a short while.

Edit: there is an error in [ccb3f176/rjollos.git] - drop_table is not being used. I'll fix that when rebasing onto 0.12-stable.

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

follow-up: 19 comment:18 by Ryan J Ollos, 11 years ago

API Changes:	modified (diff)

There is a problem after [12682]. Running the tests with a real SQLite db results in the following if test.db doesn't exist:

$ make db=sqlite unit-test
Python version: Python 2.7.4
figleaf: 
coverage: 
PYTHONPATH=.:
TRAC_TEST_DB_URI=sqlite:test.db
server-options= -p 8000 -a '*,~/tracenvs/htdigest.realm,realm' -r -e ~/tracenvs
python setup.py egg_info
running egg_info
unrecognized .svn/entries format; skipping .
writing requirements to Trac.egg-info/requires.txt
writing Trac.egg-info/PKG-INFO
writing top-level names to Trac.egg-info/top_level.txt
writing dependency_links to Trac.egg-info/dependency_links.txt
writing entry points to Trac.egg-info/entry_points.txt
unrecognized .svn/entries format in /home/user/Workspace/t11584/teo-rjollos.git
reading manifest file 'Trac.egg-info/SOURCES.txt'
writing manifest file 'Trac.egg-info/SOURCES.txt'
python ./trac/test.py --skip-functional-tests 
Traceback (most recent call last):
  File "./trac/test.py", line 478, in <module>
    unittest.main(defaultTest='suite')
  File "/usr/lib/python2.7/unittest/main.py", line 94, in __init__
    self.parseArgs(argv)
  File "/usr/lib/python2.7/unittest/main.py", line 149, in parseArgs
    self.createTests()
  File "/usr/lib/python2.7/unittest/main.py", line 158, in createTests
    self.module)
  File "/usr/lib/python2.7/unittest/loader.py", line 128, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib/python2.7/unittest/loader.py", line 113, in loadTestsFromName
    test = obj()
  File "./trac/test.py", line 451, in suite
    suite.addTest(trac.tests.basicSuite())
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/tests/__init__.py", line 34, in basicSuite
    suite.addTest(wikisyntax.suite())
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/tests/wikisyntax.py", line 192, in suite
    suite.addTest(formatter.suite(SEARCH_TEST_CASES, file=__file__))
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/wiki/tests/formatter.py", line 303, in suite
    add_test_cases(data, file)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/wiki/tests/formatter.py", line 286, in add_test_cases
    teardown, context)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/wiki/tests/formatter.py", line 164, in __init__
    self.env = EnvironmentStub(enable=['trac.*'] + all_test_components)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 319, in __init__
    self.reset_db(default_data)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 337, in reset_db
    with self.db_transaction as db:
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 147, in __enter__
    db = self.dbmgr.get_connection()
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 262, in get_connection
    db = self._cnx_pool.get_cnx(self.timeout or None)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 213, in get_cnx
    return _backend.get_cnx(self._connector, self._kwargs, timeout)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 134, in get_cnx
    raise TimeoutError(errmsg)
trac.db.pool.TimeoutError: Unable to get database connection within 0 seconds. (TracError: Database "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.db" not found.)
make: *** [unit-test] Error 1

If test.db does exist:

$ make db=sqlite unit-testPython version: Python 2.7.4
figleaf: 
coverage: 
PYTHONPATH=.:
TRAC_TEST_DB_URI=sqlite:test.db
server-options= -p 8000 -a '*,~/tracenvs/htdigest.realm,realm' -r -e ~/tracenvs
python setup.py egg_info
running egg_info
unrecognized .svn/entries format; skipping .
writing requirements to Trac.egg-info/requires.txt
writing Trac.egg-info/PKG-INFO
writing top-level names to Trac.egg-info/top_level.txt
writing dependency_links to Trac.egg-info/dependency_links.txt
writing entry points to Trac.egg-info/entry_points.txt
unrecognized .svn/entries format in /home/user/Workspace/t11584/teo-rjollos.git
reading manifest file 'Trac.egg-info/SOURCES.txt'
writing manifest file 'Trac.egg-info/SOURCES.txt'
python ./trac/test.py --skip-functional-tests 
..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................EEE....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
======================================================================
ERROR: test_empty_shared_htdocs_dir_raises_file_not_found (trac.web.tests.chrome.ChromeTestCase2)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/web/tests/chrome.py", line 352, in setUp
    self.env = EnvironmentStub(path=tempfile.mkdtemp())
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 319, in __init__
    self.reset_db(default_data)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 337, in reset_db
    with self.db_transaction as db:
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 147, in __enter__
    db = self.dbmgr.get_connection()
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 262, in get_connection
    db = self._cnx_pool.get_cnx(self.timeout or None)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 213, in get_cnx
    return _backend.get_cnx(self._connector, self._kwargs, timeout)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 134, in get_cnx
    raise TimeoutError(errmsg)
TimeoutError: Unable to get database connection within 0 seconds. (TracError: Database "/tmp/tmpCaxBih/test.db" not found.)

======================================================================
ERROR: test_malicious_filename_raises (trac.web.tests.chrome.ChromeTestCase2)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/web/tests/chrome.py", line 352, in setUp
    self.env = EnvironmentStub(path=tempfile.mkdtemp())
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 319, in __init__
    self.reset_db(default_data)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 337, in reset_db
    with self.db_transaction as db:
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 147, in __enter__
    db = self.dbmgr.get_connection()
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 262, in get_connection
    db = self._cnx_pool.get_cnx(self.timeout or None)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 213, in get_cnx
    return _backend.get_cnx(self._connector, self._kwargs, timeout)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 134, in get_cnx
    raise TimeoutError(errmsg)
TimeoutError: Unable to get database connection within 0 seconds. (TracError: Database "/tmp/tmp75oYjJ/test.db" not found.)

======================================================================
ERROR: test_shared_htdocs_dir_file_is_found (trac.web.tests.chrome.ChromeTestCase2)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/web/tests/chrome.py", line 352, in setUp
    self.env = EnvironmentStub(path=tempfile.mkdtemp())
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/core.py", line 124, in __call__
    self.__init__(*args, **kwargs)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 319, in __init__
    self.reset_db(default_data)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/test.py", line 337, in reset_db
    with self.db_transaction as db:
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 147, in __enter__
    db = self.dbmgr.get_connection()
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/api.py", line 262, in get_connection
    db = self._cnx_pool.get_cnx(self.timeout or None)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 213, in get_cnx
    return _backend.get_cnx(self._connector, self._kwargs, timeout)
  File "/home/user/Workspace/t11584/teo-rjollos.git/trac/db/pool.py", line 134, in get_cnx
    raise TimeoutError(errmsg)
TimeoutError: Unable to get database connection within 0 seconds. (TracError: Database "/tmp/tmpbfHoUA/test.db" not found.)

----------------------------------------------------------------------
Ran 1545 tests in 76.798s

FAILED (errors=3)
make: *** [unit-test] Error 1

Shall we fix it by moving the context manager inside the try block?:

trac/test.py

diff --git a/trac/test.py b/trac/test.py
index 327f9d3..337249e 100755

                class EnvironmentStub(Environment):
         scheme, db_prop = _parse_db_str(self.dburi)
         tables = []
         remove_sqlite_db = False
         with self.db_transaction as db:
             try:
+        try:
+            with self.db_transaction as db:
                 db.rollback()  # make sure there's no transaction in progress
                 # check the database version
                 database_version = self.get_version()
+            except Exception:
+                # "Database not found ...",
+                # "OperationalError: no such table: system" or the like
+                pass
+        except Exception:
+            # "Database not found ...",
+            # "OperationalError: no such table: system" or the like
+            pass
+        else:
+            if database_version == db_default.db_version:
+                # same version, simply clear the tables (faster)
+                m = sys.modules[__name__]
+                reset_fn = 'reset_%s_db' % scheme
+                if hasattr(m, reset_fn):
+                    tables = getattr(m, reset_fn)(self, db_prop)
             else:
+                if database_version == db_default.db_version:
+                    # same version, simply clear the tables (faster)
+                    m = sys.modules[__name__]
+                    reset_fn = 'reset_%s_db' % scheme
+                    if hasattr(m, reset_fn):
+                        tables = getattr(m, reset_fn)(self, db_prop)
+                else:
+                    # different version or version unknown, drop the tables
+                    remove_sqlite_db = True
+                    self.destroy_db(scheme, db_prop)
+                # different version or version unknown, drop the tables
+                remove_sqlite_db = True
+                self.destroy_db(scheme, db_prop)
         db = None  # as we might shutdown the pool    FIXME no longer needed!

in reply to: 18 ; follow-up: 20 comment:19 by Jun Omae, 11 years ago

Jun, could you apply [345479f3/rjollos.git] onto 0.12-stable? I'm afraid I might break the changeset given the manual edits needed to rebase it onto 0.12-stable.

Okay. I'll apply onto 0.12-stable.

There is a problem after [12682]. Running the tests with a real SQLite db results in the following if test.db doesn't exist:

Ah, I have not run tests with physical SQLite file. I verify it from next time.

Shall we fix it by moving the context manager inside the try block?: […]

Looks good to me and verified. Please apply.

in reply to: 19 ; follow-up: 21 comment:20 by Jun Omae, 11 years ago

Cc:	Jun Omae added

There is a problem after [12682]. Running the tests with a real SQLite db results in the following if test.db doesn't exist:

Ah, I have not run tests with physical SQLite file. I verify it from next time.

BTW, unit tests using SQLite database file on ext4 is very slow….

Python version: Python 2.7.3
figleaf:
coverage:
PYTHONPATH=.:
TRAC_TEST_DB_URI=sqlite:test.db
server-options= -p 8000  -r -e
python setup.py egg_info
.....
Ran 1545 tests in 800.592s

OK

It would make the tests faster to apply this or use database file on tmpfs. After the changes, I get Ran 1545 tests in 23.230s on the tests.

trac/test.py

                class EnvironmentStub(Environment):
                 self.global_databasemanager.shutdown()
         with self.db_transaction as db:
+            if scheme == 'sqlite':
+                db("PRAGMA synchronous = OFF")
             if default_data:
                 for table, cols, vals in db_default.get_data(db):
                     db.executemany("INSERT INTO %s (%s) VALUES (%s)"

in reply to: 20 comment:21 by Ryan J Ollos, 11 years ago

Replying to jomae:

BTW, unit tests using SQLite database file on ext4 is very slow….

I had experienced the slowness yesterday. This was before I realized that db=sqlite results in the tests running with a real SQLite db. At the time I assumed it was in-memory DB. It is nice to have an explanation for the behavior. I'll test your patch on my platform and report the results back.

Change from comment:18 committed in [12685], merged in [12686].

comment:22 by Ryan J Ollos, 11 years ago

I created #11592 to discuss improving speed of the unit-tests with SQLite.

comment:23 by Ryan J Ollos, 11 years ago

Proposed changes for 1.0-stable (to be merged to trunk): log:rjollos.git:t11512.5. Proposed changes for 0.12-stable: log:rjollos.git:t11512.5_0.12. I haven't tested with Postgres 8.0 or 8.1.

follow-up: 27 comment:24 by Jun Omae, 11 years ago

Sorry for late. I tested rjollos.git@t11512.5_0.12 with PostgreSQL 8.1.23 and Python 2.4. It seems to need to apply like this.

trac/db/postgres_backend.py

diff --git a/trac/db/postgres_backend.py b/trac/db/postgres_backend.py
index ad43a0c..a21e7e2 100644

                from trac.config import Option
 from trac.db.api import IDatabaseConnector, _parse_db_str
 from trac.db.util import ConnectionWrapper, IterableCursor
 from trac.util import get_pkginfo
 from trac.util.compat import close_fds
+from trac.util.compat import any, close_fds
 from trac.util.text import empty, exception_to_unicode, to_unicode
 from trac.util.translation import _
-…
+               class PostgreSQLConnection(ConnectionWrapper):
         return IterableCursor(self.cnx.cursor(), self.log)
     def drop_table(self, table):
+        if (self._version or '').startswith(('8.0.', '8.1.')):
+            cursor = self.cursor()
+        cursor = self.cursor()
+        if self._version and any(self._version.startswith(version)
+                                 for version in ('8.0.', '8.1.')):
             cursor.execute("""SELECT table_name FROM information_schema.tables
                               WHERE table_schema=current_schema()
                               AND table_name=%s""", (table,))
             for row in cursor:
                 if row[0] == table:
                     self.cursor().execute("DROP TABLE " + self.quote(table))
+                    cursor.execute("DROP TABLE " + self.quote(table))
                     break
         else:
+            self.cursor().execute("DROP TABLE IF EXISTS " + self.quote(table))
+        self.commit()
+            cursor.execute("DROP TABLE IF EXISTS " + self.quote(table))
     def _sequence_name(self, table, column):
         return '%s_%s_seq' % (table, column)

Edit: updated the above patch. Use cursor variable.

I don't think that drop_table() should call self.commit(). Caller of the function should use @with_transaction().

Also, transaction in PostgresSQL can contain DDL, e.g. CREATE TABLE, DROP TABLE and can rollback. However, transaction in MySQL ignores DDL and cannot rollback. See Transactional DDL in PostgreSQL: A Competitive Analysis - PostgreSQL wiki.

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

comment:25 by Jun Omae, 11 years ago

Backported [345479f3/rjollos.git] onto 0.12-stable, see jomae.git@t11512.5_0.12 included patch in comment:24.

comment:26 by Ryan J Ollos, 11 years ago

API Changes:	modified (diff)
Status:	new → assigned

Changes committed to 1.0-stable in [12701:12704] and merged to trunk in [12705:12708]. I'll post proposed 0.12-stable changes for review soon.

in reply to: 24 ; follow-up: 28 comment:27 by Ryan J Ollos, 11 years ago

Replying to jomae:

I don't think that drop_table() should call self.commit(). Caller of the function should use @with_transaction().

I attempted to address the issues: log:rjollos.git:t11512.6_0.12.

in reply to: 27 comment:28 by Jun Omae, 11 years ago

Replying to rjollos:

Replying to jomae:

I don't think that drop_table() should call self.commit(). Caller of the function should use @with_transaction().

I attempted to address the issues: log:rjollos.git:t11512.6_0.12.

Verified. Unit tests pass on SQLite 3.3.6, PostgreSQL 8.1.23 and MySQL 5.0.95 with Python 2.4. But [e5f97fa8/rjollos.git#file2] should be the following.

trac/test.py

diff --git a/trac/test.py b/trac/test.py
index c7a10fc..6d56ba7 100755

                class EnvironmentStub(Environment):
         if self.dburi.startswith('sqlite'):
             self.config.set('trac', 'database', 'sqlite::memory:')
             self.db = InMemoryDatabase()
+        self.config.set('trac', 'database', self.dburi)
+        else:
+            self.config.set('trac', 'database', self.dburi)
         if default_data:
             self.reset_db(default_data)

comment:29 by Ryan J Ollos, 11 years ago

Resolution:	→ fixed
Status:	assigned → closed

I pulled in the change from comment:28 and committed to 0.12-stable in [12724:12726], followed by a record-only merge in [12727:12728]. Thanks!

comment:30 by Ryan J Ollos, 11 years ago

Owner:	changed from Ryan J Ollos to Jun Omae

This ticket turned out to be much more work than expected. Thanks for all the help Jun!

comment:31 by Jun Omae, 11 years ago

I just realize we could make "reset sequences" procedure simple using quote_ident function. The function is available between PostgreSQL 8.0 and 9.3.

postgres=# SELECT quote_ident('HOUR');
 quote_ident
-------------
 "HOUR"
(1 row)

The changes in jomae.git@t11512.7_0.12 and jomae.git@t11512.7.

comment:32 by Ryan J Ollos, 11 years ago

Yeah that is much easier to read. Test pass for me on both branches with Postgres 9.1.11.

comment:33 by Jun Omae, 11 years ago

Thanks for testing. I've also tested with TRAC_TEST_DB_URI=postgres://...?schema=Trac-Test on PostgreSQL 8.1.23.

Committed in [12735] and merged in [12736] and [12737].

Modify Ticket

Change Properties

Summary:
Description:	To support the case that the primary key column has a name other than `id`, `update_sequence` for the Postgres backend should be modified to use the specified column name when selecting the `MAX` value: [browser:/branches/1.0-stable/trac/db/postgres_backend.py@12500:258#L255]. The patch was proposed by Olemis Lang in bh:#708. You may use WikiFormatting here.
Type:		Priority:
Milestone:		Component:
Version:		Severity:
Keywords:		Cc:	Set your email in Preferences
Branch:
Release Notes:
API Changes:	* Added `drop_table` method to the database connection classes. * Added `create_tables` and `drop_tables` methods to the `DatabaseManager` class (1.0.2). * `update_sequence` method of the connection classes uses the column name to select the max value. * `update_sequence` and `get_last_id` methods of the connection classes and `trac.test.reset_postgres_db` quote the table and column names, fixing problems with upper case characters.
Internal Changes:

Action

leave as closed The owner will remain Jun Omae.

reopen The resolution will be deleted. Next status will be 'reopened'.

change ownership to The owner will be changed from Jun Omae to the specified user.

Add Comment

Your email or username:

E-mail address and name can be saved in the Preferences .

You may use WikiFormatting here.

Attachments ↑ Description ↑

Note: See TracTickets for help on using tickets.

Download in other formats:

Context Navigation

#11512 closed enhancement (fixed)

update_sequence should use the column name when selecting the max value

Description

Attachments (0)

Change History (33)

comment:1 by Ryan J Ollos, 11 years ago

comment:2 by Peter Suter, 11 years ago

comment:3 by Ryan J Ollos, 11 years ago

comment:4 by Jun Omae, 11 years ago

trac/db/postgres_backend.py

comment:5 by Ryan J Ollos, 11 years ago

comment:6 by Jun Omae, 11 years ago

comment:7 by Ryan J Ollos, 11 years ago

comment:8 by Jun Omae, 11 years ago

follow-up: 11 comment:9 by Ryan J Ollos, 11 years ago

comment:10 by Ryan J Ollos, 11 years ago

in reply to: 9 comment:11 by Jun Omae, 11 years ago

trac/db/tests/api.py

comment:12 by Ryan J Ollos, 11 years ago

comment:13 by Jun Omae, 11 years ago

comment:14 by Ryan J Ollos, 11 years ago

comment:15 by Jun Omae, 11 years ago

comment:16 by Ryan J Ollos, 11 years ago

comment:17 by Ryan J Ollos, 11 years ago

follow-up: 19 comment:18 by Ryan J Ollos, 11 years ago

trac/test.py

in reply to: 18 ; follow-up: 20 comment:19 by Jun Omae, 11 years ago

in reply to: 19 ; follow-up: 21 comment:20 by Jun Omae, 11 years ago

trac/test.py

in reply to: 20 comment:21 by Ryan J Ollos, 11 years ago

comment:22 by Ryan J Ollos, 11 years ago

comment:23 by Ryan J Ollos, 11 years ago

follow-up: 27 comment:24 by Jun Omae, 11 years ago

trac/db/postgres_backend.py

comment:25 by Jun Omae, 11 years ago

comment:26 by Ryan J Ollos, 11 years ago

in reply to: 24 ; follow-up: 28 comment:27 by Ryan J Ollos, 11 years ago

in reply to: 27 comment:28 by Jun Omae, 11 years ago

trac/test.py

comment:29 by Ryan J Ollos, 11 years ago

comment:30 by Ryan J Ollos, 11 years ago

comment:31 by Jun Omae, 11 years ago

comment:32 by Ryan J Ollos, 11 years ago

comment:33 by Jun Omae, 11 years ago

Modify Ticket

Add Comment

by anonymous

Download in other formats: