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(

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Mollom 7.x-2.0 does not work with PostgreSQL » 2.x series is incompatible with PostgreSQL
Version: 7.x-2.0 » 7.x-2.x-dev
Issue tags: +PostgreSQL

#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?

sun’s picture

Status: Active » Postponed (maintainer needs more info)

@jaredsmith: The Drupal core fix mentioned in #1 is contained in 7.14 now. Could you try whether the module works now?

mikl’s picture

Status: Postponed (maintainer needs more info) » Active

I 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:

WD php: PDOException: SQLSTATE[42P01]: Undefined table: 7 ERROR:  relation "mollom_form" does not exist [error]
LINE 1: SELECT form_id FROM mollom_form
                            ^: SELECT form_id FROM {mollom_form}; Array
(
)
 in mollom_field_extra_fields() (line 2239 of /srv/www/development/d7/sites/all/modules/contrib/mollom/mollom.module).
mikl’s picture

This is the error I get when I try to install Mollom now:

% drush #dev.revealit.dk en mollom 
The following extensions will be enabled: mollom
Do you really want to continue? (y/n): y
WD php: PDOException: SQLSTATE[42703]: Undefined column: 7 ERROR:  column "contentId" does not exist: CREATE INDEX "mollom_contentId_idx" ON {mollom} ("contentId"); Array                                                                                                                                                                                                                                                    [error]
(
)
 in db_create_table() (line 2685 of /srv/www/development/d7/includes/database/database.inc).
Cannot modify header information - headers already sent by (output started at /usr/local/share/drush/includes/output.inc:37) bootstrap.inc:1239                                                                                                                                                                                                                                                                               [warning]
PDOException: SQLSTATE[42703]: Undefined column: 7 ERROR:  column "contentId" does not exist: CREATE INDEX "mollom_contentId_idx" ON {mollom} ("contentId"); Array
(
)
 in db_create_table() (line 2685 of /srv/www/development/d7/includes/database/database.inc).
Drush command terminated abnormally due to an unrecoverable error.                                    

I think the easiest way to solve this is to simply use lower-case for all table and column names.

iSylence’s picture

Status: Active » Needs review
FileSize
590 bytes

Drupal 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.

Status: Needs review » Needs work

The last submitted patch, fix_issue_with_db_schema-1529268-6.patch, failed testing.

iSylence’s picture

Status: Needs work » Needs review
FileSize
458 bytes

Sorry about that, fixed up the paths and reattached patch.

tomww’s picture

With 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(

sun’s picture

mikl’s picture

@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?

mikl’s picture

Even 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).

aacraig’s picture

+1 for all lower case names in databases. Avoids these unnecessary and time consuming issues.

hynnot’s picture

+1 #9 solution by tomww

jaredsmith’s picture

Sun,

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.

sun’s picture

Status: Needs review » Needs work

Sorry, 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.

tomww’s picture

I'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(

jneubert’s picture

The workarround in #19 solved it for me.

Thanks a lot - Joachim

smscotten’s picture

Is there any chance that this fix will ever get adopted?

maikeru’s picture

Confirming that patch at #17 worked for me on Postgres using Mollom 2.7.

drupdan3’s picture

What is the status of this bug? Why is it "needs work"? Are the module authors aware of the problem and the patch?

mikl’s picture

Status: Needs work » Reviewed & tested by the community

#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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, mollom.install-lowercase-contentid-capchaid-2.4-20130122.diff, failed testing.

burningdog’s picture

Status: Needs work » Needs review
FileSize
21.32 KB

Following 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.

Title: 2.x series is incompatible with PostgreSQL » 2.x series is incompatible with PostgreSQL due to use of uppercase in column names
Priority: Normal » Critical

The last submitted patch, mollom-postgres_breaks-1529268-24.patch, failed testing.

burningdog’s picture

Oh 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.

Status: Needs review » Needs work

The last submitted patch, mollom-postgres_breaks-1529268-25.patch, failed testing.

burningdog’s picture

Hmmm. Looks like the Mollom API wants a case-sensitive value of captchaid - here's a failing test:

Response: 400 Bad Request
  code = '400'
  message = 'Missing resource ID'
Error 400: Bad Request (50.23.148.2)

Request: POST http://dev.mollom.com/v1/feedback
  captchaid = 'TESTdhheuk84sp2jzm'
  reason = 'spam'

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?

drupdan3’s picture

@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?

burningdog’s picture

Some 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!

acbramley’s picture

Can 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 it

smscotten’s picture

Since 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.

bsisco’s picture

I 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:

$ psql <dbname> <username>
<dbname>=>drop table mollom;
DROP TABLE
<dbname>=>\q

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 :)

acbramley’s picture

@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:

drush dis module
drush pm-uninstall module

Drupal 7 does not require uninstall hooks to remove the db tables.

smscotten’s picture

@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.

mkinnan’s picture

Issue summary: View changes

Currently 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 ...

guillaumev’s picture

I can also confirm that the patch in #8 fixed the issue for me.

eshta’s picture

Going 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

eshta’s picture

FileSize
9.07 KB

So 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.

eshta’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: mollom.postgres.39.patch, failed testing.

eshta’s picture

FileSize
9.24 KB

Here's another attempt with corrections for the fact that the stdClass is passed by reference.... hopefully past tests ;-)

eshta’s picture

Status: Needs work » Needs review
eshta’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev
FileSize
8.88 KB

Committed patch for D7.
Attaching a backport to D6 now.

eshta’s picture

Status: Needs review » Fixed

So excited to get this one in! This allows folks to come over to the 2.x line.

  • Commit 39d405c on 7.x-2.x, fbajs by eshta:
    Issue #1529268 by eshta: 2.x series is incompatible with PostgresSQL due...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jweowu’s picture

For anyone getting errors when removing the old mixed-case indexes, you may want to use this:

db_query('DROP INDEX IF EXISTS "mollom_contentId_idx";');
db_query('DROP INDEX IF EXISTS "mollom_captchaId_idx";');
eshta’s picture

Thanks 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:

  if (db_index_exists('mollom', 'contentId')) {
    db_drop_index('mollom', 'contentId');
  }
  if (db_index_exists('mollom', 'captchaId')) {
    db_drop_index('mollom', 'captchaId');
  }
eshta’s picture

jweowu’s picture

eshta: 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-quoting contentId 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?

eshta’s picture

jweowu - 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.