"Duplicate entry" problems caused by db_affected_rows() returning 0

I think that the problem is in nodecomment_save().

  // Try an update first, do not change the original submitted IP Address.
  db_query("UPDATE {node_comments} SET nid = %d, pid = %d, thread = '%s', name = '%s', uid = %d, mail = '%s', homepage = '%s' WHERE cid = %d", $node->comment_target_nid, $node->comment_target_cid, $node->thread, $node->name, $node->uid, $node->mail, $node->homepage, $node->nid);
  // If not updated, insert a new comment.
  if (db_affected_rows() == 0) {
    db_query("INSERT INTO {node_comments} (cid, nid, pid, hostname, thread, name, uid, mail, homepage) VALUES (%d, %d, %d, '%s', '%s', '%s', %d, '%s', '%s')", $node->nid, $node->comment_target_nid, $node->comment_target_cid, ip_address(), $node->thread, $node->name, $node->uid, $node->mail, $node->homepage);
  }

The code first assumes that the comment already exists and attempts an UPDATE of the node_comment row. If db_affected_rows() returns zero then it assumes that this is a new comment and does an INSERT.

The problem is that when the columns in an UPDATE have the same values as the existing row the database does not bother updating the row and does not increment the affected rows counter for the query (this is the behaviour for MySQL, I don't know what other databases do). Therefore db_affected_rows() can return 0 when the comment does in fact exist.

I don't think we can tell from the state of $node whether we are dealing with an existing comment or not so we need to query the database to decide.

Here is a patch. My first git patch!

I have changed the nodecomment_save() function signature to accept an additional boolean parameter that indicates whether we are inserting a new comment or updating and existing one. I then modified the calls from nodecomment_nodeapi() to pass TRUE/FALSE as appropriate.

I have tested this by adding new comment nodes and updating existing ones.

I created a new version of this patch, that passes $op to node_save() rather than using a boolean flag. This may just be a bikeshed (ie. personal taste) but it seems better to me not to duplicate the code in hook_nodeapi().

Regards,
David.

Nice work dsnopek. Much cleaner. Thanks

I can't reproduce it

Please post detailed instructions to reproduce

1. Make sure that you have error reporting turned on (via http://localhost/admin/settings/error-reporting)
2. Go to an existing node which uses node comments
3. Post a new comment
4. Edit the comment and save
5. The message should appear!

I encounter this using Drupal 6.22, PHP 5.3.2, MySQL 5.1.49 (I suspect this is MySQL-specific, just in case you are using Postregres) and nodecomment 2.0-beta6.

Let me know if you need more information!

Regards,
David.

Drupal MySQL driver uses CLIENT_FOUND_ROWS flag which tells Mysql to return number of matched rows instead of number of updated ones.See db_connect() function.

  // - 2 means CLIENT_FOUND_ROWS: return the number of found
  //   (matched) rows, not the number of affected rows.

Drupal MySQLi driver uses similar flag MYSQLI_CLIENT_FOUND_ROWS.

That means either your database is badly configured or it's a bug in your version of MySQL.

Hi crea,

Thanks again for your advice!

However, I'm having considerable difficulty tracking this problem down. I'm using mysqli. I searched and searched for any documented problems with MYSQLI_CLIENT_FOUND_ROWS but found nothing.

The version of MySQL I'm using is the default for Ubuntu 10.10, so I don't think that's likely to be the problem. But I compiled PHP 5.3.2 myself, so the problem could lie there.

I think my next step will be to create a minimal PHP script which uses mysqli directly that can show the bug. But, unfortunately, I've run out of energy for this today. I'll update the ticket if I figure it out. Hopefully, the information will be useful to others.

Best regards,
David.

Since this problem appears to be quite common, let's keep this issue open as information source.

#805858: Affected rows inconsistent across database engines

What we might be able to do is just to ignore INSERT errors. I've committed a patch implementing it.

The fix will go into the next release so closing.

Thanks for the link in comment 14! This comment shows me pretty much EXACTLY what I need to compile differently with PHP in order to get it to work. I haven't tried it yet, but this really looks like the solution.

Regards,
David.

Ok, I figured out how to compile PHP differently, such that it doesn't have this bug!

Basically, you need to get your mysql or mysqli to use mysqlnd. Apparently, some distributions (ie. RedHat) don't use mysqlnd in their pre-packed version of PHP, which is unfortunate for users of those distributions. However, if you are compiling your own this should help!

Previously, I was passing the following to ./configure:

  --with-mysqli=/usr/bin/mysql_config

Which I changed to:

  --with-mysqli=mysqlnd

Immediately after installing, you may get an error connecting to the database: No such file or directory. To fix that, set the MySQL socket location in php.ini. On my system (Ubuntu 10.10), this is the line I added:

  mysqli.default_socket = /var/run/mysqld/mysqld.sock

I hope that helps anyone else who encounters this problem!

Regards,
David.