Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing remaining database triggers #2230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Jan 8, 2025

Fixes Issue

Fixes #2228
Fixes #2231

Changes proposed

Moved the logic to update run_schemas and dataset_schemas into Horreum logic and as a consequence I removed the following entities:

Triggers:

  • ds_after_insert
  • rs_after_run_untrash
  • rs_after_run_update
  • before_schema_update

Functions:

  • ds_after_dataset_insert_func
  • rs_after_run_update
  • before_schema_update_func

I kept the procedure update_run_schemas in the db as it is always called by the Horreum directly.

Additionally with commit 714223f I removed the remaining triggers on Run table:

  • after_run_update_non_data, moved logic into Horreum
  • delete_run_validations, we are not even allowing Runs to be deleted so no need to keep that

With commit d18fcc7 I also removed all triggers associated to the label table:

  • lv_before_update
  • lv_after_update
  • recalc_labels

And related functions. Moreover I also deleted the label_recalc_queue table as no longer used.

Preconditions

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@lampajr lampajr changed the title Run triggers Removing run_ and dataset_ schemas triggers Jan 8, 2025
@lampajr lampajr changed the title Removing run_ and dataset_ schemas triggers Removing remaining database triggers Jan 8, 2025
Signed-off-by: Andrea Lamparelli <[email protected]>
@lampajr lampajr marked this pull request as ready for review January 10, 2025 15:38
Copy link
Member

@stalep stalep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, the only concern is if we have good enough coverage already in our testsuite with regards to label and label values updates/delete etc. I had a look at the tests and it looks like we are covering quite a bit.
Then if we see any anomalies in testing we are aware of this change and can investigate further.

@lampajr
Copy link
Member Author

lampajr commented Feb 19, 2025

Looks good, the only concern is if we have good enough coverage already in our testsuite with regards to label and label values updates/delete etc. I had a look at the tests and it looks like we are covering quite a bit.

That's a reasonable concern!

As part of the latest refactoring I improved the coverage for those logics but for sure it can be improved even further.

Then if we see any anomalies in testing we are aware of this change and can investigate further.

Totally agree on this, I also tried to document as much as I could the methods such that it should (hopefully) be a bit more easier to debug what's going on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove triggers on label table Move run_/dataset_ schemas updates into Horreum logic
2 participants