From 928f22eba22daefed6e102e1e4497ab229212f92 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 1 Sep 2026 22:10:56 -0700 Subject: [PATCH 01/10] Stop indexing materialized table on LSID and propagate custom indices --- .../experiment/api/ExpMaterialTableImpl.java | 72 ++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 29b7b3768ca..54c6bea2902 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -52,6 +52,7 @@ import org.labkey.api.data.MaterializedQueryHelper; import org.labkey.api.data.MutableColumnInfo; import org.labkey.api.data.PHI; +import org.labkey.api.data.PropertyStorageSpec; import org.labkey.api.data.RenderContext; import org.labkey.api.data.SQLFragment; import org.labkey.api.data.Sort; @@ -88,6 +89,7 @@ import org.labkey.api.exp.query.ExpSchema; import org.labkey.api.exp.query.SamplesSchema; import org.labkey.api.gwt.client.AuditBehaviorType; +import org.labkey.api.gwt.client.model.GWTIndex; import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; import org.labkey.api.gwt.client.model.PropertyValidatorType; import org.labkey.api.inventory.InventoryService; @@ -1365,10 +1367,11 @@ private _MaterializedQueryHelper getOrCreateMQH() .updateColumns(updateColumns) .unlogged(true) .addIndex("CREATE UNIQUE INDEX uq_${NAME}_rowid ON temp.${NAME} (rowid)") - .addIndex("CREATE UNIQUE INDEX uq_${NAME}_lsid ON temp.${NAME} (lsid)") .addIndex("CREATE INDEX idx_${NAME}_container ON temp.${NAME} (container)") .addIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)"); + getDomainIndexDdl().forEach(builder::addIndex); + if (isIncrementalUpdateDisabled()) builder.addInvalidCheck(() -> String.valueOf(getInvalidateCounters(_ss.getLSID()).update.get())); @@ -1376,6 +1379,50 @@ private _MaterializedQueryHelper getOrCreateMQH() }); } + /** + * DDL mirroring the sample type domain's admin-defined indices onto the materialized table. Never unique: + * {@link #getJoinSQL} selects root-scoped columns from the root sample's row, so a value that is unique in the + * provisioned table repeats across every aliquot sharing that root. + */ + private List getDomainIndexDdl() + { + TableInfo provisioned = _ss.getTinfo(); + if (null == provisioned) + return List.of(); + + Domain domain = _ss.getDomain(); + + List ddl = new ArrayList<>(); + Set distinctColumnLists = new HashSet<>(); + + for (PropertyStorageSpec.Index index : domain.getPropertyIndices()) + { + List columns = new ArrayList<>(); + + for (String storageName : index.translateToStorageNames(domain).columnNames) + { + ColumnInfo column = provisioned.getColumn(storageName); + + if (null == column) + { + _log.warn("Sample type {} is indexed on {}, which is not in its provisioned table; that index is not mirrored onto the materialized table.", _ss.getName(), storageName); + columns.clear(); + break; + } + + columns.add(column.getSelectIdentifier().getSql().getSQL()); + } + + if (columns.isEmpty() || !distinctColumnLists.add(String.join(",", columns))) + continue; + + // Ordinal names because ${NAME} is already 33 of the 63 chars Postgres allows before it silently truncates and collides. + ddl.add("CREATE INDEX idx_${NAME}_d" + ddl.size() + " ON temp.${NAME} (" + String.join(", ", columns) + ")"); + } + + return ddl; + } + /* SELECT and JOIN, does not include WHERE, same as getJoinSQL() */ private @Nullable SQLFragment getMaterializedSQL() { @@ -2259,7 +2306,28 @@ public void testMergeInsertAndUpdate() throws Exception assertCacheMatchesFreshDerivation(table, st.getLSID()); } + @Test + public void testDomainIndexMirroredNonUnique() throws Exception + { + ExpSampleType st = createSampleType("IncrUpdIndexed", List.of(new GWTIndex(List.of("rootProp"), true))); + insertRoots(st, "R1"); + insertAliquots(st, "R1", 3); + + ExpMaterialTableImpl table = getSamplesTable(st); + List ddl = table.getDomainIndexDdl(); + assertEquals("Expected the domain's one index to be mirrored: " + ddl, 1, ddl.size()); + assertFalse("Mirrored index must not be unique: " + ddl.getFirst(), ddl.getFirst().contains("UNIQUE")); + + // All four rows share the root's rootProp, so mirroring the domain's unique index as-is would fail the build. + assertCacheMatchesFreshDerivation(table, st.getLSID()); + } + private ExpSampleType createSampleType(String name) throws Exception + { + return createSampleType(name, Collections.emptyList()); + } + + private ExpSampleType createSampleType(String name, List indices) throws Exception { List props = new ArrayList<>(); props.add(new GWTPropertyDescriptor("name", "string")); @@ -2269,7 +2337,7 @@ private ExpSampleType createSampleType(String name) throws Exception GWTPropertyDescriptor aliquotProp = new GWTPropertyDescriptor("aliquotProp", "string"); aliquotProp.setDerivationDataScope(ExpSchema.DerivationDataScopeType.ChildOnly.name()); // -> m_aliquot join props.add(aliquotProp); - return SampleTypeService.get().createSampleType(_c, _user, name, null, props, Collections.emptyList(), -1, -1, -1, -1, null); + return SampleTypeService.get().createSampleType(_c, _user, name, null, props, indices, -1, -1, -1, -1, null); } private ExpMaterialTableImpl getSamplesTable(ExpSampleType st) From 96447776dc84a1a4109deaf4651c456e5014efe0 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 2 Sep 2026 21:57:08 -0700 Subject: [PATCH 02/10] Reduce per-event cost of audit writes - Audit inserts no longer rebuild the audit table's metadata on every event - Provider-generated audit events raised during a bulk operation are batched instead of inserted one at a time - List inserts, updates, and deletes write their audit rows per operation rather than per row, and stop re-resolving column names for every row - Sample timeline audit writes are chunked so a large sample operation doesn't hold every event in memory - Fixes lookup value resolution being re-queried on every row during import, where it should have been memoized --- .../api/audit/AbstractAuditHandler.java | 35 +++++- .../api/audit/AbstractAuditTypeProvider.java | 27 +++++ .../labkey/api/audit/AuditTypeProvider.java | 9 ++ .../api/dataiterator/SimpleTranslator.java | 113 ++++++++++++++++-- .../labkey/api/exp/api/SampleTypeService.java | 4 + .../api/exp/api/StorageProvisioner.java | 1 + 6 files changed, 179 insertions(+), 10 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditHandler.java b/api/src/org/labkey/api/audit/AbstractAuditHandler.java index 801ba46957b..bfeac495cf0 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditHandler.java +++ b/api/src/org/labkey/api/audit/AbstractAuditHandler.java @@ -26,13 +26,18 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static org.labkey.api.gwt.client.AuditBehaviorType.SUMMARY; public abstract class AbstractAuditHandler implements AuditHandler { + /** Bounds both the JDBC batch and the events held in memory while a batch accumulates. */ + public static final int AUDIT_BATCH_SIZE = 2500; + protected abstract AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row); @Override @@ -72,6 +77,15 @@ public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryS */ protected abstract DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues); + /** + * Overload for providers whose record construction produces audit events of its own. Events added to + * sideEffectEvents are batched by addAuditEvent() rather than inserted one per row. + */ + protected DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues, List sideEffectEvents) + { + return createDetailedAuditRecord(user, c, tInfo, action, userComment, row, existingRow, providedValues); + } + /** * Allow for adding fields that may be present in the updated row but not represented in the original row * @@ -130,13 +144,14 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud AuditLogService auditLog = AuditLogService.get(); List batch = new ArrayList<>(); + List sideEffectEvents = new ArrayList<>(); for (int i=0; i < rows.size(); i++) { Map row = rows.get(i); Map existingRow = null == existingRows ? Collections.emptyMap() : existingRows.get(i); Map providedValueRow = null == providedValues || providedValues.size() <= i ? null : providedValues.get(i); - DetailedAuditTypeEvent event = createDetailedAuditRecord(user, c, auditConfigurable, action, userComment, row, existingRow, providedValueRow); + DetailedAuditTypeEvent event = createDetailedAuditRecord(user, c, auditConfigurable, action, userComment, row, existingRow, providedValueRow, sideEffectEvents); switch (action) { @@ -175,23 +190,39 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud } } batch.add(event); - if (batch.size() > 1000) + if (batch.size() >= AUDIT_BATCH_SIZE) { auditLog.addEvents(user, batch, useTransactionAuditCache); batch.clear(); } + // a row can contribute more than one side effect, so bound these separately from the row batch + if (sideEffectEvents.size() >= AUDIT_BATCH_SIZE) + { + addSideEffectEvents(auditLog, user, sideEffectEvents, useTransactionAuditCache); + sideEffectEvents.clear(); + } } if (!batch.isEmpty()) { auditLog.addEvents(user, batch, useTransactionAuditCache); batch.clear(); } + addSideEffectEvents(auditLog, user, sideEffectEvents, useTransactionAuditCache); break; } } } } + /** insertEvents() batches only a fully homogeneous list. Group by event type and container. Each type is stored its own provisioned table */ + private void addSideEffectEvents(AuditLogService auditLog, User user, List events, boolean useTransactionAuditCache) + { + events.stream() + .collect(Collectors.groupingBy(event -> Pair.of(event.getEventType(), event.getContainer()), LinkedHashMap::new, Collectors.toList())) + .values() + .forEach(group -> auditLog.addEvents(user, group, useTransactionAuditCache)); + } + private void setOldAndNewMapsForUpdate(DetailedAuditTypeEvent event, Container c, Map row, Map existingRow, TableInfo table) { Pair, Map> rowPair = AuditHandler.getOldAndNewRecordForMerge(row, existingRow, table.getExtraDetailedUpdateAuditFields(), table.getExcludedDetailedUpdateAuditFields(), table); diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index d749b2c1b5f..1e0de7f467e 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -31,6 +31,7 @@ import org.labkey.api.data.DbScope; import org.labkey.api.data.MultiChoice; import org.labkey.api.data.MutableColumnInfo; +import org.labkey.api.data.SchemaTableInfo; import org.labkey.api.data.Table; import org.labkey.api.data.TableInfo; import org.labkey.api.dataiterator.DataIterator; @@ -85,6 +86,9 @@ public abstract class AbstractAuditTypeProvider implements AuditTypeProvider private final AbstractAuditDomainKind _domainKind; + private record CachedStorageTable(SchemaTableInfo schemaTableInfo, TableInfo storageTableInfo) {} + private volatile CachedStorageTable _cachedStorageTable; + public AbstractAuditTypeProvider(@NotNull AbstractAuditDomainKind domainKind) { // TODO: consolidate domain kind initialization to this constructor and stop overriding getDomainKind() @@ -266,6 +270,29 @@ public TableInfo createStorageTableInfo() return StorageProvisioner.createTableInfo(domain); } + @Override @NotNull + public TableInfo getStorageTableInfoForInsert() + { + Domain domain = getDomain(); + if (null == domain) + throw new IllegalStateException("Could not find domain for audit event type " + getEventName()); + + // We want to reuse a cached provisioned TableInfo to avoid construction costs. Getting the SchemaTableInfo is + // cheap so use that as a guide for when the table has changed (primarily during startup as its shape may + // need to be updated based on current code expections) and when it's safe to reuse the previous copy. + SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); + CachedStorageTable cached = _cachedStorageTable; + if (null != cached && cached.schemaTableInfo() == schemaTableInfo) + return cached.storageTableInfo(); + + TableInfo storageTableInfo = StorageProvisioner.createTableInfo(domain); + // Shared across threads from here on + storageTableInfo.setLocked(true); + _cachedStorageTable = new CachedStorageTable(schemaTableInfo, storageTableInfo); + + return storageTableInfo; + } + @Override public TableInfo createTableInfo(UserSchema userSchema, ContainerFilter cf) { diff --git a/api/src/org/labkey/api/audit/AuditTypeProvider.java b/api/src/org/labkey/api/audit/AuditTypeProvider.java index 8cf1711c363..26df2504583 100644 --- a/api/src/org/labkey/api/audit/AuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AuditTypeProvider.java @@ -15,6 +15,7 @@ */ package org.labkey.api.audit; +import org.jetbrains.annotations.NotNull; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.TableInfo; import org.labkey.api.exp.property.Domain; @@ -43,6 +44,14 @@ public interface AuditTypeProvider TableInfo createTableInfo(UserSchema schema, ContainerFilter cf); + /** + * Provisioned storage TableInfo for the insert path. Implementations should cache - creating TableInfos is expensive. + * Never use this to read audit rows: it has no ContainerFilter and bypasses the CanSeeAuditLog check that + * {@link org.labkey.api.audit.query.DefaultAuditTypeTable} applies. Go through {@link #createTableInfo} for reads. + */ + @NotNull + TableInfo getStorageTableInfoForInsert(); + Class getEventClass(); ActionURL getAuditUrl(); diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 3d3fe1528bf..19abc80dcbd 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -220,7 +220,6 @@ public RemapConverter(@NotNull TableInfo targetTable, boolean includeTitleColumn public void setIncludePkLookup(boolean includePkLookup) { _includePkLookup = includePkLookup; - _maps = null; } public ColumnInfo getPkColumn() @@ -228,6 +227,17 @@ public ColumnInfo getPkColumn() return _targetTable.getPkColumns().getFirst(); } + private Pair> pkLookupMap() + { + if (!_includePkLookup) + return null; + + if (_pkColumnLookupMap == null) + _pkColumnLookupMap = Pair.of(getPkColumn(), new HashMap<>()); + + return _pkColumnLookupMap; + } + private List>> getMaps() { if (_maps == null) @@ -271,11 +281,6 @@ public ColumnInfo getPkColumn() _titleColumnLookupMap = Triple.of(pkCol, titleColumn, new ArrayListValuedHashMap()); } } - - if (_includePkLookup) - { - _pkColumnLookupMap = Pair.of(pkCol, new HashMap<>()); - } } return _maps; } @@ -291,9 +296,10 @@ public Object mappedValue(Object k) List>> maps = getMaps(); - if (_pkColumnLookupMap != null) + Pair> pkLookupMap = pkLookupMap(); + if (pkLookupMap != null) { - Object v = fetch(_pkColumnLookupMap, k); + Object v = fetch(pkLookupMap, k); if (v != null) return v; } @@ -2233,6 +2239,97 @@ public void convertRemapTest() throws Exception } + /** Lookup fixture with one text alternate key (Value) over an integer pk (RowId); Ordinal is unique but not text, so it yields no map. */ + private EnumTableInfo remapLookupTable() + { + var core = QueryService.get().getUserSchema(TestContext.get().getUser(), JunitUtil.getTestContainer(), "core"); + return new EnumTableInfo<>(LookupValues.class, core, "fake enum", true); + } + + @Test + public void remapMemoizationSurvivesPkLookupToggle() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + + // RemappingConvertColumn flips this before every row, so it must not discard what earlier rows resolved + converter.setIncludePkLookup(false); + + List>> maps = converter.getMaps(); + assertEquals("expected one alternate-key map, on the Value column", 1, maps.size()); + + // Seed keys no enum value can supply, so anything but a memo hit resolves to null + MultiValuedMap memo = maps.getFirst().getRight(); + Integer seeded = 42; + memo.put("seeded-hit", seeded); + memo.put("seeded-miss", converter.MISS); + + assertEquals(seeded, converter.mappedValue("seeded-hit")); + assertNull(converter.mappedValue("seeded-miss")); + + for (int i = 0; i < 3; i++) + { + converter.setIncludePkLookup(true); + converter.setIncludePkLookup(false); + } + + assertSame("toggling includePkLookup discarded the memoization maps", maps, converter.getMaps()); + assertEquals("resolved value was discarded, so every row re-queries it", seeded, converter.mappedValue("seeded-hit")); + assertNull("MISS marker was discarded, so every row re-queries the absent value", converter.mappedValue("seeded-miss")); + } + + @Test + public void remapResolutionIsStableAcrossPkLookupToggle() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + converter.setIncludePkLookup(false); + + Object resolved = converter.mappedValue(LookupValues.Two.name()); + assertNotNull("expected " + LookupValues.Two + " to resolve by alternate key", resolved); + + converter.setIncludePkLookup(true); + converter.setIncludePkLookup(false); + + assertEquals(resolved, converter.mappedValue(LookupValues.Two.name())); + } + + @Test + public void remapAlternateKeyWinsWhenPkLookupIsOff() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); + + // Seed the two maps to disagree on one key, so the resolved value says which map was consulted + Integer key = 7; + Integer pkResolution = 7; + Integer akResolution = 99; + Map pkMemo = converter.pkLookupMap().getValue(); + MultiValuedMap akMemo = converter.getMaps().getFirst().getRight(); + pkMemo.put(key, pkResolution); + akMemo.put(key, akResolution); + + assertEquals("pk lookup should take precedence while includePkLookup is on", pkResolution, converter.mappedValue(key)); + + // The pk map survives the toggle, so this also pins that it is not consulted while the flag is off + converter.setIncludePkLookup(false); + assertEquals("alternate key should resolve while includePkLookup is off", akResolution, converter.mappedValue(key)); + } + + @Test + public void remapPkLookupMapIsRetained() + { + RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, false); + assertNull("pk lookup map should not exist while includePkLookup is off", converter.pkLookupMap()); + + converter.setIncludePkLookup(true); + Pair> pkMap = converter.pkLookupMap(); + assertNotNull(pkMap); + + converter.setIncludePkLookup(false); + assertNull(converter.pkLookupMap()); + + converter.setIncludePkLookup(true); + assertSame("pk lookup map was rebuilt rather than retained", pkMap, converter.pkLookupMap()); + } + @Test public void getFileRootSubstitutedFilePathTest() { diff --git a/api/src/org/labkey/api/exp/api/SampleTypeService.java b/api/src/org/labkey/api/exp/api/SampleTypeService.java index cd12c102669..e2ddd82f266 100644 --- a/api/src/org/labkey/api/exp/api/SampleTypeService.java +++ b/api/src/org/labkey/api/exp/api/SampleTypeService.java @@ -18,6 +18,7 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.labkey.api.audit.SampleTimelineAuditEvent; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; import org.labkey.api.data.DbSequence; @@ -249,6 +250,9 @@ default Map incrementSampleCounts(@Nullable Date counterDate) void addAuditEvents(User user, Container container, String comment, String userComment, Collection samples, Map metadata); + /** Builds the same event addAuditEvent() would write, for callers on row-scaling paths that need to batch the inserts themselves. */ + SampleTimelineAuditEvent createTimelineAuditRecord(Container container, String comment, String userComment, ExpMaterial sample, Map metadata, String updateType); + // find the max sequence number with '${sampleName}-' prefix long getMaxAliquotId(@NotNull String sampleName, @NotNull String sampleTypeLsid, Container container); diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index 1c0494ef7b7..24282846971 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -86,6 +86,7 @@ static TableInfo createTableInfo(@NotNull Domain domain) void ensureTableIndices(@NotNull Domain domain); void ensureTableIndices(@NotNull Domain domain, Supplier afterAddSupplier); + @NotNull SchemaTableInfo getSchemaTableInfo(Domain domain); /** From 68fd33bc761e57b53778dc456aed2fc8cd3bf437 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Wed, 2 Sep 2026 21:57:58 -0700 Subject: [PATCH 03/10] Reduce per-event cost of audit writes - Audit inserts no longer rebuild the audit table's metadata on every event - Provider-generated audit events raised during a bulk operation are batched instead of inserted one at a time - List inserts, updates, and deletes write their audit rows per operation rather than per row, and stop re-resolving column names for every row - Sample timeline audit writes are chunked so a large sample operation doesn't hold every event in memory - Fixes lookup value resolution being re-queried on every row during import, where it should have been memoized --- .../org/labkey/audit/model/LogManager.java | 57 ++-- .../experiment/api/ExpMaterialTableImpl.java | 41 ++- .../experiment/api/SampleTypeServiceImpl.java | 28 +- .../api/property/StorageProvisionerImpl.java | 2 +- list/src/org/labkey/list/ListModule.java | 9 + .../org/labkey/list/model/ListManager.java | 128 +++++---- .../list/model/ListQueryUpdateService.java | 248 ++++++++++++++++-- 7 files changed, 394 insertions(+), 119 deletions(-) diff --git a/audit/src/org/labkey/audit/model/LogManager.java b/audit/src/org/labkey/audit/model/LogManager.java index 5349ade7eb8..6a4c120b23c 100644 --- a/audit/src/org/labkey/audit/model/LogManager.java +++ b/audit/src/org/labkey/audit/model/LogManager.java @@ -22,11 +22,9 @@ import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.AuditTypeEvent; import org.labkey.api.audit.AuditTypeProvider; -import org.labkey.api.audit.query.DefaultAuditTypeTable; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerFilter; -import org.labkey.api.data.ContainerManager; import org.labkey.api.data.DbSchema; import org.labkey.api.data.ObjectFactory; import org.labkey.api.data.ParameterMapStatement; @@ -81,23 +79,9 @@ public K insertEvent(User user, K type) if (provider != null) { - Container c = type.getContainer(); - - UserSchema schema = AuditLogService.getAuditLogSchema(user, c != null ? c : ContainerManager.getRoot()); - - if (schema != null) - { - TableInfo table = schema.getTable(provider.getEventName(), false); - - if (table instanceof DefaultAuditTypeTable auditTypeTable) - { - // consider using etl data iterator for inserts - type = validateFields(provider, type); - TableInfo dbTable = auditTypeTable.getRealTable(); - K ret = Table.insert(user, dbTable, type); - return ret; - } - } + // consider using etl data iterator for inserts + type = validateFields(provider, type); + return Table.insert(user, provider.getStorageTableInfoForInsert(), type); } return null; } @@ -142,33 +126,28 @@ public void insertEvents(User user, List events) if (null == provider) return; Container c = type.getContainer(); - UserSchema schema = AuditLogService.getAuditLogSchema(user, c != null ? c : ContainerManager.getRoot()); - TableInfo table = null==schema ? null : schema.getTable(provider.getEventName(), false); - TableInfo dbTable = table instanceof DefaultAuditTypeTable auditTypeTable ? auditTypeTable.getRealTable() : null; + TableInfo dbTable = provider.getStorageTableInfoForInsert(); Logger auditLogger = getAuditLogger(type); SQLException sqlx = null; - if (null != dbTable) + try (Connection conn = dbTable.getSchema().getScope().getConnection()) { - try (Connection conn = dbTable.getSchema().getScope().getConnection()) - { - ParameterMapStatement stmt = StatementUtils.insertStatement(conn, dbTable, c, user, false, true); - for (var event : events) - { - event = validateFields(provider, event); - Map map = ObjectFactory.Registry.getFactory((Class)event.getClass()).toMap(event, null); - stmt.clearParameters(); - stmt.putAll(map); - stmt.addBatch(); - } - stmt.executeBatch(); - } - catch (SQLException x) + ParameterMapStatement stmt = StatementUtils.insertStatement(conn, dbTable, c, user, false, true); + for (var event : events) { - auditLogger.warn("Error occurred saving audit entries to database"); - sqlx = x; + event = validateFields(provider, event); + Map map = ObjectFactory.Registry.getFactory((Class)event.getClass()).toMap(event, null); + stmt.clearParameters(); + stmt.putAll(map); + stmt.addBatch(); } + stmt.executeBatch(); + } + catch (SQLException x) + { + auditLogger.warn("Error occurred saving audit entries to database"); + sqlx = x; } if (auditLogger.isInfoEnabled()) diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 54c6bea2902..97810e1d4e8 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -79,6 +79,7 @@ import org.labkey.api.exp.api.StorageProvisioner; import org.labkey.api.exp.property.DefaultPropertyValidator; import org.labkey.api.exp.property.Domain; +import org.labkey.api.exp.property.DomainKind; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.exp.property.DomainUtil; import org.labkey.api.exp.property.IPropertyValidator; @@ -148,6 +149,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.TreeSet; @@ -1380,7 +1382,9 @@ private _MaterializedQueryHelper getOrCreateMQH() } /** - * DDL mirroring the sample type domain's admin-defined indices onto the materialized table. Never unique: + * DDL mirroring the sample type domain's admin-defined indices onto the materialized table. Read from the + * provisioned table's own indices, not {@link Domain#getPropertyIndices()} -- that set is only ever populated by + * the caller that is saving a domain, so it is empty on a domain read back from the database. Never unique: * {@link #getJoinSQL} selects root-scoped columns from the root sample's row, so a value that is unique in the * provisioned table repeats across every aliquot sharing that root. */ @@ -1391,38 +1395,55 @@ private List getDomainIndexDdl() return List.of(); Domain domain = _ss.getDomain(); + DomainKind kind = domain.getDomainKind(); + if (null == kind) + return List.of(); List ddl = new ArrayList<>(); Set distinctColumnLists = new HashSet<>(); - for (PropertyStorageSpec.Index index : domain.getPropertyIndices()) + // Seeded with the kind's own indices (rowid, name); the materialized table already indexes those. + for (PropertyStorageSpec.Index index : kind.getPropertyIndices(domain)) + distinctColumnLists.add(indexKey(Arrays.asList(index.translateToStorageNames(domain).columnNames))); + + for (TableInfo.IndexDefinition index : StorageProvisioner.get().getSchemaTableInfo(domain).getAllIndices()) { - List columns = new ArrayList<>(); + if (TableInfo.IndexType.Primary == index.indexType()) + continue; + + List names = new ArrayList<>(); + List identifiers = new ArrayList<>(); - for (String storageName : index.translateToStorageNames(domain).columnNames) + for (ColumnInfo indexColumn : index.columns()) { - ColumnInfo column = provisioned.getColumn(storageName); + ColumnInfo column = provisioned.getColumn(indexColumn.getName()); if (null == column) { - _log.warn("Sample type {} is indexed on {}, which is not in its provisioned table; that index is not mirrored onto the materialized table.", _ss.getName(), storageName); - columns.clear(); + _log.warn("Sample type {} is indexed on {}, which is not in its provisioned table; that index is not mirrored onto the materialized table.", _ss.getName(), indexColumn.getName()); + names.clear(); break; } - columns.add(column.getSelectIdentifier().getSql().getSQL()); + names.add(column.getName()); + identifiers.add(column.getSelectIdentifier().getSql().getSQL()); } - if (columns.isEmpty() || !distinctColumnLists.add(String.join(",", columns))) + if (names.isEmpty() || !distinctColumnLists.add(indexKey(names))) continue; // Ordinal names because ${NAME} is already 33 of the 63 chars Postgres allows before it silently truncates and collides. - ddl.add("CREATE INDEX idx_${NAME}_d" + ddl.size() + " ON temp.${NAME} (" + String.join(", ", columns) + ")"); + ddl.add("CREATE INDEX idx_${NAME}_d" + ddl.size() + " ON temp.${NAME} (" + String.join(", ", identifiers) + ")"); } return ddl; } + private static String indexKey(List columnNames) + { + return columnNames.stream().map(name -> name.toLowerCase(Locale.ROOT)).collect(Collectors.joining(",")); + } + /* SELECT and JOIN, does not include WHERE, same as getJoinSQL() */ private @Nullable SQLFragment getMaterializedSQL() { diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java index ed105bbe393..778689a70d4 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java @@ -1367,20 +1367,38 @@ public void addAuditEvent(User user, Container container, String comment, String @Override public void addAuditEvent(User user, Container container, String comment, String userComment, ExpMaterial sample, Map metadata, String updateType) + { + AuditLogService.get().addEvent(user, createTimelineAuditRecord(container, comment, userComment, sample, metadata, updateType)); + } + + @Override + public SampleTimelineAuditEvent createTimelineAuditRecord(Container container, String comment, String userComment, ExpMaterial sample, Map metadata, String updateType) { SampleTimelineAuditEvent event = createAuditRecord(container, comment, userComment, sample, metadata); event.setInventoryUpdateType(updateType); event.setUserComment(userComment); - AuditLogService.get().addEvent(user, event); + return event; } @Override public void addAuditEvents(User user, Container container, String comment, String userComment, Collection samples, Map metadata) { - List events = samples.stream() - .map(sample -> createAuditRecord(container, comment, userComment, sample, metadata)) - .collect(Collectors.toList()); - AuditLogService.get().addEvents(user, events); + AuditLogService auditLog = AuditLogService.get(); + List events = new ArrayList<>(Math.min(samples.size(), AUDIT_BATCH_SIZE)); + + for (ExpMaterial sample : samples) + { + events.add(createAuditRecord(container, comment, userComment, sample, metadata)); + + if (events.size() >= AUDIT_BATCH_SIZE) + { + auditLog.addEvents(user, events); + events.clear(); + } + } + + if (!events.isEmpty()) + auditLog.addEvents(user, events); } @Override diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 21e4632f511..983065dd322 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -1049,7 +1049,7 @@ private static SqlDialect getSqlDialect(Domain domain) return CoreSchema.getInstance().getSqlDialect(); } - @Override + @Override @NotNull public SchemaTableInfo getSchemaTableInfo(Domain domain) { DomainKind kind = getDomainKind(domain); diff --git a/list/src/org/labkey/list/ListModule.java b/list/src/org/labkey/list/ListModule.java index b06f55ef246..bb7fe12597b 100644 --- a/list/src/org/labkey/list/ListModule.java +++ b/list/src/org/labkey/list/ListModule.java @@ -58,6 +58,7 @@ import org.labkey.list.model.ListManager; import org.labkey.list.model.ListManagerSchema; import org.labkey.list.model.ListQuerySchema; +import org.labkey.list.model.ListQueryUpdateService; import org.labkey.list.model.ListSchema; import org.labkey.list.model.ListServiceImpl; import org.labkey.list.model.ListWriter; @@ -236,4 +237,12 @@ public Collection getProvisionedSchemaNames() ListAuditProvider.TestCase.class ); } + + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + ListQueryUpdateService.AuditBatchTestCase.class + ); + } } diff --git a/list/src/org/labkey/list/model/ListManager.java b/list/src/org/labkey/list/model/ListManager.java index 68fa5058360..876ab565239 100644 --- a/list/src/org/labkey/list/model/ListManager.java +++ b/list/src/org/labkey/list/model/ListManager.java @@ -33,6 +33,7 @@ import org.labkey.api.cache.CacheLoader; import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.CaseInsensitiveHashMap; +import org.labkey.api.collections.CaseInsensitiveHashSet; import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.data.*; import org.labkey.api.data.Selector.ForEachBlock; @@ -1202,6 +1203,11 @@ void addAuditEvent(ListDefinitionImpl list, User user, String comment) } void addAuditEvent(ListDefinitionImpl list, User user, Container c, String comment, String entityId, @Nullable String oldRecord, @Nullable String newRecord) + { + AuditLogService.get().addEvent(user, createAuditEvent(list, c, comment, entityId, oldRecord, newRecord)); + } + + ListAuditProvider.ListAuditEvent createAuditEvent(ListDefinitionImpl list, Container c, String comment, String entityId, @Nullable String oldRecord, @Nullable String newRecord) { ListAuditProvider.ListAuditEvent event = new ListAuditProvider.ListAuditEvent(c, comment, list); @@ -1209,72 +1215,100 @@ void addAuditEvent(ListDefinitionImpl list, User user, Container c, String comme if (oldRecord != null) event.setOldRecordMap(oldRecord); if (newRecord != null) event.setNewRecordMap(newRecord); - AuditLogService.get().addEvent(user, event); + return event; } String formatAuditItem(ListDefinitionImpl list, User user, Map props) { - String itemRecord = ""; - TableInfo ti = list.getTable(user); + return getAuditItemFormatter(list, user).format(props); + } - if (null != ti) + AuditItemFormatter getAuditItemFormatter(ListDefinitionImpl list, User user) + { + return new AuditItemFormatter(list, user); + } + + /** + * Builds the audit record map for list rows. Resolve one instance per operation, not per row: the table, the + * reserved property names, and the name resolution for a given key are identical for every row of a list. + */ + static class AuditItemFormatter + { + private final TableInfo _table; + private final Domain _domain; + private final Set _reserved; + private final Map _resolvedNames = new CaseInsensitiveHashMap<>(); + + AuditItemFormatter(ListDefinitionImpl list, User user) { + _table = list.getTable(user); + _domain = list.getDomain(); + _reserved = null == _table ? Set.of() : new CaseInsensitiveHashSet(_domain.getDomainKind().getReservedPropertyNames(_domain, user)); + } + + String format(Map props) + { + if (null == _table) + return ""; + Map recordChangedMap = new CaseInsensitiveHashMap<>(); - Set reserved = list.getDomain().getDomainKind().getReservedPropertyNames(list.getDomain(), user); - // Match props to columns for (Map.Entry entry : props.entrySet()) { - String baseKey = entry.getKey(); - - boolean isReserved = false; - for (String res : reserved) - { - if (res.equalsIgnoreCase(baseKey)) - { - isReserved = true; - break; - } - } - - if (isReserved) - continue; - - ColumnInfo col = ti.getColumn(FieldKey.fromParts(baseKey)); Object value = entry.getValue(); - String key = null; - if (null != col) - { - // Found the column - key = col.getName(); // best good - } - else - { - // See if there is a match in the domain properties - for (DomainProperty dp : list.getDomain().getProperties()) - { - if (dp.getName().equalsIgnoreCase(baseKey)) - { - key = dp.getName(); // middle good - } - } + if (null == value) + continue; - // Try by name - DomainProperty dp = list.getDomain().getPropertyByName(baseKey); - if (null != dp) - key = dp.getName(); - } + String key = resolveName(entry.getKey()); - if (null != key && null != value) + if (null != key) recordChangedMap.put(key, value); } - if (!recordChangedMap.isEmpty()) - itemRecord = ListAuditProvider.encodeForDataMap(recordChangedMap); + return recordChangedMap.isEmpty() ? "" : ListAuditProvider.encodeForDataMap(recordChangedMap); + } + + /** @return the canonical property name to audit under, or null if the key is reserved or unknown */ + private String resolveName(String baseKey) + { + if (_resolvedNames.containsKey(baseKey)) + return _resolvedNames.get(baseKey); + + String key = computeName(baseKey); + _resolvedNames.put(baseKey, key); + + return key; } - return itemRecord; + private String computeName(String baseKey) + { + if (_reserved.contains(baseKey)) + return null; + + ColumnInfo col = _table.getColumn(FieldKey.fromParts(baseKey)); + + if (null != col) + return col.getName(); // best good + + String key = null; + + // See if there is a match in the domain properties + for (DomainProperty dp : _domain.getProperties()) + { + if (dp.getName().equalsIgnoreCase(baseKey)) + { + key = dp.getName(); // middle good + } + } + + // Try by name + DomainProperty dp = _domain.getPropertyByName(baseKey); + if (null != dp) + key = dp.getName(); + + return key; + } } boolean importListSchema( diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index 7b3426148f4..4a2a3121e01 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -18,10 +18,15 @@ import org.apache.commons.lang3.StringUtils; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; import org.labkey.api.attachments.AttachmentFile; import org.labkey.api.attachments.AttachmentParent; import org.labkey.api.attachments.AttachmentParentFactory; import org.labkey.api.attachments.AttachmentService; +import org.labkey.api.audit.AbstractAuditHandler; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; import org.labkey.api.audit.TransactionAuditProvider; @@ -52,6 +57,7 @@ import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; import org.labkey.api.exp.property.IPropertyValidator; +import org.labkey.api.exp.property.PropertyService; import org.labkey.api.exp.property.ValidatorContext; import org.labkey.api.gwt.client.AuditBehaviorType; import org.labkey.api.lists.permissions.ManagePicklistsPermission; @@ -73,11 +79,13 @@ import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.test.TestTimeout; import org.labkey.api.usageMetrics.SimpleMetricsService; import org.labkey.api.util.GUID; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.StringUtilsLabKey; +import org.labkey.api.util.TestContext; import org.labkey.api.util.UnexpectedException; import org.labkey.api.view.UnauthorizedException; import org.labkey.api.writer.VirtualFile; @@ -102,6 +110,12 @@ public class ListQueryUpdateService extends DefaultQueryUpdateService { private final ListDefinitionImpl _list; private static final String ID = "entityId"; + private static final String AUDIT_COMMENT_INSERT = "A new list record was inserted"; + private static final String AUDIT_COMMENT_UPDATE = "An existing list record was modified"; + private static final String AUDIT_COMMENT_DELETE = "An existing list record was deleted"; + + /** Non-null while a multi-row operation is in flight; see {@link RowAuditBatch}. */ + private RowAuditBatch _auditBatch; public ListQueryUpdateService(ListTable queryTable, TableInfo dbTable, @NotNull ListDefinition list) { @@ -179,27 +193,107 @@ public List> insertRows(User user, Container container, List if (null != result) { - ListManager mgr = ListManager.get(); - - for (Map row : result) + try (RowAuditBatch ignored = beginAuditBatch(user)) { - if (null != row.get(ID)) + for (Map row : result) { - // Audit each row - String entityId = (String) row.get(ID); - String newRecord = mgr.formatAuditItem(_list, user, row); + if (null != row.get(ID)) + { + // Audit each row + String entityId = (String) row.get(ID); - mgr.addAuditEvent(_list, user, container, "A new list record was inserted", entityId, null, newRecord); + auditRowChange(user, container, AUDIT_COMMENT_INSERT, entityId, null, row); + } } } if (!result.isEmpty() && !errors.hasErrors()) - mgr.indexList(_list); + ListManager.get().indexList(_list); } return result; } + /** + * Accumulates row audit events so one operation writes them with a single batched statement. + */ + private class RowAuditBatch implements AutoCloseable + { + private final User _user; + private final ListManager.AuditItemFormatter _formatter; + private final List _events = new ArrayList<>(); + + RowAuditBatch(User user) + { + _user = user; + _formatter = ListManager.get().getAuditItemFormatter(_list, user); + } + + String format(Map props) + { + return _formatter.format(props); + } + + void add(Container c, String comment, String entityId, @Nullable String oldRecord, @Nullable String newRecord) + { + _events.add(ListManager.get().createAuditEvent(_list, c, comment, entityId, oldRecord, newRecord)); + + if (_events.size() >= AbstractAuditHandler.AUDIT_BATCH_SIZE) + flush(); + } + + void flush() + { + if (_events.isEmpty()) + return; + + // close() flushes again on the way out of a failed operation, so hand off before writing to avoid re-inserting + List toWrite = new ArrayList<>(_events); + _events.clear(); + + AuditLogService.get().addEvents(_user, toWrite); + } + + @Override + public void close() + { + flush(); + + if (this == _auditBatch) + _auditBatch = null; + } + } + + /** @return a batch to close when the operation completes, or null if an enclosing operation already owns one */ + private @Nullable RowAuditBatch beginAuditBatch(User user) + { + if (null != _auditBatch) + return null; + + _auditBatch = new RowAuditBatch(user); + + return _auditBatch; + } + + private void auditRowChange(User user, Container container, String comment, String entityId, @Nullable Map oldRow, @Nullable Map newRow) + { + RowAuditBatch batch = _auditBatch; + + if (null == batch) + { + ListManager mgr = ListManager.get(); + mgr.addAuditEvent(_list, user, container, comment, entityId, + null == oldRow ? null : mgr.formatAuditItem(_list, user, oldRow), + null == newRow ? null : mgr.formatAuditItem(_list, user, newRow)); + } + else + { + batch.add(container, comment, entityId, + null == oldRow ? null : batch.format(oldRow), + null == newRow ? null : batch.format(newRow)); + } + } + private User getListUser(User user, Container container) { if (_list.isPicklist() && container.hasPermission(user, ManagePicklistsPermission.class)) @@ -306,7 +400,15 @@ public List> updateRows(User user, Container container, List if (!_list.isVisible(user)) throw new UnauthorizedException("You do not have permission to update data into this table."); - List> result = super.updateRows(getListUser(user, container), container, rows, oldKeys, errors, configParameters, extraScriptContext); + List> result; + User listUser = getListUser(user, container); + + // updateRow() audits as the list user, so the batch must format and log as that user too + try (RowAuditBatch ignored = beginAuditBatch(listUser)) + { + result = super.updateRows(listUser, container, rows, oldKeys, errors, configParameters, extraScriptContext); + } + if (!result.isEmpty()) ListManager.get().indexList(_list); return result; @@ -394,7 +496,6 @@ else if (r.getValue() != null && !StringUtils.isEmpty(String.valueOf(r.getValue( if (null != result && null != result.get(ID)) { - ListManager mgr = ListManager.get(); String entityId = (String) result.get(ID); try @@ -430,11 +531,8 @@ else if (r.getValue() != null && !StringUtils.isEmpty(String.valueOf(r.getValue( } } - String oldRecord = mgr.formatAuditItem(_list, user, oldRow); - String newRecord = mgr.formatAuditItem(_list, user, result); - // Audit - mgr.addAuditEvent(_list, user, container, "An existing list record was modified", entityId, oldRecord, newRecord); + auditRowChange(user, container, AUDIT_COMMENT_UPDATE, entityId, oldRow, result); } } @@ -728,6 +826,16 @@ private int addDetailedMoveAuditEvents(User user, Container sourceContainer, Con return auditEvents.size(); } + @Override + public List> deleteRows(User user, Container container, List> keys, @Nullable Map configParameters, @Nullable Map extraScriptContext) + throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException + { + try (RowAuditBatch ignored = beginAuditBatch(user)) + { + return super.deleteRows(user, container, keys, configParameters, extraScriptContext); + } + } + @Override protected Map deleteRow(User user, Container container, Map oldRowMap) throws InvalidKeyException, QueryUpdateServiceException, SQLException { @@ -743,10 +851,9 @@ protected Map deleteRow(User user, Container container, Map> rows = new ArrayList<>(ROW_COUNT); + for (int i = 0; i < ROW_COUNT; i++) + rows.add(CaseInsensitiveHashMap.of(VALUE_FIELD, "row-" + i)); + + BatchValidationException errors = new BatchValidationException(); + List> inserted; + + // Without a transaction LogManager declines to batch, so the batched insert path only runs inside one + try (DbScope.Transaction tx = table.getSchema().getScope().ensureTransaction()) + { + inserted = table.getUpdateService().insertRows(_user, _container, rows, errors, null, null); + if (errors.hasErrors()) + throw errors.getLastRowError(); + tx.commit(); + } + + assertEquals("expected every row to be inserted", ROW_COUNT, inserted.size()); + assertEquals("one insert audit event per row", ROW_COUNT, auditCount(AUDIT_COMMENT_INSERT)); + + List> keys = new ArrayList<>(ROW_COUNT); + for (Map row : inserted) + keys.add(CaseInsensitiveHashMap.of(KEY_NAME, row.get(KEY_NAME))); + + List> deleted; + + try (DbScope.Transaction tx = table.getSchema().getScope().ensureTransaction()) + { + deleted = table.getUpdateService().deleteRows(_user, _container, keys, null, null); + tx.commit(); + } + + assertEquals("expected every row to be deleted", ROW_COUNT, deleted.size()); + assertEquals("one delete audit event per row", ROW_COUNT, auditCount(AUDIT_COMMENT_DELETE)); + assertEquals("deleting rows must not emit insert events", ROW_COUNT, auditCount(AUDIT_COMMENT_INSERT)); + } + } } From d7ffa3d878abcf224e27fa6114f43525aeef9849 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 3 Sep 2026 11:41:40 -0700 Subject: [PATCH 04/10] Fix memory leak failures --- api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index 1e0de7f467e..01fd3b82b3f 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -50,6 +50,7 @@ import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; import org.labkey.api.util.DateUtil; +import org.labkey.api.util.MemTracker; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; @@ -288,6 +289,8 @@ public TableInfo getStorageTableInfoForInsert() TableInfo storageTableInfo = StorageProvisioner.createTableInfo(domain); // Shared across threads from here on storageTableInfo.setLocked(true); + // Held for the process lifetime, so stop tracking it; MemTracker reports every retained TableInfo as a leak. + MemTracker.getInstance().remove(storageTableInfo); _cachedStorageTable = new CachedStorageTable(schemaTableInfo, storageTableInfo); return storageTableInfo; From 5b8945cfb51fa0d6e7e5ce033147d364e7f18642 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 3 Sep 2026 16:26:02 -0700 Subject: [PATCH 05/10] Test fixes --- audit/src/org/labkey/audit/model/LogManager.java | 4 ++-- .../org/labkey/experiment/api/ExpMaterialTableImpl.java | 9 ++++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/audit/src/org/labkey/audit/model/LogManager.java b/audit/src/org/labkey/audit/model/LogManager.java index 6a4c120b23c..7ba1c0f64ab 100644 --- a/audit/src/org/labkey/audit/model/LogManager.java +++ b/audit/src/org/labkey/audit/model/LogManager.java @@ -131,9 +131,9 @@ public void insertEvents(User user, List events) Logger auditLogger = getAuditLogger(type); SQLException sqlx = null; - try (Connection conn = dbTable.getSchema().getScope().getConnection()) + try (Connection conn = dbTable.getSchema().getScope().getConnection(); + ParameterMapStatement stmt = StatementUtils.insertStatement(conn, dbTable, c, user, false, true)) { - ParameterMapStatement stmt = StatementUtils.insertStatement(conn, dbTable, c, user, false, true); for (var event : events) { event = validateFields(provider, event); diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 97810e1d4e8..4ce12b13dc6 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -25,6 +25,7 @@ import org.jetbrains.annotations.Nullable; import org.junit.AfterClass; import org.junit.Assert; +import org.junit.Assume; import org.junit.BeforeClass; import org.junit.Test; import org.labkey.api.assay.plate.AssayPlateMetadataService; @@ -1386,10 +1387,14 @@ private _MaterializedQueryHelper getOrCreateMQH() * provisioned table's own indices, not {@link Domain#getPropertyIndices()} -- that set is only ever populated by * the caller that is saving a domain, so it is empty on a domain read back from the database. Never unique: * {@link #getJoinSQL} selects root-scoped columns from the root sample's row, so a value that is unique in the - * provisioned table repeats across every aliquot sharing that root. + * provisioned table repeats across every aliquot sharing that root. PostgreSQL only: SQL Server refuses an index + * over a column as wide as a default string property, so a mirrored index there would fail the materialization. */ private List getDomainIndexDdl() { + if (!getExpSchema().getDbSchema().getSqlDialect().isPostgreSQL()) + return List.of(); + TableInfo provisioned = _ss.getTinfo(); if (null == provisioned) return List.of(); @@ -2330,6 +2335,8 @@ public void testMergeInsertAndUpdate() throws Exception @Test public void testDomainIndexMirroredNonUnique() throws Exception { + Assume.assumeTrue("Domain indices are only mirrored on PostgreSQL", ExperimentService.get().getSchema().getSqlDialect().isPostgreSQL()); + ExpSampleType st = createSampleType("IncrUpdIndexed", List.of(new GWTIndex(List.of("rootProp"), true))); insertRoots(st, "R1"); insertAliquots(st, "R1", 3); From f8286b3df7a68fbefc2724d1fbf7b04ea664e3a8 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 3 Sep 2026 17:17:12 -0700 Subject: [PATCH 06/10] Test fixes --- .../api/audit/AbstractAuditHandler.java | 34 ++++++++++--------- audit/src/org/labkey/audit/AuditLogImpl.java | 32 +++++++++++++++++ .../experiment/api/SampleTypeServiceImpl.java | 4 +-- .../org/labkey/query/QueryServiceImpl.java | 4 +-- .../labkey/study/model/DatasetDefinition.java | 6 ++-- 5 files changed, 57 insertions(+), 23 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditHandler.java b/api/src/org/labkey/api/audit/AbstractAuditHandler.java index bfeac495cf0..cc9211bb257 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditHandler.java +++ b/api/src/org/labkey/api/audit/AbstractAuditHandler.java @@ -38,7 +38,7 @@ public abstract class AbstractAuditHandler implements AuditHandler /** Bounds both the JDBC batch and the events held in memory while a batch accumulates. */ public static final int AUDIT_BATCH_SIZE = 2500; - protected abstract AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row); + protected abstract AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents); @Override public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryService.AuditAction action, Integer dataRowCount, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String userComment) @@ -56,9 +56,11 @@ public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryS if (auditType == SUMMARY || skipAuditLevelCheck) { - AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, dataRowCount, null); + List sideEffectEvents = new ArrayList<>(); + AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, dataRowCount, null, sideEffectEvents); AuditLogService.get().addEvent(user, event); + addSideEffectEvents(AuditLogService.get(), user, sideEffectEvents, false); } } } @@ -73,18 +75,10 @@ public void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryS * @param row map of new data values * @param existingRow map of data values * @param providedValues map of values provided by the user before conversion (e.g., for quantity values) + * @param sideEffectEvents collects audit events raised as a side effect of building this record, for the caller to flush through {@link #addSideEffectEvents} * @return DetailedAuditTypeEvent object describing audit record (NOTE: not committed to DB yet) */ - protected abstract DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues); - - /** - * Overload for providers whose record construction produces audit events of its own. Events added to - * sideEffectEvents are batched by addAuditEvent() rather than inserted one per row. - */ - protected DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues, List sideEffectEvents) - { - return createDetailedAuditRecord(user, c, tInfo, action, userComment, row, existingRow, providedValues); - } + protected abstract DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues, List sideEffectEvents); /** * Allow for adding fields that may be present in the updated row but not represented in the original row @@ -117,8 +111,10 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud case SUMMARY: case DETAILED: - AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, 0, null); + List truncateSideEffects = new ArrayList<>(); + AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, 0, null, truncateSideEffects); AuditLogService.get().addEvent(user, event); + addSideEffectEvents(AuditLogService.get(), user, truncateSideEffects, useTransactionAuditCache); return; } } @@ -132,9 +128,11 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud { assert null != rows; - AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, rows.size(), rows.getFirst()); + List sideEffectEvents = new ArrayList<>(); + AuditTypeEvent event = createSummaryAuditRecord(user, c, auditConfigurable, action, userComment, rows.size(), rows.getFirst(), sideEffectEvents); AuditLogService.get().addEvent(user, event); + addSideEffectEvents(AuditLogService.get(), user, sideEffectEvents, useTransactionAuditCache); return; } @@ -214,8 +212,12 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud } } - /** insertEvents() batches only a fully homogeneous list. Group by event type and container. Each type is stored its own provisioned table */ - private void addSideEffectEvents(AuditLogService auditLog, User user, List events, boolean useTransactionAuditCache) + /** + * The one place side-effect events are written, so they can't pick up different batching or transaction-cache + * behavior depending on which call path produced them. insertEvents() batches only a fully homogeneous list, so + * group by event type and container -- each type is stored in its own provisioned table. + */ + public static void addSideEffectEvents(AuditLogService auditLog, User user, List events, boolean useTransactionAuditCache) { events.stream() .collect(Collectors.groupingBy(event -> Pair.of(event.getEventType(), event.getContainer()), LinkedHashMap::new, Collectors.toList())) diff --git a/audit/src/org/labkey/audit/AuditLogImpl.java b/audit/src/org/labkey/audit/AuditLogImpl.java index ac0364a9be8..f2e33f0846d 100644 --- a/audit/src/org/labkey/audit/AuditLogImpl.java +++ b/audit/src/org/labkey/audit/AuditLogImpl.java @@ -45,6 +45,7 @@ import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.util.ContextListener; +import org.labkey.api.util.GUID; import org.labkey.api.util.Pair; import org.labkey.api.util.StartupListener; import org.labkey.api.util.logging.LogHelper; @@ -54,13 +55,17 @@ import org.labkey.audit.query.AuditQuerySchema; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Queue; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Predicate; import java.util.stream.Collectors; public class AuditLogImpl implements AuditLogService, StartupListener @@ -248,6 +253,31 @@ public ActionURL getAuditUrl() return new ActionURL(AuditController.ShowAuditLogAction.class, ContainerManager.getRoot()); } + /** + * Mirrors {@link ContainerFilter}'s SQL: an event matches on its own container, or on being a child of an in-scope + * container whose type the filter includes. Cached events need this applied by hand -- reading them back from the + * database is what would otherwise apply it. + */ + private static Predicate inScope(User user, Container container, @Nullable ContainerFilter containerFilter) + { + Collection ids = (null == containerFilter ? ContainerFilter.current(container, user) : containerFilter).getIds(); + if (null == ids) + return event -> true; + + Set scope = new HashSet<>(ids); + Set childTypes = null == containerFilter ? Collections.emptySet() : containerFilter.getIncludedChildTypes(); + + return event -> { + Container c = event.getContainer(); + if (null == c) + return false; + if (scope.contains(c.getEntityId())) + return true; + Container parent = c.getParent(); + return null != parent && childTypes.contains(c.getType()) && scope.contains(parent.getEntityId()); + }; + } + public record TransactionRowIds(List rowIds, Map dataTypeRowCounts) {} public TransactionRowIds getTransactionSampleIds(long transactionAuditId, User user, Container container, @Nullable ContainerFilter containerFilter) @@ -265,6 +295,7 @@ public TransactionRowIds getTransactionSampleIds(long transactionAuditId, User u events = transactionEvents.stream() .filter(SampleTimelineAuditEvent.class::isInstance) .map(SampleTimelineAuditEvent.class::cast) + .filter(inScope(user, container, containerFilter)) .toList(); } Map dataTypeRowCounts = new HashMap<>(); @@ -287,6 +318,7 @@ public TransactionRowIds getTransactionSourceIds(long transactionAuditId, User u : transactionEvents.stream() .filter(DetailedAuditTypeEvent.class::isInstance) .map(DetailedAuditTypeEvent.class::cast) + .filter(inScope(user, container, containerFilter)) .toList(); detailedEvents.forEach(event -> { diff --git a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java index 778689a70d4..58b6cea2ba6 100644 --- a/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/SampleTypeServiceImpl.java @@ -1231,13 +1231,13 @@ public String getCommentDetailed(QueryService.AuditAction action, boolean isUpda } @Override - public DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues) + public DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map row, Map existingRow, Map providedValues, List sideEffectEvents) { return createAuditRecord(c, tInfo, getCommentDetailed(action, !existingRow.isEmpty()), userComment, action, row, existingRow, providedValues); } @Override - protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row) + protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents) { return createAuditRecord(c, tInfo, String.format(action.getCommentSummary(), rowCount), userComment, row); } diff --git a/query/src/org/labkey/query/QueryServiceImpl.java b/query/src/org/labkey/query/QueryServiceImpl.java index 6db625d39af..1e1927aea41 100644 --- a/query/src/org/labkey/query/QueryServiceImpl.java +++ b/query/src/org/labkey/query/QueryServiceImpl.java @@ -3083,7 +3083,7 @@ public AuditHandler getDefaultAuditHandler() return new AbstractAuditHandler() { @Override - protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row) + protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents) { DetailedAuditTypeEvent event = createAuditRecord(c, tinfo, String.format(action.getCommentSummary(), rowCount), row, null); event.setUserComment(userComment); @@ -3091,7 +3091,7 @@ protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditC } @Override - protected DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, @Nullable Map updatedRow, Map existingRow, @Nullable Map providedValues) + protected DetailedAuditTypeEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tinfo, AuditAction action, @Nullable String userComment, @Nullable Map updatedRow, Map existingRow, @Nullable Map providedValues, List sideEffectEvents) { DetailedAuditTypeEvent event = createAuditRecord(c, tinfo, action.getCommentDetailed(), updatedRow, existingRow); event.setUserComment(userComment); diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index fd9c1cebc1a..7ccaa672ffc 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -1790,7 +1790,7 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud Map row = rows.get(i); Map existingRow = null==existingRows ? null : existingRows.get(i); // note switched order (oldRecord, newRecord) - var event = createDetailedAuditRecord(user, c, (AuditConfigurable)table, action, userComment, row, existingRow, null); + var event = createDetailedAuditRecord(user, c, (AuditConfigurable)table, action, userComment, row, existingRow, null, List.of()); batch.add(event); if (batch.size() > 1000) { @@ -1808,7 +1808,7 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud } @Override - protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row) + protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, int rowCount, @Nullable Map row, List sideEffectEvents) { throw new UnsupportedOperationException(); } @@ -1817,7 +1817,7 @@ protected AuditTypeEvent createSummaryAuditRecord(User user, Container c, AuditC * NOTE: userComment field is not supported for this domain and will be ignored */ @Override - protected DatasetAuditProvider.DatasetAuditEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map record, Map existingRecord, Map providedValues) + protected DatasetAuditProvider.DatasetAuditEvent createDetailedAuditRecord(User user, Container c, AuditConfigurable tInfo, QueryService.AuditAction action, @Nullable String userComment, @Nullable Map record, Map existingRecord, Map providedValues, List sideEffectEvents) { String auditComment = switch (action) { From 93d33c714acb6feed28a99627372e45ebc3b091b Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Thu, 3 Sep 2026 18:30:55 -0700 Subject: [PATCH 07/10] Untrack the DomainDescriptor retained by cached audit storage tables The cached provisioned TableInfo pins its Domain's DomainDescriptor, which MemTracker tracks separately, so the leak check never reached zero. StorageProvisioner.createSharedTableInfo() now owns the locking and untracking for both. --- .../api/audit/AbstractAuditTypeProvider.java | 7 +------ .../org/labkey/api/exp/api/StorageProvisioner.java | 14 ++++++++++++++ .../api/property/StorageProvisionerImpl.java | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index 01fd3b82b3f..a9b497d035b 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -50,7 +50,6 @@ import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; import org.labkey.api.util.DateUtil; -import org.labkey.api.util.MemTracker; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.view.ActionURL; @@ -286,11 +285,7 @@ public TableInfo getStorageTableInfoForInsert() if (null != cached && cached.schemaTableInfo() == schemaTableInfo) return cached.storageTableInfo(); - TableInfo storageTableInfo = StorageProvisioner.createTableInfo(domain); - // Shared across threads from here on - storageTableInfo.setLocked(true); - // Held for the process lifetime, so stop tracking it; MemTracker reports every retained TableInfo as a leak. - MemTracker.getInstance().remove(storageTableInfo); + TableInfo storageTableInfo = StorageProvisioner.createSharedTableInfo(domain); _cachedStorageTable = new CachedStorageTable(schemaTableInfo, storageTableInfo); return storageTableInfo; diff --git a/api/src/org/labkey/api/exp/api/StorageProvisioner.java b/api/src/org/labkey/api/exp/api/StorageProvisioner.java index 24282846971..c59be93bea4 100644 --- a/api/src/org/labkey/api/exp/api/StorageProvisioner.java +++ b/api/src/org/labkey/api/exp/api/StorageProvisioner.java @@ -71,6 +71,20 @@ static TableInfo createTableInfo(@NotNull Domain domain) /* NOTE: static createTable/createTableImpl is a very minor hack to avoid having to update a zillions repos at once */ TableInfo createTableInfoImpl(@NotNull Domain domain); + /** + * Variant of {@link #createTableInfo} for callers that cache the result for the process lifetime. The table is + * locked for cross-thread sharing, and it plus the DomainDescriptor it retains are dropped from MemTracker, which + * would otherwise report both as leaks. + */ + @NotNull + static TableInfo createSharedTableInfo(@NotNull Domain domain) + { + return get().createSharedTableInfoImpl(domain); + } + + @NotNull + TableInfo createSharedTableInfoImpl(@NotNull Domain domain); + /** * This is really an internal method, use createTableInfo() in most scenarios * This is public to support upgrade scenarios only. diff --git a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java index 983065dd322..0ffb757bd08 100644 --- a/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java +++ b/experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java @@ -87,6 +87,7 @@ import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.GUID; import org.labkey.api.util.JunitUtil; +import org.labkey.api.util.MemTracker; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Path; import org.labkey.api.util.TestContext; @@ -650,6 +651,19 @@ public TableInfo createTableInfoImpl(@NotNull Domain domain) return wrapper; } + @Override + @NotNull + public TableInfo createSharedTableInfoImpl(@NotNull Domain domain) + { + TableInfo table = createTableInfoImpl(domain); + table.setLocked(true); + MemTracker.getInstance().remove(table); + // The table holds the Domain, which holds a DomainDescriptor that MemTracker tracks separately + if (domain instanceof DomainImpl di) + MemTracker.getInstance().remove(di._dd); + return table; + } + @NotNull private SchemaTableInfo getSchemaTableInfo(@NotNull Domain domain, String schemaName, String tableName, DbSchema schema) { From 09aa370075fca443001137cbe60d4ceabe7952d9 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 4 Sep 2026 11:17:08 -0700 Subject: [PATCH 08/10] Claude review --- .../api/audit/AbstractAuditTypeProvider.java | 2 +- .../api/data/MaterializedQueryHelper.java | 60 +++++-- .../api/dataiterator/SimpleTranslator.java | 20 +-- audit/src/org/labkey/audit/AuditLogImpl.java | 166 +++++++++++++++++- audit/src/org/labkey/audit/AuditModule.java | 8 + .../experiment/CustomPropertiesView.java | 4 +- .../experiment/api/ExpMaterialTableImpl.java | 25 ++- .../list/model/ListQueryUpdateService.java | 13 +- .../labkey/study/model/DatasetDefinition.java | 2 +- 9 files changed, 263 insertions(+), 37 deletions(-) diff --git a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java index a9b497d035b..acefaa116c3 100644 --- a/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java +++ b/api/src/org/labkey/api/audit/AbstractAuditTypeProvider.java @@ -279,7 +279,7 @@ public TableInfo getStorageTableInfoForInsert() // We want to reuse a cached provisioned TableInfo to avoid construction costs. Getting the SchemaTableInfo is // cheap so use that as a guide for when the table has changed (primarily during startup as its shape may - // need to be updated based on current code expections) and when it's safe to reuse the previous copy. + // need to be updated based on current code expectations) and when it's safe to reuse the previous copy. SchemaTableInfo schemaTableInfo = StorageProvisioner.get().getSchemaTableInfo(domain); CachedStorageTable cached = _cachedStorageTable; if (null != cached && cached.schemaTableInfo() == schemaTableInfo) diff --git a/api/src/org/labkey/api/data/MaterializedQueryHelper.java b/api/src/org/labkey/api/data/MaterializedQueryHelper.java index dd8b7ca42ef..f42446e7700 100644 --- a/api/src/org/labkey/api/data/MaterializedQueryHelper.java +++ b/api/src/org/labkey/api/data/MaterializedQueryHelper.java @@ -180,18 +180,17 @@ boolean load(SQLFragment selectQuery, boolean isSelectInto) selectInto.append("\n) _sql_"); } new SqlExecutor(_mqh._scope).execute(selectInto); - - try (var ignored = SpringActionController.ignoreSqlUpdates()) - { - for (String index : _mqh._indexes) - { - new SqlExecutor(_mqh._scope).execute(StringUtils.replace(index, "${NAME}", _tableName)); - } - } + createIndexes(_mqh._indexes, false); analyze(); }); + // Published here, not after the deferred indexes: those only make the table faster, and building them + // first keeps every reader on the unmaterialized query for the length of the slowest index. _loadingState.set(LoadingState.LOADED); + + if (!_mqh._deferredIndexes.isEmpty()) + traced("full.deferredIndexes", _mqh.getMaterializationName(), () -> createIndexes(_mqh._deferredIndexes, true)); + return true; } catch (RuntimeException rex) @@ -206,6 +205,31 @@ boolean load(SQLFragment selectQuery, boolean isSelectInto) } } + /** + * @param tolerateFailure true once the table is serving reads, where a missing index costs speed but nothing else, + * so the failure is logged and the remaining indexes are still attempted + */ + private void createIndexes(List indexes, boolean tolerateFailure) + { + try (var ignored = SpringActionController.ignoreSqlUpdates()) + { + for (String index : indexes) + { + String sql = StringUtils.replace(index, "${NAME}", _tableName); + try + { + new SqlExecutor(_mqh._scope).execute(sql); + } + catch (RuntimeException x) + { + if (!tolerateFailure) + throw x; + LOG.error("Failed to create index on materialized table {}. The table is serving queries without it. DDL: {}", _tableName, sql, x); + } + } + } + } + /** SELECT INTO leaves the table with no statistics, so the planner guesses until autovacuum eventually analyzes it. */ private void analyze() { @@ -389,6 +413,7 @@ private String makeKey(DbScope.Transaction t) protected final SQLFragment _uptodateQuery; protected final Supplier _supplier; private final List _indexes = new ArrayList<>(); + private final List _deferredIndexes = new ArrayList<>(); protected final long _maxTimeToCache; private final Map _map = Collections.synchronizedMap(new LinkedHashMap<>() { @@ -410,7 +435,7 @@ protected boolean removeEldestEntry(Map.Entry eldest) private boolean _closed = false; - protected MaterializedQueryHelper(String prefix, DbScope scope, SQLFragment select, @Nullable SQLFragment uptodate, Supplier supplier, @Nullable Collection indexes, long maxTimeToCache, + protected MaterializedQueryHelper(String prefix, DbScope scope, SQLFragment select, @Nullable SQLFragment uptodate, Supplier supplier, @Nullable Collection indexes, @Nullable Collection deferredIndexes, long maxTimeToCache, boolean isSelectIntoSql, boolean unlogged) { _prefix = Objects.toString(prefix,"mat"); @@ -421,6 +446,8 @@ protected MaterializedQueryHelper(String prefix, DbScope scope, SQLFragment sele _maxTimeToCache = maxTimeToCache; if (null != indexes) _indexes.addAll(indexes); + if (null != deferredIndexes) + _deferredIndexes.addAll(deferredIndexes); _isSelectIntoSql = isSelectIntoSql; _unlogged = unlogged; assert MemTracker.get().put(this); @@ -769,14 +796,14 @@ protected void initMaterialized(Materialized materialized) @Deprecated // use Builder public static MaterializedQueryHelper create(String prefix, DbScope scope, SQLFragment select, @Nullable SQLFragment uptodate, Collection indexes, long maxTimeToCache) { - return new MaterializedQueryHelper(prefix, scope, select, uptodate, null, indexes, maxTimeToCache, false, false); + return new MaterializedQueryHelper(prefix, scope, select, uptodate, null, indexes, null, maxTimeToCache, false, false); } @Deprecated // use Builder public static MaterializedQueryHelper create(String prefix, DbScope scope, SQLFragment select, Supplier uptodate, Collection indexes, long maxTimeToCache) { - return new MaterializedQueryHelper(prefix, scope, select, null, uptodate, indexes, maxTimeToCache, false, false); + return new MaterializedQueryHelper(prefix, scope, select, null, uptodate, indexes, null, maxTimeToCache, false, false); } public static class Builder implements org.labkey.api.data.Builder @@ -791,6 +818,7 @@ public static class Builder implements org.labkey.api.data.Builder _supplier = null; protected Collection _indexes = new ArrayList<>(); + protected Collection _deferredIndexes = new ArrayList<>(); public Builder(String prefix, DbScope scope, SQLFragment select) { @@ -834,16 +862,24 @@ public Builder addInvalidCheck(Supplier supplier) return this; } + /** Index built before the table is published, for anything a reader or the incremental maintenance SQL cannot go without. */ public Builder addIndex(String index) { _indexes.add(index); return this; } + /** Index built after the table is published to readers; a failure is logged and the table serves without it. */ + public Builder addDeferredIndex(String index) + { + _deferredIndexes.add(index); + return this; + } + @Override public MaterializedQueryHelper build() { - return new MaterializedQueryHelper(_prefix, _scope, _select, _uptodate, _supplier, _indexes, _max, _isSelectInto, _unlogged); + return new MaterializedQueryHelper(_prefix, _scope, _select, _uptodate, _supplier, _indexes, _deferredIndexes, _max, _isSelectInto, _unlogged); } } diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 19abc80dcbd..b17fc536299 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -2247,7 +2247,7 @@ private EnumTableInfo remapLookupTable() } @Test - public void remapMemoizationSurvivesPkLookupToggle() + public void remapCacheSurvivesPkLookupToggle() { RemapConverter converter = new RemapConverter(remapLookupTable(), true, false, true); @@ -2257,11 +2257,11 @@ public void remapMemoizationSurvivesPkLookupToggle() List>> maps = converter.getMaps(); assertEquals("expected one alternate-key map, on the Value column", 1, maps.size()); - // Seed keys no enum value can supply, so anything but a memo hit resolves to null - MultiValuedMap memo = maps.getFirst().getRight(); + // Seed keys no enum value can supply, so anything but a cache hit resolves to null + MultiValuedMap cache = maps.getFirst().getRight(); Integer seeded = 42; - memo.put("seeded-hit", seeded); - memo.put("seeded-miss", converter.MISS); + cache.put("seeded-hit", seeded); + cache.put("seeded-miss", converter.MISS); assertEquals(seeded, converter.mappedValue("seeded-hit")); assertNull(converter.mappedValue("seeded-miss")); @@ -2272,7 +2272,7 @@ public void remapMemoizationSurvivesPkLookupToggle() converter.setIncludePkLookup(false); } - assertSame("toggling includePkLookup discarded the memoization maps", maps, converter.getMaps()); + assertSame("toggling includePkLookup discarded the cached lookups", maps, converter.getMaps()); assertEquals("resolved value was discarded, so every row re-queries it", seeded, converter.mappedValue("seeded-hit")); assertNull("MISS marker was discarded, so every row re-queries the absent value", converter.mappedValue("seeded-miss")); } @@ -2301,10 +2301,10 @@ public void remapAlternateKeyWinsWhenPkLookupIsOff() Integer key = 7; Integer pkResolution = 7; Integer akResolution = 99; - Map pkMemo = converter.pkLookupMap().getValue(); - MultiValuedMap akMemo = converter.getMaps().getFirst().getRight(); - pkMemo.put(key, pkResolution); - akMemo.put(key, akResolution); + Map pkCache = converter.pkLookupMap().getValue(); + MultiValuedMap akCache = converter.getMaps().getFirst().getRight(); + pkCache.put(key, pkResolution); + akCache.put(key, akResolution); assertEquals("pk lookup should take precedence while includePkLookup is on", pkResolution, converter.mappedValue(key)); diff --git a/audit/src/org/labkey/audit/AuditLogImpl.java b/audit/src/org/labkey/audit/AuditLogImpl.java index f2e33f0846d..8d177fd2d8f 100644 --- a/audit/src/org/labkey/audit/AuditLogImpl.java +++ b/audit/src/org/labkey/audit/AuditLogImpl.java @@ -20,6 +20,11 @@ import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.Assume; +import org.junit.BeforeClass; +import org.junit.Test; import org.labkey.api.action.SpringActionController; import org.labkey.api.audit.AbstractAuditTypeProvider; import org.labkey.api.audit.AuditLogService; @@ -27,6 +32,7 @@ import org.labkey.api.audit.AuditTypeProvider; import org.labkey.api.audit.DetailedAuditTypeEvent; import org.labkey.api.audit.SampleTimelineAuditEvent; +import org.labkey.api.audit.permissions.CanSeeAuditLogPermission; import org.labkey.api.cache.Cache; import org.labkey.api.cache.CacheManager; import org.labkey.api.collections.CaseInsensitiveHashMap; @@ -37,17 +43,27 @@ import org.labkey.api.data.SimpleFilter; import org.labkey.api.data.Sort; import org.labkey.api.data.TableSelector; +import org.labkey.api.data.WorkbookContainerType; import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.query.AbstractQueryUpdateService; import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; +import org.labkey.api.security.LimitedUser; +import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.ReadPermission; +import org.labkey.api.security.roles.CanSeeAuditLogRole; +import org.labkey.api.security.roles.ReaderRole; +import org.labkey.api.security.roles.Role; +import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.ContextListener; import org.labkey.api.util.GUID; import org.labkey.api.util.Pair; import org.labkey.api.util.StartupListener; +import org.labkey.api.util.TestContext; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HttpView; @@ -260,12 +276,13 @@ public ActionURL getAuditUrl() */ private static Predicate inScope(User user, Container container, @Nullable ContainerFilter containerFilter) { - Collection ids = (null == containerFilter ? ContainerFilter.current(container, user) : containerFilter).getIds(); + ContainerFilter cf = null == containerFilter ? ContainerFilter.current(container, user) : containerFilter; + Collection ids = scopeIds(user, container, cf); if (null == ids) return event -> true; Set scope = new HashSet<>(ids); - Set childTypes = null == containerFilter ? Collections.emptySet() : containerFilter.getIncludedChildTypes(); + Set childTypes = cf.getIncludedChildTypes(); return event -> { Container c = event.getContainer(); @@ -278,6 +295,21 @@ private static Predicate inScope(User user, Container container, }; } + /** + * Scopes by CanSeeAuditLogPermission the way {@link org.labkey.api.audit.query.DefaultAuditTypeTable} does, not by + * the ReadPermission that {@link ContainerFilter#getIds()} applies, so a folder the user can read but whose audit + * log they cannot see stays out of scope. + */ + private static @Nullable Collection scopeIds(User user, Container container, ContainerFilter cf) + { + if (!(cf instanceof ContainerFilter.ContainerFilterWithPermission cfp)) + return cf.getIds(); + + Set roles = SecurityManager.canSeeAuditLog(user) ? RoleManager.roleSet(CanSeeAuditLogRole.class) : null; + + return cfp.generateIds(container, CanSeeAuditLogPermission.class, roles); + } + public record TransactionRowIds(List rowIds, Map dataTypeRowCounts) {} public TransactionRowIds getTransactionSampleIds(long transactionAuditId, User user, Container container, @Nullable ContainerFilter containerFilter) @@ -346,4 +378,134 @@ else if (newRecord.containsKey("LSID") && !StringUtils.isEmpty(newRecord.get("LS } return new TransactionRowIds(sourceIds, dataTypeRowCounts); } + + /** + * {@link #inScope} reproduces {@link org.labkey.api.audit.query.DefaultAuditTypeTable}'s container filtering in + * memory, so the branch that reads cached transaction events has to agree with the branch that reads them back from + * the database. Two cases separate the two: a workbook, which is in scope only through its parent via + * {@link ContainerFilter#getIncludedChildTypes()}, and a user who can read a folder but not its audit log. + */ + public static class TransactionScopeTestCase extends Assert + { + private static final String PROJECT_NAME = "AuditTransactionScopeTest Project"; + private static final long SAMPLE_TYPE_ID = 4242; + + private static User _user; + private static Container _project; + private static Container _workbook; + private static Container _subfolder; + + @BeforeClass + public static void setup() + { + Assume.assumeTrue("The SampleTimelineEvent provider is registered by the experiment module", + null != AuditLogService.get().getAuditProvider(SampleTimelineAuditEvent.EVENT_TYPE)); + + _user = TestContext.get().getUser(); + + deleteTestContainer(); + _project = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_NAME, _user); + _workbook = ContainerManager.createContainer(_project, null, "Workbook", null, WorkbookContainerType.NAME, _user); + _subfolder = ContainerManager.createContainer(_project, "Subfolder", _user); + } + + @AfterClass + public static void cleanup() + { + _subfolder = null; + _workbook = null; + _project = null; + _user = null; + + deleteTestContainer(); + } + + private static void deleteTestContainer() + { + Container project = ContainerManager.getForPath(PROJECT_NAME); + + if (null != project) + ContainerManager.deleteAll(project, TestContext.get().getUser()); + } + + /** Unique per call, and from the sequence production draws transaction ids from. */ + private static long newTransactionId() + { + return AbstractQueryUpdateService.createTransactionAuditEvent(_project, QueryService.AuditAction.INSERT).getRowId(); + } + + private static SampleTimelineAuditEvent timelineEvent(Container c, long transactionId, long sampleId) + { + SampleTimelineAuditEvent event = new SampleTimelineAuditEvent(c, "Sample inserted"); + event.setTransactionId(transactionId); + event.setSampleId(sampleId); + event.setSampleTypeId(SAMPLE_TYPE_ID); + + return event; + } + + /** One event per container, all in one transaction. Written with the cache on, so both branches see them. */ + private static long writeEvents(User user, long projectSampleId, long workbookSampleId, long subfolderSampleId) + { + long transactionId = newTransactionId(); + + AuditLogService.get().addEvents(user, List.of( + timelineEvent(_project, transactionId, projectSampleId), + timelineEvent(_workbook, transactionId, workbookSampleId), + timelineEvent(_subfolder, transactionId, subfolderSampleId)), true); + + assertFalse("expected addEvents() to populate the transaction event cache", + TRANSACTION_EVENT_CACHE.get(transactionId).second.isEmpty()); + + return transactionId; + } + + /** Emptying the cache entry is what sends getTransactionSampleIds() to the database. */ + private static TransactionRowIds fromDatabase(long transactionId, User user) + { + TRANSACTION_EVENT_CACHE.get(transactionId).second.clear(); + + return get().getTransactionSampleIds(transactionId, user, _project, null); + } + + @Test + public void workbookIsInScopeForBothBranches() + { + long transactionId = writeEvents(_user, 101, 102, 103); + + TransactionRowIds cached = get().getTransactionSampleIds(transactionId, _user, _project, null); + TransactionRowIds database = fromDatabase(transactionId, _user); + + assertEquals("the workbook is in scope through its parent, the subfolder is not in scope at all", + Set.of(101L, 102L), Set.copyOf(cached.rowIds())); + assertEquals("cached and database branches disagreed on sample ids", + Set.copyOf(cached.rowIds()), Set.copyOf(database.rowIds())); + assertEquals("cached and database branches disagreed on row counts", + Map.of(SAMPLE_TYPE_ID, 2L), cached.dataTypeRowCounts()); + assertEquals("cached and database branches disagreed on row counts", + cached.dataTypeRowCounts(), database.dataTypeRowCounts()); + } + + @Test + public void auditLogPermissionIsAppliedByBothBranches() + { + // Read alone is not enough to see audit events; the production caller elevates with CanSeeAuditLogRole first + User reader = new LimitedUser(_user, ReaderRole.class); + assertTrue("expected the reader to be able to read the project", _project.hasPermission(reader, ReadPermission.class)); + assertFalse("expected the reader to lack the audit log permission", _project.hasPermission(reader, CanSeeAuditLogPermission.class)); + + long transactionId = writeEvents(_user, 201, 202, 203); + + TransactionRowIds cached = get().getTransactionSampleIds(transactionId, reader, _project, null); + TransactionRowIds database = fromDatabase(transactionId, reader); + + assertEquals("a reader without the audit log permission should see no events", List.of(), cached.rowIds()); + assertEquals("cached and database branches disagreed on sample ids", cached.rowIds(), database.rowIds()); + + // The same events are visible to a user who can see the audit log, so the empty results above are the + // permission check and not a fixture that failed to write + assertEquals("expected the admin to see the events the reader could not", + Set.of(201L, 202L), Set.copyOf(get().getTransactionSampleIds(transactionId, _user, _project, null).rowIds())); + } + } } diff --git a/audit/src/org/labkey/audit/AuditModule.java b/audit/src/org/labkey/audit/AuditModule.java index 99f7572b177..15885d1f5e3 100644 --- a/audit/src/org/labkey/audit/AuditModule.java +++ b/audit/src/org/labkey/audit/AuditModule.java @@ -84,6 +84,14 @@ public void doStartup(ModuleContext moduleContext) AuditController.registerAdminConsoleLinks(); } + @Override + public @NotNull Set> getIntegrationTests() + { + return Set.of( + AuditLogImpl.TransactionScopeTestCase.class + ); + } + @Override @NotNull public Set getSchemaNames() diff --git a/experiment/src/org/labkey/experiment/CustomPropertiesView.java b/experiment/src/org/labkey/experiment/CustomPropertiesView.java index 7c010a71e36..a8846b76098 100644 --- a/experiment/src/org/labkey/experiment/CustomPropertiesView.java +++ b/experiment/src/org/labkey/experiment/CustomPropertiesView.java @@ -27,8 +27,8 @@ import org.labkey.api.exp.PropertyType; import org.labkey.api.exp.property.Domain; import org.labkey.api.exp.property.DomainProperty; +import org.labkey.api.exp.query.ExpMaterialTable; import org.labkey.api.exp.query.SamplesSchema; -import org.labkey.api.query.FieldKey; import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; @@ -155,7 +155,7 @@ public CustomPropertiesView(ExpMaterialImpl m, Container c, User u) propertyUris.add(property.getPropertyURI()); } - SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("lsid"), parentLSID); + SimpleFilter filter = new SimpleFilter(ExpMaterialTable.Column.RowId.fieldKey(), m.getRowId()); Map tableProps = new TableSelector(queryTable, filter, null).getMap(); // include calculated fields from the domain / query as well List cols = queryTable.getColumns().stream() diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 4ce12b13dc6..29a93f1bf38 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -1369,11 +1369,15 @@ private _MaterializedQueryHelper getOrCreateMQH() MaterializedQueryHelper.Builder builder = new _MaterializedQueryHelper.Builder(_ss.getLSID(), "", getExpSchema().getDbSchema().getScope(), viewSql) .updateColumns(updateColumns) .unlogged(true) + // RowId and Container are used in many places so ensure they're created before use. Other indices can + // be added as a followup step .addIndex("CREATE UNIQUE INDEX uq_${NAME}_rowid ON temp.${NAME} (rowid)") .addIndex("CREATE INDEX idx_${NAME}_container ON temp.${NAME} (container)") - .addIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)"); + .addDeferredIndex("CREATE INDEX idx_${NAME}_root ON temp.${NAME} (rootmaterialrowid)") + // Deferred despite being UNIQUE. Source data guarantees uniqueness, and this is very expensive to build + .addDeferredIndex("CREATE UNIQUE INDEX uq_${NAME}_lsid ON temp.${NAME} (lsid)"); - getDomainIndexDdl().forEach(builder::addIndex); + getDomainIndexDdl().forEach(builder::addDeferredIndex); if (isIncrementalUpdateDisabled()) builder.addInvalidCheck(() -> String.valueOf(getInvalidateCounters(_ss.getLSID()).update.get())); @@ -1407,9 +1411,11 @@ private List getDomainIndexDdl() List ddl = new ArrayList<>(); Set distinctColumnLists = new HashSet<>(); - // Seeded with the kind's own indices (rowid, name); the materialized table already indexes those. + // Seeded with the provisioned columns the builder already indexes above: the kind's own indices (rowid, name) + // and lsid. Its other indices are over exp.material columns, which cannot appear in a provisioned index. for (PropertyStorageSpec.Index index : kind.getPropertyIndices(domain)) distinctColumnLists.add(indexKey(Arrays.asList(index.translateToStorageNames(domain).columnNames))); + distinctColumnLists.add(indexKey(List.of("lsid"))); for (TableInfo.IndexDefinition index : StorageProvisioner.get().getSchemaTableInfo(domain).getAllIndices()) { @@ -1521,7 +1527,7 @@ public Builder updateColumns(List updateColumns) @Override public _MaterializedQueryHelper build() { - return new _MaterializedQueryHelper(_lsid, _updateColumns, _prefix, _scope, _select, _uptodate, _supplier, _indexes, _max, _isSelectInto, _unlogged); + return new _MaterializedQueryHelper(_lsid, _updateColumns, _prefix, _scope, _select, _uptodate, _supplier, _indexes, _deferredIndexes, _max, _isSelectInto, _unlogged); } } @@ -1534,12 +1540,13 @@ public _MaterializedQueryHelper build() @Nullable SQLFragment uptodate, Supplier supplier, @Nullable Collection indexes, + @Nullable Collection deferredIndexes, long maxTimeToCache, boolean isSelectIntoSql, boolean unlogged ) { - super(prefix, scope, select, uptodate, supplier, indexes, maxTimeToCache, isSelectIntoSql, unlogged); + super(prefix, scope, select, uptodate, supplier, indexes, deferredIndexes, maxTimeToCache, isSelectIntoSql, unlogged); this._lsid = lsid; this._updateColumns = updateColumns; } @@ -1566,6 +1573,14 @@ protected void incrementalUpdateBeforeSelect(Materialized m) if (Materialized.LoadingState.ERROR == materialized._loadingState.get()) throw materialized._loadException; + // The lock is held by a full rebuild (including its deferred indexes). Leave the counters untouched so + // readers stay on the live query and the next materializeAsync retries, rather than racing that rebuild. + if (!lockAcquired) + { + _log.info("Skipping incremental update of {}; a rebuild still holds the lock.", getMaterializationName()); + return; + } + runIncremental("delete", materialized.incrementalDeleteCheck, this::executeIncrementalDelete); runIncremental("update", materialized.incrementalUpdateCheck, this::executeIncrementalUpdate); runIncremental("rollup", materialized.incrementalRollupCheck, this::executeIncrementalRollup); diff --git a/list/src/org/labkey/list/model/ListQueryUpdateService.java b/list/src/org/labkey/list/model/ListQueryUpdateService.java index 4a2a3121e01..1a2e35ab321 100644 --- a/list/src/org/labkey/list/model/ListQueryUpdateService.java +++ b/list/src/org/labkey/list/model/ListQueryUpdateService.java @@ -257,10 +257,15 @@ void flush() @Override public void close() { - flush(); - - if (this == _auditBatch) - _auditBatch = null; + try + { + flush(); + } + finally + { + if (this == _auditBatch) + _auditBatch = null; + } } } diff --git a/study/src/org/labkey/study/model/DatasetDefinition.java b/study/src/org/labkey/study/model/DatasetDefinition.java index 7ccaa672ffc..7f9c0d629a8 100644 --- a/study/src/org/labkey/study/model/DatasetDefinition.java +++ b/study/src/org/labkey/study/model/DatasetDefinition.java @@ -1792,7 +1792,7 @@ public void addAuditEvent(User user, Container c, TableInfo table, @Nullable Aud // note switched order (oldRecord, newRecord) var event = createDetailedAuditRecord(user, c, (AuditConfigurable)table, action, userComment, row, existingRow, null, List.of()); batch.add(event); - if (batch.size() > 1000) + if (batch.size() > AbstractAuditHandler.AUDIT_BATCH_SIZE) { auditLog.addEvents(user, batch); batch.clear(); From 61334f24f2e738abc42872ab5136a945a8080d1d Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 4 Sep 2026 11:55:29 -0700 Subject: [PATCH 09/10] Resolve conflict --- .../api/data/MaterializedQueryHelper.java | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/api/src/org/labkey/api/data/MaterializedQueryHelper.java b/api/src/org/labkey/api/data/MaterializedQueryHelper.java index 7ee88a1351f..a88240a71ba 100644 --- a/api/src/org/labkey/api/data/MaterializedQueryHelper.java +++ b/api/src/org/labkey/api/data/MaterializedQueryHelper.java @@ -215,21 +215,18 @@ boolean load(SQLFragment selectQuery, boolean isSelectInto) */ private void createIndexes(List indexes, boolean tolerateFailure) { - try (var ignored = SpringActionController.ignoreSqlUpdates()) + for (String index : indexes) { - for (String index : indexes) + String sql = StringUtils.replace(index, "${NAME}", _tableName); + try { - String sql = StringUtils.replace(index, "${NAME}", _tableName); - try - { - new SqlExecutor(_mqh._scope).execute(sql); - } - catch (RuntimeException x) - { - if (!tolerateFailure) - throw x; - LOG.error("Failed to create index on materialized table {}. The table is serving queries without it. DDL: {}", _tableName, sql, x); - } + new SqlExecutor(_mqh._scope).execute(sql); + } + catch (RuntimeException x) + { + if (!tolerateFailure) + throw x; + LOG.error("Failed to create index on materialized table {}. The table is serving queries without it. DDL: {}", _tableName, sql, x); } } } From 0058122a0ccc4cf00108a1d06460561be016f8c2 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Fri, 4 Sep 2026 15:05:26 -0700 Subject: [PATCH 10/10] TTL and minor code review feedback --- audit/src/org/labkey/audit/AuditLogImpl.java | 3 ++- .../src/org/labkey/experiment/api/ExpMaterialTableImpl.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/audit/src/org/labkey/audit/AuditLogImpl.java b/audit/src/org/labkey/audit/AuditLogImpl.java index 8d177fd2d8f..6b87feda48a 100644 --- a/audit/src/org/labkey/audit/AuditLogImpl.java +++ b/audit/src/org/labkey/audit/AuditLogImpl.java @@ -279,8 +279,9 @@ private static Predicate inScope(User user, Container container, ContainerFilter cf = null == containerFilter ? ContainerFilter.current(container, user) : containerFilter; Collection ids = scopeIds(user, container, cf); if (null == ids) - return event -> true; + return _ -> true; + // Ensure an O(1) lookup Set scope = new HashSet<>(ids); Set childTypes = cf.getIncludedChildTypes(); diff --git a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java index 29a93f1bf38..6ff9ce9f4f0 100644 --- a/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExpMaterialTableImpl.java @@ -1265,7 +1265,7 @@ void recordPendingUpdate(@NotNull Timestamp changedSince) } } - static final BlockingCache _materializedQueries = CacheManager.getBlockingStringKeyCache(CacheManager.UNLIMITED, 12 * CacheManager.HOUR, "materialized sample types", null); + static final BlockingCache _materializedQueries = CacheManager.getBlockingStringKeyCache(CacheManager.UNLIMITED, 8 * CacheManager.DAY, "materialized sample types", null); static final Map _invalidationCounters = Collections.synchronizedMap(new HashMap<>()); static final AtomicBoolean initializedListeners = new AtomicBoolean(false);