From #1096750: Display RSS feed for new projects (without sandbox projects), found a small bug, when edit a project and save it:

user warning: Duplicate entry '3-1' for key 'PRIMARY' query: INSERT INTO project_maintainer (nid, uid, edit_project, administer_maintainers) VALUES (3, 1, 1, 1)

The problem is db_affected_rows returns FALSE on project_maintainer_save because existing record has the same values.

I've attached simple patch to fix this issue, using db_query_range to check if to update an existing record or insert a new row.

CommentFileSizeAuthor
#2 project-duplicate-maintainer-2.patch1.43 KBsmartinm
project-duplicate-maintainer.patch1.35 KBsmartinm

Comments

dww’s picture

dww
we/he/they
commented

Hrmph. That's unfortunate, since this approach appears a lot as a way to do the equivalent of a D7 merge query. :/

In this case, since there's no auto-increment ID we're trying to preserve, maybe it makes more sense to just always do a DELETE then INSERT?

smartinm’s picture

smartinm commented
StatusFileSize
new project-duplicate-maintainer-2.patch1.43 KB

If we keep using db_affected_rows, we can use the @ error suppression directive, for example search.module in search_update_totals():

    db_query("UPDATE {search_total} SET count = %f WHERE word = '%s'", $total, $word);
    if (!db_affected_rows()) {
      @db_query("INSERT INTO {search_total} (word, count) VALUES ('%s', %f)", $word, $total);
    }

More info:
#798626: Cron produces duplicate entry error in search.module
#638702: insertion errors

As the db_affected_rows function has been removed D7, the strategy that you say could be better. Attached a new patch that does a DELETE then INSERT.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Both patches look good to me, just needs a decision which one is preferred. Thinking about performance, both result in always two queries but SELECT + UPDATE/INSERT might be slightly faster.

And yes, core usually uses @ although I don't really like using @ personally :)

Edit: This actually causes exceptions in the current tests, so would be great to have it fixed.

smartinm’s picture

smartinm commented

Indeed, the main purpose of this patch is to fix the exceptions in the tests :)

Regarding patches, I think the first patch (SELECT + UPDATE/INSERT) is a bit more understandable.

smartinm’s picture

smartinm commented
Status: Needs review » Reviewed & tested by the community

Marking as RTBC, both patches work fine, just needs choose one ;)

mikey_p’s picture

mikey_p commented

I think I'd rather see select + (insert|update) vs. delete. It just feels better and while not supporting ideal concurrency, would be better than delete/insert.

I'm not a fan of db_affected_rows() and I'm definitely not a fan of @ error surpression either.

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Hrm. I can't locally reproduce this error at all. Neither manually editing projects, nor manually manipulating maintainers, nor when running the project simpletests (which do both of those automatically). When I run the project tests locally, I just get:

253 passes, 0 fails, 0 exceptions, and 75 debug messages

My local test environment is PHP 5.2, maybe that explains the difference?

Can someone explain how to reproduce the errors here? I'm willing to just commit something, but I'd rather verify that the bug exists and then see the patch fix it.

Thanks!
-Derek

smartinm’s picture

smartinm commented

Ok, I think I have found the source of the problem, this error only occurs when use the 'mysqli' extension instead of 'mysql' and (i think) only with PHP 5.3. To reproduce change your site's $db_url in settings.php to use mysqli, create a project, edit the project and save it.

The source of the problem appears to be this PHP bug:

http://bugs.php.net/bug.php?id=53425

In some builds of MySQLi PHP extension, mysqli_real_connect ignores MYSQLI_CLIENT_FOUND_ROWS flag, necessary to db_affected_rows work properly. Drupal uses MYSQLI_CLIENT_FOUND_ROWS flag on db_connect (database.mysqli.inc):

...
$connection = mysqli_init();
@mysqli_real_connect($connection, $url['host'], $url['user'], $url['pass'], substr($url['path'], 1), $url['port'], NULL, MYSQLI_CLIENT_FOUND_ROWS); 
...

The PHP bug has been fixed in PHP 5.3.5. The question is whether we fix this bug in project.module. I think you should apply this patch because this bug actually causes exceptions in the Drupal QA/PIFR testing environment.

bdragon’s picture

bdragon commented
Assigned: Unassigned » bdragon
Status: Postponed (maintainer needs more info) » Fixed

I am preempting this issue because it is breaking the branch tests.

It is a drupal *pattern* in D6 to have the @ to work around differences. See, e.g. variable_set().

I have committed the @ fix and will rerun the branch tests now.

http://drupalcode.org/project/project.git/commit/da770cae5e633e3e6e48051...

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Fixed » Active

Actually this is a bad pattern to use. Because if there is an existing record in the table, and the UPDATE query doesn't actually change any field values, then db_affected_rows() returns 0. It is a better pattern to first run a DELETE and then an INSERT rather than UPDATE and then INSERT with error-suppression.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Fixed

Meh, I guess this was already discussed. It's not great to be suppressing the errors because it still causes a small hit by having the error in the first place.

bdragon’s picture

bdragon commented

Yeah, it's a sucky pattern, but if the row is already what we want it to be then we don't care anyway.
The thing that makes it tolerable my eye is mainly that it's an easy to recognize block of code, and it makes me think "Oh, yeah, that right there is a D6 style pseudo-merge query".

Actually, when I looked at the code (before I dug up this issue), the *lack* of the @ there stood out.

In D7 of course we have much better options.

I'm just happy the tests aren't wedged anymore. ;)

dww’s picture

dww
we/he/they
commented

Right, and always doing delete/insert is crappy if you have an auto-increment ID that changes all the time...

Anyway, agreed, this isn't ideal, but neither is developing software in PHP. ;)

Thanks for the fix, bdragon!

Cheers,
-Derek

Status: Fixed » Closed (fixed)

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