Repository navigation
Speed up bulk sample deletes and the follow-up materialized view refresh - #8145
Conversation
…resh Batched delete lookups match names exactly and fail when a name matches more than one sample, as the per-key lookup did. Callers that delete exp.material directly can now signal the materialized view, since a later delete no longer reconciles the whole table.
| } | ||
|
|
||
| /** Batched {@link #getMaterialMap(Map)}: one result per key, in key order, null where no sample matched. */ | ||
| private List<Map<String, Object>> getMaterialMaps(List<Map<String, Object>> keys) throws QueryUpdateServiceException |
There was a problem hiding this comment.
I don't love this, but since we don't guarantee callers supply the same fields for every incoming row, I don't see a significantly better approach.
There was a problem hiding this comment.
Yes, I was going to mention the same thing Xing said.
There was a problem hiding this comment.
I think Xing deleted her comment. The implementation already breaks the incoming rows into RowId/LSID/Name chunks and does a bulk query for each grouping (if present). If the client includes RowId for each row, we'll do a single IN query for the whole bunch.
My concern is the clunkiness around all the map handling.
There was a problem hiding this comment.
Strange... I saw the comment posted twice so I deleted one that's not attached to this discussion. Anyhow, my original comment was partition the rows based on keys in rows, similar to AbstractQueryUpdateService.updateRowsUsingPartitionedDIB
There was a problem hiding this comment.
My concern is the clunkiness around all the map handling.
This was the concern for updateRowsUsingPartitionedDIB as well so it partitions based on map keys.
There was a problem hiding this comment.
I could use a similar strategy to replace the loop over the incoming key maps, but I'd still have to check each partition to see if it has RowId/LSID/Name&MaterialSourceId so I don't think it would make the code any leaner here. Let me know if I'm missing something.
| if (!untrackedDeletes.contains(material.getCpasType())) | ||
| { | ||
| Set<Long> ids = deletedRowIds.computeIfAbsent(material.getCpasType(), (_) -> new LongHashSet()); | ||
| ids.add(material.getRowId()); |
There was a problem hiding this comment.
This set is built from the OFFSET-paged enumeration, but the delete that actually happens is DELETE FROM exp.Material WHERE <materialFilterSQL> at :5135. Those are not the same rows.
deleteRunsUsingInputs at :5036 runs inside this loop and deletes material rows, not just runs — deleteExperimentRuns → deleteRun :3839 → ExpRunImpl.deleteProtocolApplications :557 → deleteRunMaterials :636 → getExpMaterialsForRun (exp.material WHERE RunId = ?) → ExpMaterialImpl.delete :379. The runs reached via getDeletableSourceRunsFromInputRowId at :5230 are by construction ones whose outputs are all in rawMaterials — the current batch, at or below the current paging position. Removing K of them makes the next OFFSET count query at :4959 land K rows further along, so K rows are never enumerated here yet are still deleted at :5135. (moreBatches = rawMaterials.size() == maxBatch at :4962 can end the loop early for the same reason.)
Those rows then get no entry in this set and none in sampleTypeAliquotRoots at :5018. Both were latent before, because every delete triggered the full EXCEPT reconcile; now the targeted pass removes only what this set holds, so they survive in the materialized view. The rollup miss is the worse of the two — a skipped root means a wrong AliquotCount written into exp.material itself, which neither pass will correct, since both treat exp.material as the source of truth.
Worth sourcing the ids from the delete itself instead: DELETE FROM exp.Material WHERE <filter> RETURNING RowId, CpasType, RootMaterialRowId at :5135 would feed this set and the rollup roots from one authoritative source, and would make the returned count correct too.
Runs reached via getDerivedRunsFromMaterial at :5208 are fine — their outputs need not match the filter, so they don't shift the paging, and the nested deleteMaterialByRowIds logs them into the same per-LSID DeleteRefreshRunnable.
There was a problem hiding this comment.
I switched to return the deleted RowIds from the DELETE statement as suggested. Based on my analysis, it wasn't strictly required here but it's a good idea and will make it more robust going forward.
|
@XingY the fix for the new test flakiness is now merged. |
This is still a test failure on ExpMaterialTableImpl$IncrementalUpdateTestCase |
I pushed another iteration and queued 10 runs in TeamCity. All succeeded. https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_BvtBPostgres?branch=sample_delete_perf @labkey-tchad FYI if you see failures in BVTs or other suites for ExpMaterialTableImpl$IncrementalUpdateTestCase |
## Rationale Specimen import deletes removed specimens from exp.material directly, so it must now tell the sample type's materialized view to reconcile. With LabKey/platform#8145, a later sample delete only drops the rows it logged rather than reconciling the whole table, so specimens removed by import would otherwise stay in the view. ## Related Pull Requests - LabKey/platform#8145 ## Changes - Signal a full materialized view reconcile after specimen import removes specimens
Rationale
Deleting a few thousand samples from a sample type with tens of millions of rows takes ~70s. ~40s is spent rebuilding the provisioned table and its SQL once per row, ~9 s in the orphaned-run check.
After the delete, it triggers a 20–60 s refresh of the sample type's materialized view during which grid reads fall back to the much slower live query. The materialized update scales with table size rather than with the number of rows deleted.
Related Pull Requests
Changes
Tasks