Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#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, 10 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, 10 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 10 years ago by Peter Suter (previous) (diff)

comment:3 by Ryan J Ollos, 10 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, 10 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
    a b class PostgreSQLConnection(ConnectionWrapper):  
    250250        return '"%s"' % identifier.replace('"', '""')
    251251
    252252    def get_last_id(self, cursor, table, column='id'):
    253         cursor.execute("""SELECT CURRVAL('"%s_%s_seq"')""" % (table, column))
     253        cursor.execute("SELECT CURRVAL(%s)",
     254                       (self.quote(self._sequence_name(table, column)),))
    254255        return cursor.fetchone()[0]
    255256
    256257    def update_sequence(self, cursor, table, column='id'):
    257         cursor.execute("""
    258             SELECT setval('"%s_%s_seq"', (SELECT MAX(id) FROM %s))
    259             """ % (table, column, table))
     258        quote = self.quote
     259        cursor.execute("SELECT SETVAL(%%s, (SELECT MAX(%s) FROM %s))"
     260                       % (quote(column), quote(table)),
     261                       (quote(self._sequence_name(table, column)),))
    260262
    261263    def cursor(self):
    262264        return IterableCursor(self.cnx.cursor(), self.log)
     265
     266    def _sequence_name(self, table, column):
     267        return '%s_%s_seq' % (table, column)

comment:5 by Ryan J Ollos, 10 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, 10 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, 10 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 10 years ago by Ryan J Ollos (previous) (diff)

comment:8 by Jun Omae, 10 years ago

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

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

Milestone: 0.12.6

in reply to:  9 comment:11 by Jun Omae, 10 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
    a b class ConnectionTestCase(unittest.TestCase):  
    362362                    db(sql)
    363363
    364364    def tearDown(self):
    365         self._cleanup_tables()
     365        self.env.destroy_db()
    366366        self.env.reset_db()
    367367
    368368    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, 10 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, 10 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, 10 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 10 years ago by Ryan J Ollos (previous) (diff)

comment:15 by Jun Omae, 10 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, 10 years ago

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

comment:17 by Ryan J Ollos, 10 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.

Version 0, edited 10 years ago by Ryan J Ollos (next)

comment:18 by Ryan J Ollos, 10 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
    a b class EnvironmentStub(Environment):  
    334334        scheme, db_prop = _parse_db_str(self.dburi)
    335335        tables = []
    336336        remove_sqlite_db = False
    337         with self.db_transaction as db:
    338             try:
     337        try:
     338            with self.db_transaction as db:
    339339                db.rollback()  # make sure there's no transaction in progress
    340340                # check the database version
    341341                database_version = self.get_version()
    342             except Exception:
    343                 # "Database not found ...",
    344                 # "OperationalError: no such table: system" or the like
    345                 pass
     342        except Exception:
     343            # "Database not found ...",
     344            # "OperationalError: no such table: system" or the like
     345            pass
     346        else:
     347            if database_version == db_default.db_version:
     348                # same version, simply clear the tables (faster)
     349                m = sys.modules[__name__]
     350                reset_fn = 'reset_%s_db' % scheme
     351                if hasattr(m, reset_fn):
     352                    tables = getattr(m, reset_fn)(self, db_prop)
    346353            else:
    347                 if database_version == db_default.db_version:
    348                     # same version, simply clear the tables (faster)
    349                     m = sys.modules[__name__]
    350                     reset_fn = 'reset_%s_db' % scheme
    351                     if hasattr(m, reset_fn):
    352                         tables = getattr(m, reset_fn)(self, db_prop)
    353                 else:
    354                     # different version or version unknown, drop the tables
    355                     remove_sqlite_db = True
    356                     self.destroy_db(scheme, db_prop)
     354                # different version or version unknown, drop the tables
     355                remove_sqlite_db = True
     356                self.destroy_db(scheme, db_prop)
    357357
    358358        db = None  # as we might shutdown the pool    FIXME no longer needed!

in reply to:  18 ; comment:19 by Jun Omae, 10 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 ; comment:20 by Jun Omae, 10 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

    a b class EnvironmentStub(Environment):  
    373373                self.global_databasemanager.shutdown()
    374374
    375375        with self.db_transaction as db:
     376            if scheme == 'sqlite':
     377                db("PRAGMA synchronous = OFF")
    376378            if default_data:
    377379                for table, cols, vals in db_default.get_data(db):
    378380                    db.executemany("INSERT INTO %s (%s) VALUES (%s)"

in reply to:  20 comment:21 by Ryan J Ollos, 10 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, 10 years ago

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

comment:23 by Ryan J Ollos, 10 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.

comment:24 by Jun Omae, 10 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
    a b from trac.config import Option  
    2323from trac.db.api import IDatabaseConnector, _parse_db_str
    2424from trac.db.util import ConnectionWrapper, IterableCursor
    2525from trac.util import get_pkginfo
    26 from trac.util.compat import close_fds
     26from trac.util.compat import any, close_fds
    2727from trac.util.text import empty, exception_to_unicode, to_unicode
    2828from trac.util.translation import _
    2929
    class PostgreSQLConnection(ConnectionWrapper):  
    261261        return IterableCursor(self.cnx.cursor(), self.log)
    262262
    263263    def drop_table(self, table):
    264         if (self._version or '').startswith(('8.0.', '8.1.')):
    265             cursor = self.cursor()
     264        cursor = self.cursor()
     265        if self._version and any(self._version.startswith(version)
     266                                 for version in ('8.0.', '8.1.')):
    266267            cursor.execute("""SELECT table_name FROM information_schema.tables
    267268                              WHERE table_schema=current_schema()
    268269                              AND table_name=%s""", (table,))
    269270            for row in cursor:
    270271                if row[0] == table:
    271                     self.cursor().execute("DROP TABLE " + self.quote(table))
     272                    cursor.execute("DROP TABLE " + self.quote(table))
    272273                    break
    273274        else:
    274             self.cursor().execute("DROP TABLE IF EXISTS " + self.quote(table))
    275         self.commit()
     275            cursor.execute("DROP TABLE IF EXISTS " + self.quote(table))
    276276
    277277    def _sequence_name(self, table, column):
    278278        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 10 years ago by Jun Omae (previous) (diff)

comment:25 by Jun Omae, 10 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, 10 years ago

API Changes: modified (diff)
Status: newassigned

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 ; comment:27 by Ryan J Ollos, 10 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, 10 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
    a b class EnvironmentStub(Environment):  
    291291        if self.dburi.startswith('sqlite'):
    292292            self.config.set('trac', 'database', 'sqlite::memory:')
    293293            self.db = InMemoryDatabase()
    294         self.config.set('trac', 'database', self.dburi)
     294        else:
     295            self.config.set('trac', 'database', self.dburi)
    295296
    296297        if default_data:
    297298            self.reset_db(default_data)

comment:29 by Ryan J Ollos, 10 years ago

Resolution: fixed
Status: assignedclosed

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, 10 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, 10 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, 10 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, 10 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
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


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