Standardize IN clause generation (#7221)

Author: labkey-adam

api/src/org/labkey/api/attachments/LookAndFeelResourceType.java

@@ -41,6 +41,7 @@ private LookAndFeelResourceType()
     @Override
     public void addWhereSql(SQLFragment sql, String parentColumn, String documentNameColumn)
     {
+        // Keep in sync with CoreMigrationSchemaHandler.copyAttachments()
         sql.append(parentColumn).append(" IN (SELECT EntityId FROM ").append(CoreSchema.getInstance().getTableInfoContainers(), "c").append(") AND (");
         sql.append(documentNameColumn).append(" IN (?, ?) OR ");
         sql.add(AttachmentCache.FAVICON_FILE_NAME);

api/src/org/labkey/api/data/SimpleFilter.java

@@ -1865,6 +1865,41 @@ private int howManyLessThan(List<Integer> userIdsDesc, int max)
             }
             return howMany;
         }
+
+        /**
+         * This used to be a PostgreSQL-specific test, but it should run and pass on SQL Server as well. It's largely
+         * redundant with testLargeInClause() above, but causes no harm.
+         */
+        @Test
+        public void testTempTableInClause()
+        {
+            DbSchema core = CoreSchema.getInstance().getSchema();
+            SqlDialect d = core.getSqlDialect();
+
+            Collection<Integer> allUserIds = new TableSelector(CoreSchema.getInstance().getTableInfoUsersData(), Collections.singleton("UserId")).getCollection(Integer.class);
+            SQLFragment shortSql = new SQLFragment("SELECT * FROM core.UsersData WHERE UserId");
+            d.appendInClauseSql(shortSql, allUserIds);
+            assertEquals(allUserIds.size(), new SqlSelector(core, shortSql).getRowCount());
+
+            ArrayList<Object> l = new ArrayList<>(allUserIds);
+            while (l.size() < SqlDialect.TEMP_TABLE_GENERATOR_MIN_SIZE)
+                l.addAll(allUserIds);
+            SQLFragment longSql = new SQLFragment("SELECT * FROM core.UsersData WHERE UserId");
+            d.appendInClauseSql(longSql, l);
+            assertEquals(allUserIds.size(), new SqlSelector(core, longSql).getRowCount());
+
+            Collection<String> allDisplayNames = new TableSelector(CoreSchema.getInstance().getTableInfoUsersData(), Collections.singleton("DisplayName")).getCollection(String.class);
+            SQLFragment shortSqlStr = new SQLFragment("SELECT * FROM core.UsersData WHERE DisplayName");
+            d.appendInClauseSql(shortSqlStr, allDisplayNames);
+            assertEquals(allDisplayNames.size(), new SqlSelector(core, shortSqlStr).getRowCount());
+
+            l = new ArrayList<>(allDisplayNames);
+            while (l.size() < SqlDialect.TEMP_TABLE_GENERATOR_MIN_SIZE)
+                l.addAll(allDisplayNames);
+            SQLFragment longSqlStr = new SQLFragment("SELECT * FROM core.UsersData WHERE DisplayName");
+            d.appendInClauseSql(longSqlStr, l);
+            assertEquals(allDisplayNames.size(), new SqlSelector(core, longSqlStr).getRowCount());
+        }
     }
 
     public static class BetweenClauseTestCase extends ClauseTestCase

api/src/org/labkey/api/data/TempTableInClauseGenerator.java

@@ -142,7 +142,9 @@ else if (jdbcType == JdbcType.INTEGER)
             try (var ignored = SpringActionController.ignoreSqlUpdates())
             {
                 new SqlExecutor(tempSchema).execute(indexSql);
+                tempSchema.getSqlDialect().updateStatistics(tempTableInfo); // Immediately analyze the newly populated & indexed table
             }
+
             TempTableInfo cacheEntry = tempTableInfo;
 
             // Don't bother caching if we're in a transaction

api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java

@@ -34,7 +34,6 @@
 import org.labkey.api.data.DbSchemaType;
 import org.labkey.api.data.DbScope;
 import org.labkey.api.data.DbScope.LabKeyDataSource;
-import org.labkey.api.data.InClauseGenerator;
 import org.labkey.api.data.JdbcType;
 import org.labkey.api.data.MetadataSqlSelector;
 import org.labkey.api.data.PropertyStorageSpec;
@@ -49,7 +48,6 @@
 import org.labkey.api.data.Table;
 import org.labkey.api.data.TableChange;
 import org.labkey.api.data.TableInfo;
-import org.labkey.api.data.TempTableInClauseGenerator;
 import org.labkey.api.data.TempTableTracker;
 import org.labkey.api.data.dialect.LimitRowsSqlGenerator.LimitRowsCustomizer;
 import org.labkey.api.data.dialect.LimitRowsSqlGenerator.StandardLimitRowsCustomizer;
@@ -92,19 +90,15 @@ public abstract class BasePostgreSqlDialect extends SqlDialect
 {
     // Issue 52190: Expose troubleshooting data that supports postgreSQL-specific analysis
     public static final String POSTGRES_SCHEMA_NAME = "postgres";
-    public static final int TEMPTABLE_GENERATOR_MINSIZE = 1000;
 
     private final Map<String, Integer> _domainScaleMap = new CopyOnWriteHashMap<>();
     private final AtomicBoolean _arraySortFunctionExists = new AtomicBoolean(false);
-    private final InClauseGenerator _tempTableInClauseGenerator = new TempTableInClauseGenerator();
 
     private HtmlString _adminWarning = null;
 
     // Default to 9 and let newer versions be refreshed later
     private int _majorVersion = 9;
 
-    protected InClauseGenerator _inClauseGenerator = null;
-
     // Specifies if this PostgreSQL server treats backslashes in string literals as normal characters (as per the SQL
     // standard) or as escape characters (old, non-standard behavior). As of PostgreSQL 9.1, the setting
     // standard_conforming_strings is on by default; before 9.1, it was off by default. We check the server setting
@@ -289,24 +283,6 @@ public String addReselect(SQLFragment sql, ColumnInfo column, @Nullable String p
         return proposedVariable;
     }
 
-    @Override
-    public SQLFragment appendInClauseSql(SQLFragment sql, @NotNull Collection<?> params)
-    {
-        return appendInClauseSqlWithCustomInClauseGenerator(sql, params, _tempTableInClauseGenerator);
-    }
-
-    @Override
-    public SQLFragment appendInClauseSqlWithCustomInClauseGenerator(SQLFragment sql, @NotNull Collection<?> params, InClauseGenerator tempTableGenerator)
-    {
-        if (params.size() >= TEMPTABLE_GENERATOR_MINSIZE)
-        {
-            SQLFragment ret = tempTableGenerator.appendInClauseSql(sql, params);
-            if (null != ret)
-                return ret;
-        }
-        return _inClauseGenerator.appendInClauseSql(sql, params);
-    }
-
     @Override
     public @NotNull ResultSet executeWithResults(@NotNull PreparedStatement stmt) throws SQLException
     {
@@ -543,12 +519,6 @@ public boolean isSystemSchema(String schemaName)
                 schemaName.startsWith("pg_toast_temp_");
     }
 
-    @Override
-    public String getAnalyzeCommandForTable(String tableName)
-    {
-        return "ANALYZE " + tableName;
-    }
-
     @Override
     protected String getSIDQuery()
     {
@@ -783,7 +753,6 @@ public void prepare(LabKeyDataSource dataSource)
     public String prepare(DbScope scope)
     {
         initializeUserDefinedTypes(scope);
-        initializeInClauseGenerator(scope);
         determineSettings(scope);
         determineIfArraySortFunctionExists(scope);
         return super.prepare(scope);
@@ -835,10 +804,6 @@ private String getDomainKey(String schemaName, String domainName)
         return ("public".equals(schemaName) ? domainName : "\"" + schemaName + "\".\"" + domainName + "\"");
     }
 
-
-    abstract protected void initializeInClauseGenerator(DbScope scope);
-
-
     // Query any settings that may affect dialect behavior. Right now, only "standard_conforming_strings".
     protected void determineSettings(DbScope scope)
     {

api/src/org/labkey/api/data/dialect/DatabaseMaintenanceTask.java

@@ -24,7 +24,7 @@
 import org.labkey.api.util.SystemMaintenance.MaintenanceTask;
 import org.springframework.jdbc.BadSqlGrammarException;
 
-class DatabaseMaintenanceTask implements MaintenanceTask
+public class DatabaseMaintenanceTask implements MaintenanceTask
 {
     @Override
     public String getDescription()
@@ -41,13 +41,17 @@ public String getName()
     @Override
     public void run(Logger log)
     {
-        DbScope scope = DbScope.getLabKeyScope();
+        run(DbScope.getLabKeyScope(), log);
+    }
+
+    public static void run(DbScope scope, Logger log)
+    {
         String url = null;
 
         try
         {
             url = scope.getDataSourceProperties().getUrl();
-            log.info("Database maintenance on " + url + " started");
+            log.info("Database maintenance on {} started", url);
         }
         catch (Exception e)
         {
@@ -69,6 +73,6 @@ public void run(Logger log)
         }
 
         if (null != url)
-            log.info("Database maintenance on " + url + " complete");
+            log.info("Database maintenance on {} complete", url);
     }
 }

api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java

@@ -382,7 +382,7 @@ public void overrideAutoIncrement(StringBuilder statements, TableInfo tinfo)
     }
 
     @Override
-    public String getAnalyzeCommandForTable(String tableName)
+    public SQLFragment getAnalyzeCommandForTable(String tableName)
     {
         throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement");
     }

api/src/org/labkey/api/data/dialect/SqlDialect.java

@@ -102,6 +102,7 @@ public abstract class SqlDialect
 {
     public static final String GENERIC_ERROR_MESSAGE = "The database experienced an unexpected problem. Please check your input and try again.";
     public static final String CUSTOM_UNIQUE_ERROR_MESSAGE = "Constraint violation: cannot insert duplicate value for column";
+    public static final int TEMP_TABLE_GENERATOR_MIN_SIZE = 1000;
 
     protected static final Logger LOG = LogHelper.getLogger(SqlDialect.class, "Database warnings and errors");
     protected static final int MAX_VARCHAR_SIZE = 4000;  //Any length over this will be set to nvarchar(max)/text
@@ -527,16 +528,33 @@ protected Set<String> getJdbcKeywords(SqlExecutor executor) throws SQLException,
 
     private static final InClauseGenerator DEFAULT_GENERATOR = new ParameterMarkerInClauseGenerator();
 
+    public InClauseGenerator getDefaultInClauseGenerator()
+    {
+        return DEFAULT_GENERATOR;
+    }
+
+    // Dialects that support temp-table IN clauses must override this method
+    public InClauseGenerator getTempTableInClauseGenerator()
+    {
+        return null;
+    }
+
     // Most callers should use this method
-    public SQLFragment appendInClauseSql(SQLFragment sql, @NotNull Collection<?> params)
+    public final SQLFragment appendInClauseSql(SQLFragment sql, @NotNull Collection<?> params)
     {
-        return appendInClauseSqlWithCustomInClauseGenerator(sql, params, null);
+        return appendInClauseSqlWithCustomInClauseGenerator(sql, params, getTempTableInClauseGenerator());
     }
 
-    // Use only in cases where the default temp-table generator won't do, e.g., you need to apply a large IN clause in an external data source
-    public SQLFragment appendInClauseSqlWithCustomInClauseGenerator(SQLFragment sql, @NotNull Collection<?> params, InClauseGenerator tempTableGenerator)
+    // Call directly only in cases where the default temp-table generator won't do, e.g., you need to apply a large IN clause in an external data source
+    public final SQLFragment appendInClauseSqlWithCustomInClauseGenerator(SQLFragment sql, @NotNull Collection<?> params, @Nullable InClauseGenerator largeInClauseGenerator)
     {
-        return DEFAULT_GENERATOR.appendInClauseSql(sql, params);
+        if (params.size() >= TEMP_TABLE_GENERATOR_MIN_SIZE && largeInClauseGenerator != null)
+        {
+            SQLFragment ret = largeInClauseGenerator.appendInClauseSql(sql, params);
+            if (null != ret)
+                return ret;
+        }
+        return getDefaultInClauseGenerator().appendInClauseSql(sql, params);
     }
 
     public SQLFragment appendCaseInsensitiveLikeClause(SQLFragment sql, @NotNull String matchStr, @Nullable String wildcardPrefix, @Nullable String wildcardSuffix, char escapeChar)
@@ -1352,7 +1370,7 @@ public String getMessage()
         }
     }
 
-    public abstract String getAnalyzeCommandForTable(String tableName);
+    public abstract SQLFragment getAnalyzeCommandForTable(String tableName);
 
     protected abstract String getSIDQuery();
 
@@ -1370,7 +1388,7 @@ public Integer getSPID(Connection conn) throws SQLException
 
     public boolean updateStatistics(TableInfo table)
     {
-        String sql = getAnalyzeCommandForTable(table.getSelectName());
+        SQLFragment sql = getAnalyzeCommandForTable(table.getSelectName());
         if (sql != null)
         {
             new SqlExecutor(table.getSchema()).execute(sql);

api/src/org/labkey/api/migration/DatabaseMigrationService.java

@@ -74,7 +74,7 @@ static void addDataFilter(String filterName, List<DataFilter> dataFilters, Set<G
         String value = filterParts[1];
         FilterClause clause = CompareType.EQUAL.createFilterClause(new FieldKey(null, column), value);
         // If another container is already using this filter clause, then simply add this guid to that filter.
-        // Otherwise, add a new domain filter to the list.
+        // Otherwise, add a new data filter to the list.
         dataFilters.stream()
             .filter(df -> df.column().equals(column) && df.condition().equals(clause))
             .findFirst()

api/src/org/labkey/api/migration/DefaultMigrationSchemaHandler.java

@@ -275,7 +275,7 @@ protected InClauseGenerator getTempTableInClauseGenerator(DbScope sourceScope)
     }
 
     private static final Set<AttachmentType> SEEN = new HashSet<>();
-    private static final JobRunner ATTACHMENT_JOB_RUNNER = new JobRunner("Attachment JobRunner", 1);
+    private static final JobRunner ATTACHMENT_JOB_RUNNER = new JobRunner("Attachment JobRunner", 1, () -> "Attachments");
 
     // Copy all core.Documents rows that match the provided filter clause
     protected final void copyAttachments(DatabaseMigrationConfiguration configuration, DbSchema sourceSchema, FilterClause filterClause, AttachmentType... type)
@@ -309,12 +309,12 @@ public static void afterMigration() throws InterruptedException
             throw new ConfigurationException("These AttachmentTypes have not been seen: " + unseen.stream().map(type -> type.getClass().getSimpleName()).collect(Collectors.joining(", ")));
 
         // Shut down the attachment JobRunner
-        LOG.info("Waiting for core.Documents background transfer to complete");
+        LOG.info("Waiting for attachments background transfer to complete");
         ATTACHMENT_JOB_RUNNER.shutdown();
         if (ATTACHMENT_JOB_RUNNER.awaitTermination(1, TimeUnit.HOURS))
-            LOG.info("core.Documents background transfer is complete");
+            LOG.info("Attachments background transfer is complete");
         else
-            LOG.error("core.Documents background transfer did not complete after one hour! Giving up.");
+            LOG.error("Attachments background transfer did not complete after one hour! Giving up.");
     }
 
     @Override

api/src/org/labkey/api/util/JobRunner.java

@@ -16,10 +16,11 @@
 
 package org.labkey.api.util;
 
-import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.labkey.api.data.DbScope;
+import org.labkey.api.util.logging.LogHelper;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -30,6 +31,7 @@
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Supplier;
 
 /**
  * This is a simple Executor, that can be used to implement more advanced services
@@ -48,7 +50,7 @@
  */
 public class JobRunner implements Executor
 {
-    static final Logger _log = LogManager.getLogger(JobRunner.class);
+    static final Logger _log = LogHelper.getLogger(JobRunner.class, "JobRunner status and errors");
 
     private static final JobRunner _defaultJobRunner = new JobRunner("Default", 1);
 
@@ -58,14 +60,18 @@ public class JobRunner implements Executor
 
     public JobRunner(String name, int max)
     {
-        this(name, max, Thread.MIN_PRIORITY);
+        this(name, max, null);
     }
 
+    public JobRunner(String name, int max, @Nullable Supplier<String> threadNameFactory)
+    {
+        this(name, max, threadNameFactory, Thread.MIN_PRIORITY);
+    }
 
-    private JobRunner(String name, int max, int priority)
+    private JobRunner(String name, int max, @Nullable Supplier<String> threadNameFactory, int priority)
     {
         _executor = new JobThreadPoolExecutor(max);
-        _executor.setThreadFactory(new JobThreadFactory(priority));
+        _executor.setThreadFactory(new JobThreadFactory(threadNameFactory, priority));
         ContextListener.addShutdownListener(new ShutdownListener()
         {
             @Override
@@ -263,25 +269,26 @@ protected void afterExecute(Runnable r, Throwable t)
     }
 
 
-    static class JobThreadFactory implements ThreadFactory
+    private static class JobThreadFactory implements ThreadFactory
     {
-        static final AtomicInteger poolNumber = new AtomicInteger(1);
-        final ThreadGroup group;
-        final AtomicInteger threadNumber = new AtomicInteger(1);
-        final String namePrefix;
-        final int priority;
+        private static final AtomicInteger POOL_NUMBER = new AtomicInteger(1);
+
+        private final ThreadGroup _group;
+        private final AtomicInteger _threadNumber = new AtomicInteger(1);
+        private final Supplier<String> _threadNameFactory;
+        private final int priority;
 
-        JobThreadFactory(int priority)
+        JobThreadFactory(@Nullable Supplier<String> threadNameFactory, int priority)
         {
-            group = Thread.currentThread().getThreadGroup();
-            namePrefix = "JobThread-" + poolNumber.getAndIncrement() + ".";
+            _group = Thread.currentThread().getThreadGroup();
+            _threadNameFactory = threadNameFactory != null ? threadNameFactory : () -> "JobThread-" + POOL_NUMBER.getAndIncrement() + "." + _threadNumber.getAndIncrement();
             this.priority = priority;
         }
 
         @Override
         public Thread newThread(@NotNull Runnable r)
         {
-            Thread t = new Thread(group, r, namePrefix + threadNumber.getAndIncrement(), 0);
+            Thread t = new Thread(_group, r, _threadNameFactory.get(), 0);
             if (t.isDaemon())
                 t.setDaemon(false);
             if (t.getPriority() != priority)