From f63dc3b9df7a313c649d125f73d0ac6aa693514a Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 16 Oct 2021 13:36:16 +0200 Subject: [PATCH] Do not explicitly begin a new transaction when getting next_hid There is no transactional state, so it seems unncessary. Not starting an explicit transaction means sqlalchemy won't flush the current session, which means we're not committing a HDA/HDCA to the database that doesn't have a hid yet. --- lib/galaxy/managers/collections.py | 2 +- lib/galaxy/model/__init__.py | 7 +++---- lib/galaxy/model/mapping.py | 20 +++++++------------- lib/galaxy/tools/execute.py | 12 +++++------- test/unit/tools/test_history_imp_exp.py | 1 + 5 files changed, 17 insertions(+), 25 deletions(-) diff --git a/lib/galaxy/managers/collections.py b/lib/galaxy/managers/collections.py index 26a87d4d6cc..ca858d57214 100644 --- a/lib/galaxy/managers/collections.py +++ b/lib/galaxy/managers/collections.py @@ -341,7 +341,7 @@ class DatasetCollectionManager: copy_kwds["element_destination"] = parent # e.g. a history if dataset_instance_attributes is not None: copy_kwds["dataset_instance_attributes"] = dataset_instance_attributes - new_hdca = source_hdca.copy(**copy_kwds) + new_hdca = source_hdca.copy(flush=False, **copy_kwds) new_hdca.copy_tags_from(target_user=trans.get_user(), source=source_hdca) if not copy_elements: parent.add_dataset_collection(new_hdca) diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index 1b7580b6d4d..fc29148fe89 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -2566,10 +2566,9 @@ class History(Base, HasTags, Dictifiable, UsesAnnotations, HasName, RepresentByI else: hdcas = self.active_dataset_collections for hdca in hdcas: - new_hdca = hdca.copy() + new_hdca = hdca.copy(flush=False) new_history.add_dataset_collection(new_hdca, set_hid=False) db_session.add(new_hdca) - db_session.flush() if target_user: new_hdca.copy_item_annotation(db_session, self.user, hdca, target_user, new_hdca) @@ -5699,7 +5698,7 @@ class HistoryDatasetCollectionAssociation( break return matching_collection - def copy(self, element_destination=None, dataset_instance_attributes=None): + def copy(self, element_destination=None, dataset_instance_attributes=None, flush=True): """ Create a copy of this history dataset collection association. Copy underlying collection. @@ -5729,7 +5728,7 @@ class HistoryDatasetCollectionAssociation( if element_destination: element_destination.stage_addition(hdca) element_destination.add_pending_items() - else: + if flush: object_session(self).flush() return hdca diff --git a/lib/galaxy/model/mapping.py b/lib/galaxy/model/mapping.py index d4b32513a99..de6414c4ab4 100644 --- a/lib/galaxy/model/mapping.py +++ b/lib/galaxy/model/mapping.py @@ -45,19 +45,13 @@ def db_next_hid(self, n=1): """ session = object_session(self) table = self.table - trans = session.begin() - try: - if "postgres" not in session.bind.dialect.name: - next_hid = select([table.c.hid_counter], table.c.id == model.cached_id(self)).with_for_update().scalar() - table.update(table.c.id == self.id).execute(hid_counter=(next_hid + n)) - else: - stmt = table.update().where(table.c.id == model.cached_id(self)).values(hid_counter=(table.c.hid_counter + n)).returning(table.c.hid_counter) - next_hid = session.execute(stmt).scalar() - n - trans.commit() - return next_hid - except Exception: - trans.rollback() - raise + if "postgres" not in session.bind.dialect.name: + next_hid = select([table.c.hid_counter], table.c.id == model.cached_id(self)).with_for_update().scalar() + table.update(table.c.id == self.id).execute(hid_counter=(next_hid + n)) + else: + stmt = table.update().where(table.c.id == model.cached_id(self)).values(hid_counter=(table.c.hid_counter + n)).returning(table.c.hid_counter) + next_hid = session.execute(stmt).scalar() - n + return next_hid model.History._next_hid = db_next_hid # type: ignore diff --git a/lib/galaxy/tools/execute.py b/lib/galaxy/tools/execute.py index 750f6818243..d45419b7535 100644 --- a/lib/galaxy/tools/execute.py +++ b/lib/galaxy/tools/execute.py @@ -111,18 +111,16 @@ def execute(trans, tool, mapping_params, history, rerun_remap_job_id=None, colle history = execution_slice.history or history jobs_executed += 1 - if execution_slice: - # a side effect of adding datasets to a history is a commit within db_next_hid (even with flush=False). - history.add_pending_items() - else: - # Make sure collections, implicit jobs etc are flushed even if there are no precreated output datasets - trans.sa_session.flush() - if job_datasets: for job, datasets in job_datasets.items(): for dataset_instance in datasets: dataset_instance.dataset.job = job + if execution_slice: + history.add_pending_items() + # Make sure collections, implicit jobs etc are flushed even if there are no precreated output datasets + trans.sa_session.flush() + tool_id = tool.id for job in execution_tracker.successful_jobs: # Put the job in the queue if tracking in memory diff --git a/test/unit/tools/test_history_imp_exp.py b/test/unit/tools/test_history_imp_exp.py index d3f2991c94c..796c7b92561 100644 --- a/test/unit/tools/test_history_imp_exp.py +++ b/test/unit/tools/test_history_imp_exp.py @@ -516,6 +516,7 @@ def test_export_copied_objects_copied_outside_history(): other_h = model.History(name=h.name + "-other", user=h.user) sa_session.add(other_h) + sa_session.flush() hc3 = hc2.copy(element_destination=other_h) other_h.add_pending_items()