Race condition in locale() - duplicates in {locales_source}

This looks like the textbook case for a merge query.

Locking patch could lead to serious bottleneck on i18n sites, by default code uses DB to perform locking

Also md5() been removed from the most of core

I think we should fix #851362: Add hash column to {locales_source} to query faster locale strings first

There was also #811158: We should not index on (source, context)

subscribing

Sure. Let's fix this one first and then think about uniqueness of strings!

Patch looks good, waiting for bot.

There's a issue about #1050616: Figure out backport workflow from Drupal 8 to Drupal 7
Let's decide on it

Tag for Drupal 8 Multilingual Initiative.

The questions are
- should we throw away VERSION as key?
- hook_update_N()
- D7 backport

+++ b/modules/locale/locale.moduleundefined
@@ -668,12 +668,14 @@ function locale($string = NULL, $context = NULL, $langcode = NULL) {
+        ->key(array(
           'source' => $string,
           'context' => (string) $context,
-          'version' => VERSION,

I think version key is useful

-2 days to next Drupal core point release.

I'm not 100% sure how we could write a sane update hook for this. We would need to identify string duplicates and then figure out if either of them have translations in any number of languages, and then try and merge those translations. If there are multiple translations for certain languages for certain duplicates, what would we do? Looks like a messy situation. Sounds like it would be preferable to stop the bleeding (get the fix in) and then figure out a data cleanup routine (which in itself looks like going to be no small feat).

I think we generally try to avoid data loss as much as possible, but I don't have great ideas to overcome it this time. I think however that that kind of data loss would be much more tolerated on a major upgrade vs. a minor one, so I'd put the data cleanup in the major upgrade, not the minor one. I understand that would leave D7 dbs in an inconsistent state, which might not indeed be good. What do you think?

That looks good to me, but we do need the D8 patch to land first and then "backport" to D7.

Can we get someone else to review/support these patches? :) That would be great.

Adding UI language translation tag.

Once again, any other opinions, others to verify the bug + the fix is good?

Checked the code, investigated and tested the merge query, agree on the hook_update approach.

Here is a very tiny change to conform to current coding standards. "Remove" changed to "Removes" in the first phpdoc line. No other change, RTBC should still stand.

Add backport tag to indicate this is bug to be fixed in D7 too. There is a solid D7 patch in #24 to be reviewed more and to land after the D8 patch landed.

+++ b/core/modules/locale/locale.install
@@ -278,6 +278,36 @@ function locale_update_8002() {
+  $results = db_query("SELECT source, context FROM {locales_source} GROUP BY source, context HAVING COUNT(*) > 1");

This query should be checked against pgsql and sqlite.

Sounds like you are saying this is not RTBC?

tested update with pgsql & sqlite, updated works!

Update 8002 was backed out in #1272840: Add upgrade path for language domains and validation so rerolling to attempt to use that number which is the current last number to take for updates. No other change so should still be RTBC as per above manual tests too.

#50 passed the test bot. Back to RTBC.

Tag for sprint to be easier to follow.

I'm not sure exactly in what order is best to commit D8MI patches, but since this one is a candidate for D7 backport, I started with it. :)

The switch to db_merge() is sensible. I'm a little nervous about the upgrade path accidentally destroying data. :\ But I can see that it does its best to be careful, and I think the reasoning of doing a "lite" data destruction in D7 and a "heavy" data destruction in D8 may be the way to handle that.

Thanks for the additional testing on pgsql and sqlite, too. Yay!

Committed and pushed to 8.x. Sounds like the D7 patch in #24 could use additional review (and possibly needs a re-roll; I'll check in a sec.)

#24: locale-race-condition-7-D7.patch queued for re-testing.

Reviewed again, looks good.

Ok, great!

Committed and pushed to 7.x. Tagging to mention in the 7.13 release notes.

After updating from Drupal core 7.12 to 7.14, I ran update.php. I got the following error:
Your {locales_source} table contains duplicates that could not be removed automatically. See http://drupal.org/node/746240 for more information.
After updating, no visible inconsistencies (related to this error) occurred on the website.

I'm not sure what to do with this. I don't know what caused this error and why it's referring my to a D6 issue. If this rings a bell for anyone, please let me know!

@mhlut I had real world errors and crashes because of this, but in very peculiar cases. If you don't experience anything, you can leave them. But I would advice you to get rid of it: it's pretty simple to achieve:

1. Export all your translations as .po files
2. TRUNCATE the locales_source and locales_target SQL tables
3. Import your exported .po files

EDIT: In any cases, backup your database first in case something goes wrong during this procedure!

@c960657, the update hook in your D6 patch only touches rows with textgroup = 'default'. Is that by design? Should it be modified to remove duplicates in all textgroups?

I think we should leave other textgroups alone, they were never managed by core and should not be touched by core updates IMHO.

Fixing tags per http://drupal.org/node/1517250

Just found this as I have seen this in d7 still, but I believe that is due to race condition in db_merge itself due to heavy traffic (e.g. performance tests).