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

[BLOCKED] Suggested performance improvement for adding nodes to group #1677

Merged
merged 2 commits into from
Jun 26, 2018

Conversation

muhrin
Copy link
Contributor

@muhrin muhrin commented Jun 22, 2018

I'm trying a way to add nodes to a group that doesn't require an explcit
check to see if the node is already there. There are several parts to
this:

  1. A UNIQUE constraint needs to be added to the association table as we
    have in Django. Something like:
    CREATE UNIQUE INDEX db_dbgroup_dbnodes_dbgroup_id_dbnode_id_key ON public.db_dbgroup_dbnodes (dbgroup_id, dbnode_id);
  2. Add the nodes one by one and flush after each one which will cause
    the IntegrityError to appear if the constraint is violated
  3. Wrap the append/flush in a nested session so that it is automatically
    rolled back in the case of a constraint violation
  4. finally after everything do the final commit (because flush just puts
    it in SQLs buffers)

BLOCKED: Before this gets considered for a merge we have to find a way to
put the constraint in the model. I didn't see how to do this.

I'm trying a way to add nodes to a group that doesn't require an explcit
check to see if the node is already there.  There are several parts to
this:
1) A UNIQUE constraint needs to be added to the association table as we
have in Django.  Something like:
CREATE UNIQUE INDEX db_dbgroup_dbnodes_dbgroup_id_dbnode_id_key ON public.db_dbgroup_dbnodes (dbgroup_id, dbnode_id);
2) Add the nodes one by one and flush after each one which will cause
the IntegrityError to appear if the constraint is violated
3) Wrap the append/flush in a nested session so that it is automatically
rolled back in the case of a constraint violation
4) finally after everything do the final commit (because flush just puts
it in SQLs buffers)

NOTE: Before this gets considered for a merge we have to find a way to
put the constraint in the model.  I didn't see how to do this.
@szoupanos
Copy link
Contributor

So the PR for constraint can be found at #1680

@sphuber
Copy link
Contributor

sphuber commented Jun 25, 2018

PR #1680 has been merged, so this can be updated

@codecov-io
Copy link

Codecov Report

Merging #1677 into develop will increase coverage by 0.04%.
The diff coverage is 96%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1677      +/-   ##
===========================================
+ Coverage    57.13%   57.17%   +0.04%     
===========================================
  Files          275      275              
  Lines        33919    33931      +12     
===========================================
+ Hits         19378    19399      +21     
+ Misses       14541    14532       -9
Impacted Files Coverage Δ
aiida/orm/implementation/sqlalchemy/utils.py 38.62% <100%> (+3.11%) ⬆️
aiida/orm/implementation/sqlalchemy/group.py 84.97% <94.44%> (+0.39%) ⬆️
aiida/backends/djsite/db/models.py 75.97% <0%> (+0.88%) ⬆️
aiida/backends/djsite/globalsettings.py 86.84% <0%> (+5.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbb081b...74b8e19. Read the comment docs.

@szoupanos
Copy link
Contributor

Thanks a lot Martin!

@szoupanos szoupanos merged commit b6e6880 into develop Jun 26, 2018
@giovannipizzi giovannipizzi deleted the fix_1319_group_adding_slow branch July 13, 2018 16:38
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.

4 participants