MDL-58593 database: More efficiently track issued key names

lib/ddl/sql_generator.php

@@ -1074,8 +1074,11 @@ abstract class sql_generator {
         // along all the request life, but never to return cached results
         // We need this because sql statements are created before executing
         // them, hence names doesn't exist "physically" yet in DB, so we need
-        // to known which ones have been used
+        // to known which ones have been used.
-        static $used_names = array();
+        // We track all the keys used, and the previous counters to make subsequent creates faster.
+        // This may happen a lot with things like bulk backups or restores.
+        static $usednames = array();
+        static $previouscounters = array();
 
         // Use standard naming. See http://docs.moodle.org/en/XMLDB_key_and_index_naming
         $tablearr = explode ('_', $tablename);
@@ -1094,17 +1097,26 @@ abstract class sql_generator {
         $maxlengthwithoutsuffix = $this->names_max_length - strlen($suffix) - ($suffix ? 1 : 0);
         $namewithsuffix = substr($name, 0, $maxlengthwithoutsuffix) . ($suffix ? ('_' . $suffix) : '');
 
-        // If the calculated name is in the cache, or if we detect it by introspecting the DB let's modify if
+        if (isset($previouscounters[$name])) {
+            // If we have a counter stored, we will need to modify the key to the next counter location.
+            $counter = $previouscounters[$name] + 1;
+            $namewithsuffix = substr($name, 0, $maxlengthwithoutsuffix - strlen($counter)) .
+                    $counter . ($suffix ? ('_' . $suffix) : '');
+        } else {
             $counter = 1;
-        while (in_array($namewithsuffix, $used_names) || $this->isNameInUse($namewithsuffix, $suffix, $tablename)) {
+        }
+
+        // If the calculated name is in the cache, or if we detect it by introspecting the DB let's modify it.
+        while (isset($usednames[$namewithsuffix]) || $this->isNameInUse($namewithsuffix, $suffix, $tablename)) {
             // Now iterate until not used name is found, incrementing the counter
             $counter++;
             $namewithsuffix = substr($name, 0, $maxlengthwithoutsuffix - strlen($counter)) .
                     $counter . ($suffix ? ('_' . $suffix) : '');
         }
 
-        // Add the name to the cache
+        // Add the name to the cache. Using key look with isset because it is much faster than in_array.
-        $used_names[] = $namewithsuffix;
+        $usednames[$namewithsuffix] = true;
+        $previouscounters[$name] = $counter;
 
         // Quote it if necessary (reserved words)
         $namewithsuffix = $this->getEncQuoted($namewithsuffix);

lib/ddl/tests/ddl_test.php

@@ -2080,6 +2080,38 @@ class core_ddl_testcase extends database_driver_testcase {
                         strlen($gen->getNameForObject($table, $fields, $suffix)),
                         'Generated object name is too long. $i = '.$i);
             }
+
+            // Now test to confirm that a duplicate name isn't issued, even if they come from different root names.
+            // Move to a new field.
+            $fields = "fl";
+
+            // Insert twice, moving is to a key with fl2.
+            $this->assertEquals($gen->names_max_length - 1, strlen($gen->getNameForObject($table, $fields, $suffix)));
+            $result1 = $gen->getNameForObject($table, $fields, $suffix);
+
+            // Make sure we end up with _fl2_ in the result.
+            $this->assertRegExp('/_fl2_/', $result1);
+
+            // Now, use a field that would result in the same key if it wasn't already taken.
+            $fields = "fl2";
+            // Because we are now at the max key length, it will try:
+            // - _fl2_ (the natural name)
+            // - _fl2_ (removing the original 2, and adding a counter 2)
+            // - then settle on _fl3_.
+            $result2 = $gen->getNameForObject($table, $fields, $suffix);
+            $this->assertRegExp('/_fl3_/', $result2);
+
+            // Make sure they don't match.
+            $this->assertNotEquals($result1, $result2);
+            // But are only different in the way we expect. This confirms the test is working properly.
+            $this->assertEquals(str_replace('_fl2_', '', $result1), str_replace('_fl3_', '', $result2));
+
+            // Now go back. We would expect the next result to be fl3 again, but it is taken, so it should move to fl4.
+            $fields = "fl";
+            $result3 = $gen->getNameForObject($table, $fields, $suffix);
+
+            $this->assertNotEquals($result2, $result3);
+            $this->assertRegExp('/_fl4_/', $result3);
         }
     }