The Mollom 7.x-2.0 module uses case-sensitive column names (such as contentId), where PostgreSQL expects case-insensitive column names. Installing the 2.0 version of the module fails when trying to create the index on the mollom table, and doesn't finish creating the mollom_form table, for example.
The failing query is:
CREATE INDEX "mollom_contentId_idx" ON mollom ("contentId")
The problem is that when the table was created, the contentId column got created in lower-case (because it didn't have double quotes around the column name.
A hackish patch is posted here as an idea (and not as a patch I'd actually suggest, which is why it's in the comment field and not attached as a real patch). It should at least give you an idea of the problem.
--- mollom/mollom.install 2012-04-11 23:51:13.948535733 -0500
+++ mollom/mollom.install.new 2012-04-11 23:50:56.130660827 -0500
@@ -179,8 +179,8 @@
),
),
'indexes' => array(
- 'contentId' => array('contentId'),
- 'captchaId' => array('captchaId'),
+ 'contentId' => array('contentid'),
+ 'captchaId' => array('captchaid'),
),
'primary key' => array('entity', 'id'),
'foreign keys' => array(
Comment | File | Size | Author |
---|---|---|---|
#44 | mollom.postgres.d6.44.patch | 8.88 KB | eshta |
#42 | mollom.postgres.42.patch | 9.24 KB | eshta |
#39 | mollom.postgres.39.patch | 9.07 KB | eshta |
#26 | mollom-postgres_breaks-1529268-25.patch | 58.11 KB | burningdog |
#24 | mollom-postgres_breaks-1529268-24.patch | 21.32 KB | burningdog |
Comments
Comment #1
sun#1171866: Enforced fetching of fields/columns in lowercase breaks third-party integration was very recently fixed for Drupal 7 core and is not contained in any stable core release yet. I wonder whether that might fix this issue?
Could you try the latest Drupal core 7.x-dev on a temporary/testing site?
Comment #2
sunAlso, for later reference, since I just spent some minutes to dig into this:
http://www.postgresql.org/docs/8.0/static/sql-syntax.html#AEN1148
http://codesnippets.joyent.com/posts/show/1701
http://binodsblog.blogspot.de/2011/02/postgresql-is-case-sensitive.html
#56599: Case sensitivity issue with fields/columns in Postgres.
Comment #3
sun@jaredsmith: The Drupal core fix mentioned in #1 is contained in 7.14 now. Could you try whether the module works now?
Comment #4
mikl CreditAttribution: mikl commentedI just tried installing Mollom 2.0 on Drupal 7.14 and PostgreSQL 9.1. Not only does it throw errors, it also breaks the site completely, since it's schema is not installed properly due to the inital error, causing errors like these:
Comment #5
mikl CreditAttribution: mikl commentedThis is the error I get when I try to install Mollom now:
I think the easiest way to solve this is to simply use lower-case for all table and column names.
Comment #6
iSylence CreditAttribution: iSylence commentedDrupal 7.14 does not fix the issue.
Attached patch to fix issue defined by mikl in comment #6. Code changes are the same as those found at the bottom of the issue description on this page. Perhaps not the best for a permanent fix, but it allows for installation of the module.
Comment #8
iSylence CreditAttribution: iSylence commentedSorry about that, fixed up the paths and reattached patch.
Comment #9
tomww CreditAttribution: tomww commentedWith patch fix_issue_with_db_schema-1529268-8.patch I still got errors with the indexes and the table "mollom" not created as the arrays didn't match.
I cleaned up the tables in postgresql (8.4) and enhanced the patch, see below. Please note: This needs to be verified to be sure that changing the array names has no other side effects.
diff -u mollom.install.orig mollom.install
--- mollom.install.orig Thu Jun 7 20:17:58 2012
+++ mollom.install Fri Jun 8 00:19:59 2012
@@ -100,7 +100,7 @@
'not null' => TRUE,
'default' => '',
),
- 'contentId' => array(
+ 'contentid' => array(
'description' => 'Content ID returned by Mollom.',
'type' => 'varchar',
'length' => 128,
@@ -107,7 +107,7 @@
'not null' => TRUE,
'default' => '',
),
- 'captchaId' => array(
+ 'captchaid' => array(
'description' => 'CAPTCHA ID returned by Mollom.',
'type' => 'varchar',
'length' => 128,
@@ -184,8 +184,8 @@
),
),
'indexes' => array(
- 'contentId' => array('contentId'),
- 'captchaId' => array('captchaId'),
+ 'contentId' => array('contentid'),
+ 'captchaId' => array('captchaid'),
),
'primary key' => array('entity', 'id'),
'foreign keys' => array(
Comment #10
sunFiled a bug report against Drupal core: #1622982: PostgreSQL auto-converts column names into lowercase
Comment #11
mikl CreditAttribution: mikl commented@sun: I know having to modify your code to work around compatibility issues like these is annoying, but would you be willing to accept a patch that would change the Mollom module to use lowercase-only column names?
Comment #12
mikl CreditAttribution: mikl commentedEven worse, MySQLs case-sensitivity for database names vary across platforms: http://dev.mysql.com/doc/refman/5.0/en/identifier-case-sensitivity.html
Upper case letters in table/column names: Just say no(tm).
Comment #13
aacraig CreditAttribution: aacraig commented+1 for all lower case names in databases. Avoids these unnecessary and time consuming issues.
Comment #14
hynnot CreditAttribution: hynnot commented+1 #9 solution by tomww
Comment #15
jaredsmith CreditAttribution: jaredsmith commentedSun,
I just did another test as you asked, and the problem still exists (using the 7.x-2.3 version of mollom, and Drupal 7.18).
The solution in comment 9 solved the issue for me.
Comment #16
sunSorry, this is still on my radar.
However, changing the table columns to be lowercase requires significant changes to the module. At this point, I think it's the only viable option, but it will take me some hours to work out a patch.
As others have reported, I don't think that the existing patch is complete, as it only changes the index definitions, but not the actual table column names. Hence, marking needs work.
As I don't think anyone besides me will be able to work out this major change, I could assign this issue to me, but OTOH, I also don't want to hold anyone's horses. ;) I hope to be able to get back to this in the next few weeks.
Comment #17
tomww CreditAttribution: tomww commentedI've updated the patch for version mollom-7.x-2.4-reworked.tar.gz
Looking only at postgres the patch fixes the creation of tables/indexes where upper case isn't allowed for columns names.
For querying postgresql doesn't care on case of columns names (except they are enclosed with quotes, e.g. "columnName"), so no further code changes needed.
--- mollom/mollom.install.orig 2013-01-22 18:01:18.000000000 +0100
+++ mollom/mollom.install 2013-02-03 10:06:04.733925637 +0100
@@ -107,14 +107,14 @@
'not null' => TRUE,
'default' => '',
),
- 'contentId' => array(
+ 'contentid' => array(
'description' => 'Content ID returned by Mollom.',
'type' => 'varchar',
'length' => 128,
'not null' => TRUE,
'default' => '',
),
- 'captchaId' => array(
+ 'captchaid' => array(
'description' => 'CAPTCHA ID returned by Mollom.',
'type' => 'varchar',
'length' => 128,
@@ -191,8 +191,8 @@
),
),
'indexes' => array(
- 'contentId' => array('contentId'),
- 'captchaId' => array('captchaId'),
+ 'contentid' => array('contentid'),
+ 'captchaid' => array('captchaid'),
),
'primary key' => array('entity', 'id'),
'foreign keys' => array(
Comment #18
jneubert CreditAttribution: jneubert commentedThe workarround in #19 solved it for me.
Thanks a lot - Joachim
Comment #19
smscotten CreditAttribution: smscotten commentedIs there any chance that this fix will ever get adopted?
Comment #20
maikeru CreditAttribution: maikeru commentedConfirming that patch at #17 worked for me on Postgres using Mollom 2.7.
Comment #21
drupdan3 CreditAttribution: drupdan3 commentedWhat is the status of this bug? Why is it "needs work"? Are the module authors aware of the problem and the patch?
Comment #22
mikl CreditAttribution: mikl commented#21: I have contacted Mollom (the company), and sun, a couple of times over this, so they should be well aware of the issue.
As far as I can tell, the only reason that this haven't been merged is that sun doesn't want to.
Comment #24
burningdog CreditAttribution: burningdog commentedFollowing on from #16...
Marking critical, because enabling this module immediately breaks a drupal site (if using postgres), and drupal is unable to bootstrap. To get drupal working again, I had to apply the attached patch, enable mollom on a different postres database, export the mollom_cache table definition via drush, then import it on the broken site.
The site breaks because upon enabling mollom, drupal creates the
mollom
table, then tries to create 2 indices on it. The first index fails:PDOException: SQLSTATE[42703]: Undefined column: 7 ERROR: column "contentId" does not exist: CREATE INDEX "mollom_contentId_idx" ON {mollom} ("contentId");
This means that the
mollom_form
table does NOT get created, which results in drupal being unable to bootstrap, filling up watchdog with:PDOException: SQLSTATE[42P01]: Undefined table: 7 ERROR: relation "mollom_form" does not exist LINE 1: SELECT form_id FROM mollom_form ^: SELECT form_id FROM {mollom_form};
Site = nuked.
The attached patch simply converts all mentions of "contentId" and "captchaId" to lowercase. This allows Mollom to install and work normally.
Comment #26
burningdog CreditAttribution: burningdog commentedOh right - all of the tests will fail because they're set to look for captchaId and contentId. Eep. I've now patched the tests too, but I have no idea if the testing server will accept that. Retry.
Comment #28
burningdog CreditAttribution: burningdog commentedHmmm. Looks like the Mollom API wants a case-sensitive value of
captchaid
- here's a failing test:So all integration with the Mollom API is case-sensitive while all database stuff is lowercase. That's a bit, well, messy. Is this still a good direction to go in?
Comment #29
drupdan3 CreditAttribution: drupdan3 commented@sun hasn't been writing since January 2013.
I think Mollom doesn't want PostgreSQL Drupal customers. As infuriating and frustrating as it is, It's time to acknowledge that. Is there another plugin that can handle comment spam?
Comment #30
burningdog CreditAttribution: burningdog commentedSome good news - #1622982: PostgreSQL auto-converts column names into lowercase now passes, which means it can be backported to d7, which automatically will solve this issue. So let's hope we get some feedback over there soon!
Comment #31
acbramley CreditAttribution: acbramley commentedCan we possibly get a patch for the module before the backport?I see #8 works :)Curiously I have an old project running postgres with Mollom 7.x-2.2 and we never had an issue with itComment #32
smscotten CreditAttribution: smscotten commentedSince the Mollom module doesn't have an uninstall feature, it looks like I have to manually remove tables in the database in order to reinstall and patch. Simply patching the install file and enabling the module results in a dead website.
Any suggestions before I go butchering up my database manually? Something I'd rather not do but at this point I'm drowning in spam. I miss having Mollom on the site.
Comment #33
bsisco CreditAttribution: bsisco commentedI just wanted to stop by and say that the patch in #8 worked like a charm for me (Drupal 7.23, PostgreSQL 9.2.4 running on Arch Linux). I did however have to perform the following:
It was as easy as that...once that was done i was able to run the patch on the install file and enable & configure mollom without issues.
just my 2 cents :)
Comment #34
acbramley CreditAttribution: acbramley commented@smscotten the problem actually comes from core I believe with the uninstall issue you are having. If you enable a module and it throws an error for the db table, it will not be removed by uninstalling the module (this is not the same as disabling). If there is an error in the db table you have to manually drop it as per @bsisco's comment.
If a module provides a db table and it doesn't throw an error on installation, you can easily get rid of it with:
Drupal 7 does not require uninstall hooks to remove the db tables.
Comment #35
smscotten CreditAttribution: smscotten commented@bsisco @acbramley thanks. Dropping the table did the job and I was pleasantly surprised to learn that I didn't lose any of my config info (stored in the variable table, I assume.) I'm back in the land of Mollom on my PGSQL D7 install.
Comment #36
mkinnan CreditAttribution: mkinnan commentedCurrently having this same issue with 7-2.9, Drupal 7.26, and PostgreSQL.
I prefer using PSQL and this is diasspointing it will probably never be integrated into the module.
Looks like it's time to patch the files ...
Comment #37
guillaumev CreditAttribution: guillaumev commentedI can also confirm that the patch in #8 fixed the issue for me.
Comment #38
eshta CreditAttribution: eshta commentedGoing to take a stab at re-rolling this today and fixing up the tests as well as a review. Just wanted to chime in and let you all know that this hasn't fallen off the Mollom radar.
eshta
Mollom engineering
Comment #39
eshta CreditAttribution: eshta commentedSo what I tried to do here was to update all of the column names that are mixed case to underscore separated column names. This should alleviate the pain with various case sensitivity levels across databases and platforms. Then I updated the crud functions to allow for the changed names while still maintaining the status quo in the normal variable names that are passed around. The hope is that this limits the affected code and therefore the likelihood for error. I'd love a review, especially given that this falls kind of in between the two main approaches I see i this thread.
Comment #40
eshta CreditAttribution: eshta commentedComment #42
eshta CreditAttribution: eshta commentedHere's another attempt with corrections for the fact that the stdClass is passed by reference.... hopefully past tests ;-)
Comment #43
eshta CreditAttribution: eshta commentedComment #44
eshta CreditAttribution: eshta commentedCommitted patch for D7.
Attaching a backport to D6 now.
Comment #45
eshta CreditAttribution: eshta commentedSo excited to get this one in! This allows folks to come over to the 2.x line.
Comment #48
jweowu CreditAttribution: jweowu commentedFor anyone getting errors when removing the old mixed-case indexes, you may want to use this:
Comment #49
eshta CreditAttribution: eshta commentedThanks jweowu for adding that information in here. I would use the following (and will be updating this in the upgrade path as well for those who haven't updated:
Comment #50
eshta CreditAttribution: eshta commentedComment #51
jweowu CreditAttribution: jweowu commentedeshta: FYI that was how I wrote it the first time, and it didn't work.
I didn't dig for the exact details, but IIRC I didn't get an error, so I suspect
db_index_exists('mollom', 'contentId'))
does not end up double-quotingcontentId
for postgres, and so (as per comments in the issue description) it will be treated as lower-case and the test returns false.In any case, the original code (calling
db_drop_index()
) had failed before I made changes, so I presume that has the same quoting/case issue.Hence the raw SQL approach, with double-quotes.
I don't know whether there's a single cross-DBMS way you can write it?
Comment #52
eshta CreditAttribution: eshta commentedjweowu - good to know. Do you know it was quoting or do you think it was the differences in index names? Looks like there is a difference between mollom_contentId_idx and just 'content_id' as well.