#11461 closed defect (fixed)
MariaDB errors initializing SMP
Reported by: | johna | Owned by: | falkb |
---|---|---|---|
Priority: | normal | Component: | SimpleMultiProjectPlugin |
Severity: | major | Keywords: | |
Cc: | Trac Release: | 1.0 |
Description (last modified by )
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.
Attachments (1)
Change History (30)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
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
comment:3 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:4 Changed 11 years ago by
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?
comment:5 Changed 11 years ago by
Status: | new → accepted |
---|
comment:6 Changed 11 years ago by
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.
Changed 11 years ago by
Attachment: | t11461.patch added |
---|
comment:7 follow-up: 9 Changed 11 years ago by
Description: | modified (diff) |
---|
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.
comment:8 Changed 11 years ago by
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.
comment:9 follow-up: 10 Changed 11 years ago by
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.
comment:10 follow-up: 11 Changed 11 years ago by
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?
comment:11 Changed 11 years ago by
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
comment:12 Changed 11 years ago by
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.
comment:13 Changed 11 years ago by
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()
comment:14 Changed 11 years ago by
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.
comment:15 Changed 11 years ago by
Those comments were by me, btw. I didn't mean to use my "test" account.
comment:16 Changed 11 years ago by
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.
comment:17 Changed 11 years ago by
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.
comment:18 Changed 11 years ago by
In that case, it would probably be easiest to just remove the __get_db
, __get_cursor
and __start_transaction
methods.
comment:19 Changed 11 years ago by
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...
comment:20 Changed 11 years ago by
Editing my comment:19
- s/Event/even
- plus
from trac.db import with_transaction
in the first snippet - this is
__start_transaction()
:# DB Methods def __start_transaction(self, db): if VERSION < '0.12': # deprecated in newer versions db.commit() db.close()
comment:22 Changed 11 years ago by
Keywords: | testing added |
---|
finally I get it how to use the db API (hopefully properly now), so please test
comment:23 Changed 11 years ago by
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]).
comment:24 follow-up: 25 Changed 11 years ago by
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?
comment:25 follow-up: 26 Changed 11 years ago by
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.
comment:26 Changed 11 years ago by
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.
comment:27 Changed 11 years ago by
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.
comment:28 Changed 11 years ago by
Keywords: | testing removed |
---|---|
Resolution: | → fixed |
Status: | accepted → closed |
After related fixes in #11484 it works now properly.
Apologies.
SMP version 0.0.4dev-py2.6