Addressing code review comments

h2/src/docsrc/html/tutorial.html

@@ -1320,13 +1320,6 @@ SELECT * FROM FTL_SEARCH_DATA('John', 0, 0);
 SELECT * FROM FTL_SEARCH_DATA('LAST_NAME:John', 0, 0);
 CALL FTL_DROP_ALL();
 </pre>
-<p>
-The Lucene fulltext search implementation is not synchronized internally.
-If you update the database and query the fulltext search concurrently
-(directly using the Java API of H2 or Lucene itself), you need to ensure
-operations are properly synchronized. If this is not the case, you may get
-exceptions such as <code>org.apache.lucene.store.AlreadyClosedException: this IndexReader is closed</code>.
-</p>
 
 <h2 id="user_defined_variables">User-Defined Variables</h2>
 <p>

h2/src/main/org/h2/fulltext/FullText.java

@@ -153,13 +153,12 @@ public class FullText {
             }
         }
         rs = stat.executeQuery("SELECT * FROM " + SCHEMA + ".WORDS");
-        Map<String, Integer> map = setting.getWordList();
         while (rs.next()) {
             String word = rs.getString("NAME");
             int id = rs.getInt("ID");
             word = setting.convertWord(word);
             if (word != null) {
-                map.put(word, id);
+                setting.addWord(word, id);
             }
         }
         setting.setInitialized(true);
@@ -197,7 +196,7 @@ public class FullText {
         init(conn);
         removeAllTriggers(conn, TRIGGER_PREFIX);
         FullTextSettings setting = FullTextSettings.getInstance(conn);
-        setting.getWordList().clear();
+        setting.clearWordList();
         Statement stat = conn.createStatement();
         stat.execute("TRUNCATE TABLE " + SCHEMA + ".WORDS");
         stat.execute("TRUNCATE TABLE " + SCHEMA + ".ROWS");
@@ -268,8 +267,8 @@ public class FullText {
         removeAllTriggers(conn, TRIGGER_PREFIX);
         FullTextSettings setting = FullTextSettings.getInstance(conn);
         setting.removeAllIndexes();
-        setting.getIgnoreList().clear();
-        setting.getWordList().clear();
+        setting.clearInored();
+        setting.clearWordList();
     }
 
     /**
@@ -610,14 +609,13 @@ public class FullText {
         Set<String> words = New.hashSet();
         addWords(setting, words, text);
         Set<Integer> rIds = null, lastRowIds;
-        Map<String, Integer> allWords = setting.getWordList();
 
         PreparedStatement prepSelectMapByWordId = setting.prepare(conn,
                 SELECT_MAP_BY_WORD_ID);
         for (String word : words) {
             lastRowIds = rIds;
             rIds = New.hashSet();
-            Integer wId = allWords.get(word);
+            Integer wId = setting.getWordId(word);
             if (wId == null) {
                 continue;
             }
@@ -765,9 +763,12 @@ public class FullText {
                     + StringUtils.quoteIdentifier(TRIGGER_PREFIX + table);
             stat.execute("DROP TRIGGER IF EXISTS " + trigger);
             if (create) {
-                ResultSet rs = stat.executeQuery("SELECT value FROM information_schema.settings WHERE name = 'MV_STORE'");
+                ResultSet rs = stat.executeQuery("SELECT value" +
+                        " FROM information_schema.settings" +
+                        " WHERE name = 'MV_STORE'");
                 boolean multiThread = FullTextTrigger.isMultiThread(conn);
-                StringBuilder buff = new StringBuilder("CREATE TRIGGER IF NOT EXISTS ");
+                StringBuilder buff = new StringBuilder(
+                        "CREATE TRIGGER IF NOT EXISTS ");
                 // unless multithread, trigger needs to be called on rollback as well,
                 // because we use the init connection do to changes in the index
                 // (not the user connection)
@@ -833,13 +834,7 @@ public class FullText {
     private static void setIgnoreList(FullTextSettings setting,
             String commaSeparatedList) {
         String[] list = StringUtils.arraySplit(commaSeparatedList, ',', true);
-        Set<String> set = setting.getIgnoreList();
-        for (String word : list) {
-            String converted = setting.convertWord(word);
-            if (converted != null) {
-                set.add(converted);
-            }
-        }
+        setting.addInored(Arrays.asList(list));
     }
 
     /**
@@ -885,12 +880,12 @@ public class FullText {
         private static final int SELECT_ROW  = 5;
 
         private static final String SQL[] = {
-                "INSERT INTO " + SCHEMA + ".WORDS(NAME) VALUES(?)",
-                "INSERT INTO " + SCHEMA + ".ROWS(HASH, INDEXID, KEY) VALUES(?, ?, ?)",
-                "INSERT INTO " + SCHEMA + ".MAP(ROWID, WORDID) VALUES(?, ?)",
-                "DELETE FROM " + SCHEMA + ".ROWS WHERE HASH=? AND INDEXID=? AND KEY=?",
-                "DELETE FROM " + SCHEMA + ".MAP WHERE ROWID=? AND WORDID=?",
-                "SELECT ID FROM " + SCHEMA + ".ROWS WHERE HASH=? AND INDEXID=? AND KEY=?"
+            "MERGE INTO " + SCHEMA + ".WORDS(NAME) KEY(NAME) VALUES(?)",
+            "INSERT INTO " + SCHEMA + ".ROWS(HASH, INDEXID, KEY) VALUES(?, ?, ?)",
+            "INSERT INTO " + SCHEMA + ".MAP(ROWID, WORDID) VALUES(?, ?)",
+            "DELETE FROM " + SCHEMA + ".ROWS WHERE HASH=? AND INDEXID=? AND KEY=?",
+            "DELETE FROM " + SCHEMA + ".MAP WHERE ROWID=? AND WORDID=?",
+            "SELECT ID FROM " + SCHEMA + ".ROWS WHERE HASH=? AND INDEXID=? AND KEY=?"
         };
 
         /**
@@ -939,7 +934,8 @@ public class FullText {
             }
             ArrayList<String> indexList = New.arrayList();
             PreparedStatement prep = conn.prepareStatement(
-                    "SELECT ID, COLUMNS FROM " + SCHEMA + ".INDEXES WHERE SCHEMA=? AND TABLE=?");
+                    "SELECT ID, COLUMNS FROM " + SCHEMA + ".INDEXES" +
+                    " WHERE SCHEMA=? AND TABLE=?");
             prep.setString(1, schemaName);
             prep.setString(2, tableName);
             rs = prep.executeQuery();
@@ -1115,25 +1111,25 @@ public class FullText {
             PreparedStatement prepInsertWord = null;
             try {
                 prepInsertWord = getStatement(conn, INSERT_WORD);
-                Map<String, Integer> allWords = setting.getWordList();
                 int[] wordIds = new int[words.size()];
-                synchronized (allWords) {
-                    int i = 0;
-                    for (String word : words) {
-                        Integer wId = allWords.get(word);
-                        int wordId;
-                        if (wId == null) {
-                            prepInsertWord.setString(1, word);
-                            prepInsertWord.execute();
-                            ResultSet rs = prepInsertWord.getGeneratedKeys();
-                            rs.next();
+                int i = 0;
+                for (String word : words) {
+                    int wordId;
+                    Integer wId;
+                    while((wId = setting.getWordId(word)) == null) {
+                        prepInsertWord.setString(1, word);
+                        prepInsertWord.execute();
+                        ResultSet rs = prepInsertWord.getGeneratedKeys();
+                        if (rs.next()) {
                             wordId = rs.getInt(1);
-                            allWords.put(word, wordId);
-                        } else {
-                            wordId = wId;
+                            if (wordId != 0) {
+                                setting.addWord(word, wordId);
+                                wId = wordId;
+                                break;
+                            }
                         }
-                        wordIds[i++] = wordId;
                     }
+                    wordIds[i++] = wId;
                 }
                 Arrays.sort(wordIds);
                 return wordIds;

h2/src/main/org/h2/fulltext/FullTextSettings.java

@@ -25,7 +25,7 @@ final class FullTextSettings {
     /**
      * The settings of open indexes.
      */
-    private static final Map<String, FullTextSettings> SETTINGS = Collections.synchronizedMap(New.<String, FullTextSettings>hashMap());
+    private static final Map<String, FullTextSettings> SETTINGS = New.hashMap();
 
     /**
      * Whether this instance has been initialized.
@@ -35,12 +35,12 @@ final class FullTextSettings {
     /**
      * The set of words not to index (stop words).
      */
-    private final Set<String> ignoreList = Collections.synchronizedSet(New.<String>hashSet());
+    private final Set<String> ignoreList = New.hashSet();
 
     /**
      * The set of words / terms.
      */
-    private final Map<String, Integer> words = Collections.synchronizedMap(New.<String, Integer>hashMap());
+    private final Map<String, Integer> words = New.hashMap();
 
     /**
      * The set of indexes in this database.
@@ -67,21 +67,58 @@ final class FullTextSettings {
     }
 
     /**
-     * Get the ignore list.
-     *
-     * @return the ignore list
+     * Clear set of ignored words
      */
-    protected Set<String> getIgnoreList() {
-        return ignoreList;
+    public void clearInored() {
+        synchronized (ignoreList) {
+            ignoreList.clear();
+        }
     }
 
     /**
-     * Get the word list.
-     *
-     * @return the word list
+     * Amend set of ignored words
+     * @param words to add
+     */
+    public void addInored(Iterable<String> words) {
+        synchronized (ignoreList) {
+            for (String word : words) {
+                word = normalizeWord(word);
+                ignoreList.add(word);
+            }
+        }
+    }
+
+    /**
+     * Clear set of searchable words
      */
-    protected Map<String, Integer> getWordList() {
-        return words;
+    public void clearWordList() {
+        synchronized (words) {
+            words.clear();
+        }
+    }
+
+    /**
+     * Get id for a searchable word
+     * @param word to find id for
+     * @return Integer id or null if word is not found
+     */
+    public Integer getWordId(String word) {
+        synchronized (words) {
+            return words.get(word);
+        }
+    }
+
+    /**
+     * Register searchable word
+     * @param word to register
+     * @param id to register with
+     */
+    public void addWord(String word, Integer id) {
+        synchronized (words) {
+            if(!words.containsKey(word)) {
+                words.put(word, id);
+            }
+        }
     }
 
     /**
@@ -111,10 +148,11 @@ final class FullTextSettings {
      * @return the uppercase version of the word or null
      */
     protected String convertWord(String word) {
-        // TODO this is locale specific, document
-        word = word.toUpperCase();
-        if (ignoreList.contains(word)) {
-            return null;
+        word = normalizeWord(word);
+        synchronized (ignoreList) {
+            if (ignoreList.contains(word)) {
+                return null;
+            }
         }
         return word;
     }
@@ -128,10 +166,13 @@ final class FullTextSettings {
     protected static FullTextSettings getInstance(Connection conn)
             throws SQLException {
         String path = getIndexPath(conn);
-        FullTextSettings setting = SETTINGS.get(path);
-        if (setting == null) {
-            setting = new FullTextSettings();
-            SETTINGS.put(path, setting);
+        FullTextSettings setting;
+        synchronized (SETTINGS) {
+            setting = SETTINGS.get(path);
+            if (setting == null) {
+                setting = new FullTextSettings();
+                SETTINGS.put(path, setting);
+            }
         }
         return setting;
     }
@@ -220,7 +261,9 @@ final class FullTextSettings {
      * Close all fulltext settings, freeing up memory.
      */
     protected static void closeAll() {
-        SETTINGS.clear();
+        synchronized (SETTINGS) {
+            SETTINGS.clear();
+        }
     }
 
     protected void setWhitespaceChars(String whitespaceChars) {
@@ -231,4 +274,8 @@ final class FullTextSettings {
         return whitespaceChars;
     }
 
+    private String normalizeWord(String word) {
+        // TODO this is locale specific, document
+        return word.toUpperCase();
+    }
 }

h2/src/test/org/h2/test/db/TestFullText.java

@@ -22,6 +22,7 @@ import java.util.concurrent.TimeUnit;
 
 import org.h2.fulltext.FullText;
 import org.h2.store.fs.FileUtils;
+import org.h2.test.TestAll;
 import org.h2.test.TestBase;
 import org.h2.util.IOUtils;
 import org.h2.util.Task;
@@ -48,6 +49,12 @@ public class TestFullText extends TestBase {
         TestBase.createCaller().init().test();
     }
 
+    @Override
+    public TestBase init(TestAll conf) throws Exception {
+        conf.lockTimeout = 60000;
+        return super.init(conf);
+    }
+
     @Override
     public void test() throws Exception {
         testUuidPrimaryKey(false);