diff --git a/api/src/org/labkey/api/attachments/LookAndFeelResourceType.java b/api/src/org/labkey/api/attachments/LookAndFeelResourceType.java index 0cf38f92686..4495a645f8d 100644 --- a/api/src/org/labkey/api/attachments/LookAndFeelResourceType.java +++ b/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); diff --git a/api/src/org/labkey/api/data/SimpleFilter.java b/api/src/org/labkey/api/data/SimpleFilter.java index 8dff53f20ff..5393bbe41aa 100644 --- a/api/src/org/labkey/api/data/SimpleFilter.java +++ b/api/src/org/labkey/api/data/SimpleFilter.java @@ -1865,6 +1865,41 @@ private int howManyLessThan(List 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 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 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 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 diff --git a/api/src/org/labkey/api/data/TempTableInClauseGenerator.java b/api/src/org/labkey/api/data/TempTableInClauseGenerator.java index 14276fb551d..3649ea4b7e6 100644 --- a/api/src/org/labkey/api/data/TempTableInClauseGenerator.java +++ b/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 diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index ecdb80e1507..7f367ef89c7 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/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 _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) { diff --git a/api/src/org/labkey/api/data/dialect/DatabaseMaintenanceTask.java b/api/src/org/labkey/api/data/dialect/DatabaseMaintenanceTask.java index a2545f3bf8a..bd446d602ef 100644 --- a/api/src/org/labkey/api/data/dialect/DatabaseMaintenanceTask.java +++ b/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); } } diff --git a/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java b/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java index d597dd10232..73337dd3feb 100644 --- a/api/src/org/labkey/api/data/dialect/SimpleSqlDialect.java +++ b/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"); } diff --git a/api/src/org/labkey/api/data/dialect/SqlDialect.java b/api/src/org/labkey/api/data/dialect/SqlDialect.java index b11a782b34a..cac58227ec7 100644 --- a/api/src/org/labkey/api/data/dialect/SqlDialect.java +++ b/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 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); diff --git a/api/src/org/labkey/api/migration/DefaultMigrationSchemaHandler.java b/api/src/org/labkey/api/migration/DefaultMigrationSchemaHandler.java index e54fe4953a0..36fb4089026 100644 --- a/api/src/org/labkey/api/migration/DefaultMigrationSchemaHandler.java +++ b/api/src/org/labkey/api/migration/DefaultMigrationSchemaHandler.java @@ -275,7 +275,7 @@ protected InClauseGenerator getTempTableInClauseGenerator(DbScope sourceScope) } private static final Set 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 diff --git a/api/src/org/labkey/api/util/JobRunner.java b/api/src/org/labkey/api/util/JobRunner.java index 9b1998dbc58..7b478856fb1 100644 --- a/api/src/org/labkey/api/util/JobRunner.java +++ b/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 threadNameFactory) + { + this(name, max, threadNameFactory, Thread.MIN_PRIORITY); + } - private JobRunner(String name, int max, int priority) + private JobRunner(String name, int max, @Nullable Supplier 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 _threadNameFactory; + private final int priority; - JobThreadFactory(int priority) + JobThreadFactory(@Nullable Supplier 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) diff --git a/core/src/org/labkey/core/CoreMigrationSchemaHandler.java b/core/src/org/labkey/core/CoreMigrationSchemaHandler.java index 2e42998c397..3387000a510 100644 --- a/core/src/org/labkey/core/CoreMigrationSchemaHandler.java +++ b/core/src/org/labkey/core/CoreMigrationSchemaHandler.java @@ -198,10 +198,11 @@ public void copyAttachments(DatabaseMigrationConfiguration configuration, DbSche // Default handling for core's standard attachment types super.copyAttachments(configuration, sourceSchema, targetSchema, copyContainers); - // Special handling for LookAndFeelResourceType, which must select from the source database + // Special handling for LookAndFeelResourceType, which must select from the source database. Keep in sync with + // LookAndFeelResourceType.addWhereSql(). SQLFragment sql = new SQLFragment() .append("Parent").appendInClause(copyContainers, sourceSchema.getSqlDialect()) - .append("AND (DocumentName IN (?, ?) OR ") + .append(" AND (DocumentName IN (?, ?) OR ") .add(AttachmentCache.FAVICON_FILE_NAME) .add(AttachmentCache.STYLESHEET_FILE_NAME) .append("DocumentName LIKE '" + AttachmentCache.LOGO_FILE_NAME_PREFIX + "%' OR ") diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 23391a9f3cb..8f288682bde 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -244,7 +244,6 @@ import org.labkey.core.analytics.AnalyticsServiceImpl; import org.labkey.core.attachment.AttachmentServiceImpl; import org.labkey.core.dialect.PostgreSqlDialectFactory; -import org.labkey.core.dialect.PostgreSqlInClauseTest; import org.labkey.core.dialect.PostgreSqlVersion; import org.labkey.core.junit.JunitController; import org.labkey.core.login.DbLoginAuthenticationProvider; @@ -1440,7 +1439,6 @@ public TabDisplayMode getTabDisplayMode() ModuleStaticResolverImpl.TestCase.class, NotificationServiceImpl.TestCase.class, PortalJUnitTest.class, - PostgreSqlInClauseTest.class, ProductRegistry.TestCase.class, RadeoxRenderer.RadeoxRenderTest.class, RhinoService.TestCase.class, diff --git a/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java b/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java index 8e171ab8689..cb351949890 100644 --- a/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java +++ b/core/src/org/labkey/core/dialect/PostgreSql92Dialect.java @@ -15,13 +15,15 @@ */ package org.labkey.core.dialect; -import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Strings; import org.jetbrains.annotations.NotNull; import org.labkey.api.data.DbScope; +import org.labkey.api.data.InClauseGenerator; import org.labkey.api.data.ParameterMarkerInClauseGenerator; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SqlSelector; import org.labkey.api.data.TableInfo; +import org.labkey.api.data.TempTableInClauseGenerator; import org.labkey.api.data.dialect.BasePostgreSqlDialect; import org.labkey.api.data.dialect.DialectStringHandler; import org.labkey.api.data.dialect.JdbcHelper; @@ -53,6 +55,16 @@ abstract class PostgreSql92Dialect extends BasePostgreSqlDialect // each database. private int _maxIdentifierByteLength = 63; + private InClauseGenerator _inClauseGenerator; + private final TempTableInClauseGenerator _tempTableInClauseGenerator = new TempTableInClauseGenerator(); + + @Override + public String prepare(DbScope scope) + { + initializeInClauseGenerator(scope); + return super.prepare(scope); + } + @NotNull @Override protected Set getReservedWords() @@ -127,19 +139,36 @@ public Collection getScriptWarnings(String name, String sql) // Split statements by semicolon and CRLF for (String statement : noComments.split(";[\\n\\r]+")) { - if (StringUtils.startsWithIgnoreCase(statement.trim(), "SET ")) + if (Strings.CI.startsWith(statement.trim(), "SET ")) warnings.add(statement); } return warnings; } - @Override - protected void initializeInClauseGenerator(DbScope scope) + private void initializeInClauseGenerator(DbScope scope) { _inClauseGenerator = getJdbcVersion(scope) >= 4 ? new ArrayParameterInClauseGenerator(scope) : new ParameterMarkerInClauseGenerator(); } + @Override + public SQLFragment getAnalyzeCommandForTable(String tableName) + { + return new SQLFragment("ANALYZE ").appendIdentifier(tableName); + } + + @Override + public InClauseGenerator getDefaultInClauseGenerator() + { + return _inClauseGenerator; + } + + @Override + public TempTableInClauseGenerator getTempTableInClauseGenerator() + { + return _tempTableInClauseGenerator; + } + @Override public void addAdminWarningMessages(Warnings warnings, boolean showAllWarnings) { diff --git a/core/src/org/labkey/core/dialect/PostgreSqlInClauseTest.java b/core/src/org/labkey/core/dialect/PostgreSqlInClauseTest.java deleted file mode 100644 index adb2da26498..00000000000 --- a/core/src/org/labkey/core/dialect/PostgreSqlInClauseTest.java +++ /dev/null @@ -1,59 +0,0 @@ -package org.labkey.core.dialect; - -import org.junit.Assert; -import org.junit.Test; -import org.labkey.api.data.CoreSchema; -import org.labkey.api.data.DbSchema; -import org.labkey.api.data.SQLFragment; -import org.labkey.api.data.SqlSelector; -import org.labkey.api.data.dialect.BasePostgreSqlDialect; -import org.labkey.api.data.dialect.SqlDialect; - -import java.util.ArrayList; -import java.util.Arrays; - -/* - TestCase migrated from PostgreSql91Dialect when that class promoted to api. - */ -public class PostgreSqlInClauseTest extends Assert -{ - private PostgreSql_11_Dialect getDialect() - { - DbSchema core = CoreSchema.getInstance().getSchema(); - SqlDialect d = core.getSqlDialect(); - if (d instanceof PostgreSql_11_Dialect) - return (PostgreSql_11_Dialect) d; - return null; - } - - @Test - public void testInClause() - { - PostgreSql_11_Dialect d = getDialect(); - if (null == d) - return; - DbSchema core = CoreSchema.getInstance().getSchema(); - - SQLFragment shortSql = new SQLFragment("SELECT COUNT(*) FROM core.usersdata WHERE userid "); - d.appendInClauseSql(shortSql, Arrays.asList(1, 2, 3)); - assertEquals(1, new SqlSelector(core, shortSql).getRowCount()); - - ArrayList l = new ArrayList<>(); - for (int i = 1; i <= BasePostgreSqlDialect.TEMPTABLE_GENERATOR_MINSIZE + 1; i++) - l.add(i); - SQLFragment longSql = new SQLFragment("SELECT COUNT(*) FROM core.usersdata WHERE userid "); - d.appendInClauseSql(longSql, l); - assertEquals(1, new SqlSelector(core, longSql).getRowCount()); - - SQLFragment shortSqlStr = new SQLFragment("SELECT COUNT(*) FROM core.usersdata WHERE displayname "); - d.appendInClauseSql(shortSqlStr, Arrays.asList("1", "2", "3")); - assertEquals(1, new SqlSelector(core, shortSqlStr).getRowCount()); - - l = new ArrayList<>(); - for (int i = 1; i <= BasePostgreSqlDialect.TEMPTABLE_GENERATOR_MINSIZE + 1; i++) - l.add(String.valueOf(i)); - SQLFragment longSqlStr = new SQLFragment("SELECT COUNT(*) FROM core.usersdata WHERE displayname "); - d.appendInClauseSql(longSqlStr, l); - assertEquals(1, new SqlSelector(core, longSqlStr).getRowCount()); - } -} diff --git a/experiment/src/org/labkey/experiment/ExperimentMigrationSchemaHandler.java b/experiment/src/org/labkey/experiment/ExperimentMigrationSchemaHandler.java index 75ade1977e5..3a1bfb1d6bb 100644 --- a/experiment/src/org/labkey/experiment/ExperimentMigrationSchemaHandler.java +++ b/experiment/src/org/labkey/experiment/ExperimentMigrationSchemaHandler.java @@ -5,10 +5,10 @@ import org.jetbrains.annotations.Nullable; import org.labkey.api.attachments.AttachmentType; import org.labkey.api.collections.CsvSet; +import org.labkey.api.data.ColumnInfo; import org.labkey.api.data.CompareType; import org.labkey.api.data.CompareType.CompareClause; import org.labkey.api.data.DbSchema; -import org.labkey.api.data.DbSchemaType; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.SimpleFilter.AndClause; @@ -20,6 +20,7 @@ import org.labkey.api.data.SqlExecutor; import org.labkey.api.data.TableInfo; import org.labkey.api.data.TableSelector; +import org.labkey.api.data.dialect.SqlDialect; import org.labkey.api.exp.OntologyManager; import org.labkey.api.exp.api.ExpProtocolAttachmentType; import org.labkey.api.exp.api.ExpRunAttachmentType; @@ -33,8 +34,8 @@ import org.labkey.experiment.api.ExperimentServiceImpl; import java.util.Collection; -import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Set; class ExperimentMigrationSchemaHandler extends DefaultMigrationSchemaHandler @@ -86,104 +87,134 @@ public FilterClause getTableFilterClause(TableInfo sourceTable, Set contai FilterClause containerClause = getContainerClause(sourceTable, containers); return switch (sourceTable.getName()) { - case "ExperimentRun" -> getExcludedExperimentRunsFilter(sourceTable, containerClause, FieldKey.fromParts("RowId"), false); - case "ProtocolApplication" -> getExcludedExperimentRunsFilter(sourceTable, containerClause, FieldKey.fromParts("RunId"), false); - case "Data", "Edge" -> getExcludedExperimentRunsFilter(sourceTable, containerClause, FieldKey.fromParts("RunId"), true); - case "DataInput", "MaterialInput" -> getExcludedExperimentRunsFilter(sourceTable, containerClause, FieldKey.fromParts("TargetApplicationId", "RunId"), false); - case "RunList" -> getExcludedExperimentRunsFilter(sourceTable, containerClause, FieldKey.fromParts("ExperimentRunId"), false); - case "DataAncestors" -> getExcludedExperimentRunsFilter(sourceTable, containerClause, FieldKey.fromParts("RowId", "RunId"), true); + case "ExperimentRun" -> { + createIncludedExperimentRunRowIdCollection(sourceTable, containerClause); + yield getIncludedExperimentRunFilter(sourceTable, containerClause, FieldKey.fromParts("RowId"), false); + } + case "ProtocolApplication" -> getIncludedExperimentRunFilter(sourceTable, containerClause, FieldKey.fromParts("RunId"), false); + case "Data", "Edge" -> getIncludedExperimentRunFilter(sourceTable, containerClause, FieldKey.fromParts("RunId"), true); + case "DataInput", "MaterialInput" -> getIncludedExperimentRunFilter(sourceTable, containerClause, FieldKey.fromParts("TargetApplicationId", "RunId"), false); + case "RunList" -> getIncludedExperimentRunFilter(sourceTable, containerClause, FieldKey.fromParts("ExperimentRunId"), false); + case "DataAncestors" -> getIncludedExperimentRunFilter(sourceTable, containerClause, FieldKey.fromParts("RowId", "RunId"), true); default -> containerClause; }; } - // Combine the full container clause with the assay experiment run exclusion filter, if present - private FilterClause getExcludedExperimentRunsFilter(TableInfo sourceTable, FilterClause containerClause, FieldKey runIdFieldKey, boolean nullable) + // Combine the full container clause with the assay experiment run inclusion filter, if present + private FilterClause getIncludedExperimentRunFilter(TableInfo sourceTable, FilterClause containerClause, FieldKey runIdFieldKey, boolean nullable) { - FilterClause excludedRowIdClause = getExcludedRowIdClause(sourceTable, runIdFieldKey); + FilterClause includedRowIdClause = getIncludedRowIdClause(sourceTable, runIdFieldKey); - return excludedRowIdClause == null ? + return includedRowIdClause == null ? containerClause : new AndClause( containerClause, nullable ? new OrClause( new CompareClause(runIdFieldKey, CompareType.ISBLANK, null), - excludedRowIdClause + includedRowIdClause ) : - excludedRowIdClause + includedRowIdClause ); } - @Nullable FilterClause getExcludedRowIdClause(TableInfo sourceTable, FieldKey runIdFieldKey) + @Nullable FilterClause getIncludedRowIdClause(TableInfo sourceTable, FieldKey runIdFieldKey) { - Collection experimentRunsToExclude = getExcludedExperimentRunRowIds(sourceTable.getSchema()); + Collection experimentRunsToInclude = getIncludedExperimentRunRowIds(); + + if (null == experimentRunsToInclude) + return null; - return experimentRunsToExclude.isEmpty() ? - null : - new NotClause(new InClause(runIdFieldKey, experimentRunsToExclude, getTempTableInClauseGenerator(sourceTable.getSchema().getScope()))); + return new InClause(runIdFieldKey, experimentRunsToInclude, getTempTableInClauseGenerator(sourceTable.getSchema().getScope())) + { + @Override + public SQLFragment toSQLFragment(Map columnMap, SqlDialect dialect) + { + // Hackfest: turn temp-table IN clause into the equivalent EXISTS() clause because performance on PostgreSQL is so much better + SQLFragment fragment = super.toSQLFragment(columnMap, dialect); + String sql = fragment.getSQL(); + int idx = sql.indexOf("temp.InClause$"); + if (idx > -1) + { + SQLFragment newFragment = new SQLFragment("EXISTS (SELECT 1 FROM ") + .append(sql.substring(idx, idx + 46)) + .append(" WHERE ") + .append(sql.substring(1, sql.indexOf(" IN "))) + .append(" = Id)"); + newFragment.addAll(fragment.getParams()); + newFragment.addTempTokens(fragment); + fragment = newFragment; + } + return fragment; + } + }; } - private Collection _excludedExperimentRunRowIds = null; + // Collection of all exp.ExperimentRun RowIds in all copy containers that should be copied. If an AssaySkipFilter is + // provided, this collection excludes assay runs in those containers plus runs that list one of the excluded runs + // as a replacement, etc. A null value means include all experiment runs in all copy containers. + private Collection _includedExperimentRunRowIds = null; - private @NotNull Collection getExcludedExperimentRunRowIds(DbSchema schema) + private @Nullable Collection getIncludedExperimentRunRowIds() { - if (null == _excludedExperimentRunRowIds) + return _includedExperimentRunRowIds; + } + + private void createIncludedExperimentRunRowIdCollection(TableInfo sourceExperimentRunsTable, FilterClause containerClause) + { + if (AssaySkipContainers.getContainers().isEmpty()) { - if (AssaySkipContainers.getContainers().isEmpty()) - { - _excludedExperimentRunRowIds = Collections.emptyList(); - } - else - { - // We need the source exp schema; if it wasn't passed in, retrieve it from the scope. - DbSchema expSchema = "exp".equals(schema.getName()) ? schema : schema.getScope().getSchema("exp", DbSchemaType.Migration); + _includedExperimentRunRowIds = null; + } + else + { + DbSchema sourceSchema = sourceExperimentRunsTable.getSchema(); - // Select all the assay runs (same filter used by assay.AssayRuns) - SQLFragment assayRunSql = new SQLFragment( - "ProtocolLSID IN (SELECT LSID FROM exp.Protocol x WHERE (ApplicationType = 'ExperimentRun') AND " + - "((SELECT MAX(pd.PropertyId) from exp.Object o, exp.ObjectProperty op, exp.PropertyDescriptor pd WHERE " + - "pd.PropertyId = op.PropertyId and op.ObjectId = o.ObjectId and o.ObjectURI = LSID AND pd.PropertyURI LIKE '%AssayDomain-Run%') IS NOT NULL))" - ); + // Selects all assay runs (same filter used by assay.AssayRuns) + SQLFragment assayRunSql = new SQLFragment( + "ProtocolLSID IN (SELECT LSID FROM exp.Protocol x WHERE (ApplicationType = 'ExperimentRun') AND " + + "((SELECT MAX(pd.PropertyId) from exp.Object o, exp.ObjectProperty op, exp.PropertyDescriptor pd WHERE " + + "pd.PropertyId = op.PropertyId and op.ObjectId = o.ObjectId and o.ObjectURI = LSID AND pd.PropertyURI LIKE '%AssayDomain-Run%') IS NOT NULL))" + ); - // Select all assay runs in the assay-skip containers - FilterClause assayRunClause = new AndClause( - new InClause(FieldKey.fromParts("Container"), AssaySkipContainers.getContainers()), - new SQLClause(assayRunSql) - ); + // Selects all assay runs in the configured assay-skip containers + FilterClause assayRunClause = new AndClause( + new InClause(FieldKey.fromParts("Container"), AssaySkipContainers.getContainers()), + new SQLClause(assayRunSql) + ); - // Select assay runs (regardless of their container) that were replaced by assay runs that are being - // excluded - FilterClause replaceByRunIdClause = new SQLClause( - new SQLFragment("ReplacedByRunId IN (SELECT RowId FROM exp.ExperimentRun WHERE ") - .append(assayRunClause.toSQLFragment(null, expSchema.getSqlDialect())) - .append(")") - ); + // Selects assay runs (regardless of their container) that were replaced by assay runs that are being + // excluded + FilterClause replaceByRunIdClause = new SQLClause( + new SQLFragment("ReplacedByRunId IS NOT NULL AND ReplacedByRunId IN (SELECT RowId FROM exp.ExperimentRun WHERE ") + .append(assayRunClause.toSQLFragment(null, sourceSchema.getSqlDialect())) + .append(")") + ); - // Select assay runs that were replaced by assay runs that are being excluded because they were replaced - // by an excluded assay run. Yes, we actually have to do this... - FilterClause replaceByReplacedRunIdClause = new SQLClause( - new SQLFragment("ReplacedByRunId IN (SELECT RowId FROM exp.ExperimentRun WHERE ") - .append(replaceByRunIdClause.toSQLFragment(null, expSchema.getSqlDialect())) - .append(")") - ); + // Selects assay runs that were replaced by assay runs that are being excluded because they were replaced + // by an excluded assay run. Yes, we actually have to do this... + FilterClause replaceByReplacedRunIdClause = new SQLClause( + new SQLFragment("ReplacedByRunId IS NOT NULL AND ReplacedByRunId IN (SELECT RowId FROM exp.ExperimentRun WHERE ") + .append(replaceByRunIdClause.toSQLFragment(null, sourceSchema.getSqlDialect())) + .append(")") + ); - // Select all assay runs that need to be excluded - SimpleFilter filter = new SimpleFilter( - new OrClause( - assayRunClause, - replaceByRunIdClause, - replaceByReplacedRunIdClause - ) - ); + // Selects all assay runs that need to be included -- all rows in exp.ExperimentRuns in all copy containers, + // except assay runs in the assay-skip containers or runs replaced by one of those excluded runs. + SimpleFilter filter = new SimpleFilter( + containerClause, + new NotClause(new OrClause( + assayRunClause, + replaceByRunIdClause, + replaceByReplacedRunIdClause + )) + ); - // Select the excluded assay experiment run RowIds. All tables with FKs to ExperimentRun (or FKs to - // other tables with FKs to ExperimentRun) must exclude these run IDs. - _excludedExperimentRunRowIds = new TableSelector(expSchema.getTable("ExperimentRun"), new CsvSet("RowId, ProtocolLSID, ReplacedByRunId"), filter, null).getCollection(Integer.class); - LOG.info(" {} being excluded due to the configured AssaySkipContainers parameter", StringUtilsLabKey.pluralize(_excludedExperimentRunRowIds.size(), "assay experiment run is", "assay experiment runs are")); - } + // Select the experiment run RowIds to transfer. All tables with FKs to ExperimentRun (or FKs to other + // tables with FKs to ExperimentRun) must add these run IDs as an include filter to avoid FK violations. + _includedExperimentRunRowIds = new TableSelector(sourceExperimentRunsTable, new CsvSet("RowId, ProtocolLSID, ReplacedByRunId"), filter, null).getCollection(Integer.class); + LOG.info(" {} being included due to the configured AssaySkipContainers parameter", StringUtilsLabKey.pluralize(_includedExperimentRunRowIds.size(), "assay experiment run is", "assay experiment runs are")); } - - return _excludedExperimentRunRowIds; } @Override diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 592f80e9d7b..333f1f28b7c 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/experiment/src/org/labkey/experiment/ExperimentModule.java @@ -890,10 +890,10 @@ public TableInfo getTableInfo() @Override public void adjustFilter(TableInfo sourceTable, SimpleFilter filter, Set containers) { - // Exclude assay experiment runs that weren't copied - FilterClause excludedClause = handler.getExcludedRowIdClause(sourceTable, FieldKey.fromParts("RunId")); - if (excludedClause != null) - filter.addClause(excludedClause); + // Include experiment runs that were copied + FilterClause includedClause = handler.getIncludedRowIdClause(sourceTable, FieldKey.fromParts("RunId")); + if (includedClause != null) + filter.addClause(includedClause); } }); DatabaseMigrationService.get().registerTableHandler(new MigrationTableHandler() @@ -907,10 +907,10 @@ public TableInfo getTableInfo() @Override public void adjustFilter(TableInfo sourceTable, SimpleFilter filter, Set containers) { - // Exclude assay experiment runs that weren't copied - FilterClause excludedClause = handler.getExcludedRowIdClause(sourceTable, FieldKey.fromParts("ExclusionId", "RunId")); - if (excludedClause != null) - filter.addClause(excludedClause); + // Include experiment runs that were copied + FilterClause includedClause = handler.getIncludedRowIdClause(sourceTable, FieldKey.fromParts("ExclusionId", "RunId")); + if (includedClause != null) + filter.addClause(includedClause); } }); DatabaseMigrationService.get().registerTableHandler(new MigrationTableHandler() @@ -924,10 +924,10 @@ public TableInfo getTableInfo() @Override public void adjustFilter(TableInfo sourceTable, SimpleFilter filter, Set containers) { - // Exclude assay experiment runs that weren't copied - FilterClause excludedClause = handler.getExcludedRowIdClause(sourceTable, FieldKey.fromParts("RunId")); - if (excludedClause != null) - filter.addClause(excludedClause); + // Include experiment runs that were copied + FilterClause includedClause = handler.getIncludedRowIdClause(sourceTable, FieldKey.fromParts("RunId")); + if (includedClause != null) + filter.addClause(includedClause); } }); DatabaseMigrationService.get().registerSchemaHandler(new SampleTypeMigrationSchemaHandler());