MariaDB errors initializing SMP

[johna]

Trac 	1.0.1
Genshi 	0.7 (without speedups)
MySQL 	server: "5.5.32-MariaDB", client: "5.1.54", thread-safe: 1
MySQLdb 	1.2.3c1
Pygments 	1.6
Python 	2.6.6 (r266:84292, Nov 22 2013, 12:16:22) [GCC 4.4.7 20120313 (Red Hat 4.4.7-4)]
pytz 	2013.8
setuptools 	0.6
Subversion 	1.7.4 (r1295709)
jQuery	1.7.2

After installation of the plugin and restart of the server, the database update fails. The culprit appears to be in line 154 of environmentSetup.py:

cursor.execute("""ALTER TABLE smp_project ADD restrict TEXT""")

This change appears to fix this issue. (adding backtiks to "restrict")

cursor.execute("""ALTER TABLE smp_project ADD `restrict` TEXT""")

Created a new environment and ran update. It indicated success, but none of the smp_ tables in the database have data.

[root@fw-svn01 Testing]# trac-admin /home/trac/Testing upgrade
Upgrading SimpleMultiProject database schema
SimpleMultiProject database schema version is 0, should be 5
Upgrade done.

You may want to upgrade the Trac documentation now by running:

  trac-admin /home/trac/Testing wiki upgrade

Additonally, there is the following error in the console.

ProgrammingError: (1064, "You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'restrict\n                   FROM\n                        smp_project' at line 2")

This shows up in: Manage Projects -> Projects and the "Timeline", "Roadmap", "View Tickets", and "New Ticket" items.

I've not been able to test this against MySQL, but it may be that "restrict" is a restricted word for MariaDB.

[johna]

Apologies.

SMP version 0.0.4dev-py2.6

[johna]

I confirmed that the issue existed in MySQL 5.5.33

The issue appears to be the use of restrict as a column name in the smp_project table.

I renamed all instances of restrict to restrick in the following files:

admin.py
environmentSetup.py
model.py

Redid the whole thing and it appears to be working properly. Evidently restrict is a restricted word in mysql and MariaDB

[falkb]

Oh what a silly bug! It seems to me there's no other way than to rename it in an incompatible way for db schema version 5 of the plugin, and to force people, who upgraded already, to rename it manually. What do you think?

[johna]

Yeah. It is a problematic one to find this far down the roadmap.

Reserved words can be utilized as object names, but it gets kind of ugly. ​http://dev.mysql.com/doc/refman/5.5/en/identifiers.html

I tried enclosing the words in backticks, but python didn't like that at all and wouldn't compile the .egg

As a straight .sql file it ran just fine against the database with the reserved word(s) in backticks. You might consider packaging a .sql file for the next version and run that to change the row as needed.

Not pretty, but it might get the job done.

[Ryan J Ollos]

t11461.patch​ has been tested with SQLite and MySQL, and I believe it will fix the issue in a cross-db compatible way. Someone more familiar with the project should probably do some thorough testing before committing the change.

[Ryan J Ollos]

One other suggestion - a common approach is to use IEnvironmentSetupParticipant to insert entries in the ticket-custom section. This can avoid a manual step in the installation process. It would be similar to ​here. TimingAndEstimationPlugin does this as well: timingandestimationplugin/branches/trac0.12/timingandestimationplugin/api.py@13377#L140​.

[falkb]

Replying to rjollos:

t11461.patch​ has been tested with SQLite and MySQL, and I believe it will fix the issue in a cross-db compatible way. Someone more familiar with the project should probably do some thorough testing before committing the change.

Thanks a lot, rjollos! I've tried this patch but adding new projects via insert_project() in model.py does not work anymore then. Simply, it doesn't add a new record to my SQLite db. I played around with the parameter formatting for a while but haven't found a solution yet.

[Ryan J Ollos]

Replying to falkb:

Thanks a lot, rjollos! I've tried this patch but adding new projects via insert_project() in model.py does not work anymore then.

I had tested that behavior and it was working okay for me when adding new projects through the admin page. Which database did you test with? What result did you get?

[falkb]

Replying to rjollos:

Replying to falkb:

Thanks a lot, rjollos! I've tried this patch but adding new projects via insert_project() in model.py does not work anymore then.

I had tested that behavior and it was working okay for me when adding new projects through the admin page. Which database did you test with? What result did you get?

SQLite. Note from IRC:

hmm.... Now I saw it started to work again as I removed one of my test projects (through Admin page)
falkb	after that I was able to add new projects again
falkb	I start to think it has something to do with the db cursor
falkb	maybe sometimes it's valid and sometimes not
falkb	maybe there's an issue with the __get_db() or __get_cursor() function
falkb	and the scope of the return value of both functions
falkb	maybe it's luck whether the returned object (db or cursor) is still valid or not
falkb	but that's just a guess

[falkb]

This behaviour is reproducable through Admin page. After login to Trac I always fail to add new projects. It works again as soon as I have removed a project.

[Ryan Ollos (test account)]

Which version of Trac did you test with? It looks like there is an issue with Trac < 0.12. Since I've added __get_db, then db is no longer bound to self in insert_project. So we would either need to assign db to self.db in insert_project, or pass the db object to __start_transaction:

-    def __start_transacction(self):
+    def __start_transacction(self, db):
+       db = db or self.db
        if VERSION < '0.12':
            # deprecated in newer versions
            db.commit()
            db.close()

[Ryan Ollos (test account)]

I think maybe we should drop the __get_cursor method and modify __start_transaction to accept a db object. It is really easy to get into trouble by not keeping the db object in scope while trying to use a cursor object (e.g. see #11115 and cannot operate on closed cursor). You may be okay by keeping it bound to the instance object, but I'm not sure of that.

[Ryan J Ollos]

Those comments were by me, btw. I didn't mean to use my "test" account.

[Ryan J Ollos]

I was also just reminded in #11234 that db.quote is only available since Trac 0.12: ​[trac 9062]. I suppose you could backport the methods to your plugin though if you want support for Trac < 0.12.

[falkb]

I only support 0.12 and 1.0 at present. I have no environment for backporting to 0.11, and 0.11 was long ago, so I don't think it's a good idea anyway.

[Ryan J Ollos]

In that case, it would probably be easiest to just remove the __get_db, __get_cursor and __start_transaction methods.

[anonymous]

I spent the whole evening for figuring out how that db access must be programmed, but I always failed. Event reading ​http://trac.edgewall.org/wiki/TracDev/DatabaseApi thousand times did not help. Please, give me a code example as a start.

I tried for instance in model.py:

from trac.perm import PermissionSystem, IPermissionGroupProvider
...
class SmpModel(Component):
...
    def insert_project(self, name, summary, description, closed, restrict):
        @with_transaction(env)
        def do_something(db):
            query    = """INSERT INTO
                            smp_project (name, summary, description, closed, %s)
                          VALUES (%%s, %%s, %%s, %%s, %%s);""" % db.quote('restrict')
    
            cursor = db.cursor()
            cursor.execute(query, [name, summary, description, closed, restrict])
            self.__start_transaction(db)

or

    # AdminPanel Methods
    def insert_project(self, name, summary, description, closed, restrict):
        db = None
        if VERSION < '0.12':
            db = self.env.get_db_cnx()
        else:
            db = self.env.get_read_db()
        query    = """INSERT INTO
                        smp_project (name, summary, description, closed, %s)
                      VALUES (%%s, %%s, %%s, %%s, %%s);""" % db.quote('restrict')

        cursor = db.cursor()
        cursor.execute(query, [name, summary, description, closed, restrict])
        self.__start_transaction(db)

The effect is always like described above...

[falkb]

Editing my comment:19

1. s/Event/even
2. plus from trac.db import with_transaction in the first snippet
3. this is __start_transaction() :

    # DB Methods
    def __start_transaction(self, db):
        db = None
        if VERSION < '0.12':
            # deprecated in newer versions
            db.commit()
            db.close()
   

[falkb]

In 13549:

bugfix (see #11461):

• fixes model.py concerning all DB access calls (proper use of cursor and scope of db object)
• applied a patch of rjollos to fix the issue with quoting the 'restrict' SQL keyword to get it work with MariaDB

[falkb]

finally I get it how to use the db API (hopefully properly now), so please test

[Ryan J Ollos]

Looks good. Since db.quote is not available until Trac 0.12, unless you backport it in your plugin then you've already broken backward compatibility with Trac < 0.12, and you could therefore just drop all of the conditionals if VERSION < '0.12' and __start_transaction.

Another approach, in order to keep it working with at least SQLite (and maybe PostgreSQL) for Trac < 0.12, would be as shown by jun66j5 in comment:6:ticket:11234:

if hasattr(db, 'quote'): 
    quote = db.quote  # Trac 0.12+ 
else: 
    quote = lambda name: name

This adds a dummy quote function for Trac < 0.12. If you are ambituous about making it would work Trac < 0.12, you could add a real quote function for each database type (i.e. backport ​[trac 9062]).

[falkb]

Hey rjollos, thanks for your review and your useful hints and links! My plan is a backport to 0.11 some day, that's why I keep as much if VERSION <'0.12' as I can overview. I must search a bit, but somewhere I've got a Bitnami package of 0.11 around, which I'm gonna install as test bench...

P.S.: Uhm... wouldn't it a good idea to add a state 'testing' to the trac-hacks.org ticket workflow?

[Ryan J Ollos]

Replying to falkb:

P.S.: Uhm... wouldn't it a good idea to add a state 'testing' to the trac-hacks.org ticket workflow?

We've talked about similar changes. Others have mentioned having a Review step in the workflow.

Adding multiple workflows to Trac is high on the list of features that I'd like to add to Trac for 1.2, and that might allow us to customize the workflow for each Component. Although, the implementation might couple the workflow to ticket types. We'd have to see how flexible it could be made without becoming overly complex.

[falkb]

Replying to rjollos:

Replying to falkb:

P.S.: Uhm... wouldn't it a good idea to add a state 'testing' to the trac-hacks.org ticket workflow?

Adding multiple workflows to Trac is high on the list of features that I'd like to add to Trac for 1.2, and that might allow us to customize the workflow for each Component.

I actually thought of just installing AdvancedTicketWorkflowPlugin and define 'testing' for the whole trac-hacks.org which may be a good idea in general.

[Ryan J Ollos]

Adding another plugin to the site requires a lot of testing, and then we take on the burden of maintaining the plugin if it doesn't already have a maintainer. You'll see from report:9?COMPONENT=TracHacks, report:9?COMPONENT=TracHacksPlugin that there is already quite a bit of work planned for the site, and additionally we'd like to get VotePlugin installed, AccountManagerPlugin 0.5 released and installed, ... and the list goes on and on. Ultimately, these things just need to be prioritized, and so far the multiple-workflow doesn't see as much as an improvement to the site as several other things.

[falkb]

After related fixes in #11484 it works now properly.

[falkb]

In 13735:

• fixed #11553: timestamps need to be bigint with PostgreSQL
• get rid of table 'smp_project' column 'restrict' because that is a db keyword and therefore not suitable as column name, renamed it to 'restrict_to' (see #11461)
• => plugin db schema version increased to 6