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

Fixes #1582: rebind flag in apoc.periodic.iterate #3098

Merged
merged 3 commits into from
Dec 12, 2022

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Aug 2, 2022

Fixes #1582

  • Added apoc.node.rebind / apoc.rel.rebind and apoc.any.rebind functions
  • Added rebind config to apoc.periodic.iterate proc not to rebind manually the query if needed
  • Added tests which fails without rebind via match (n) where id(n) = $id... or without apoc.<entity>.rebind function

@ncordon ncordon self-requested a review August 3, 2022 08:13
@ncordon ncordon self-assigned this Aug 3, 2022
Copy link
Contributor

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

Thanks for PR man.

We decided core should not receive new functionality unless especifically approved. Feel free to either move the functionality to full or we can discuss with the whole team whether this is worthy of being added to core and breaking that policy.

@vga91
Copy link
Collaborator Author

vga91 commented Aug 26, 2022

I just moved the new functionality in full

@jexp
Copy link
Member

jexp commented Sep 28, 2022

@ncordon We should probably discuss this with Luisa.

Since 4.x this breaking change in neo4j is a big source of problems, as transaction state from nodes that are not rebound will leak into the original transaction and make it blow up.
As you can see from the issue it was already raised July 2020 and should have been long fixed.

Copy link
Contributor

@ncordon ncordon left a comment

Choose a reason for hiding this comment

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

I don't understand why this is needed, nor we have added documentation in this PR, even though it's for 4.4 and the docs still live with the code.

Could you explain me what rebind is for, please?

@vga91
Copy link
Collaborator Author

vga91 commented Oct 5, 2022

I don't understand why this is needed, nor we have added documentation in this PR, even though it's for 4.4 and the docs still live with the code.

Could you explain me what rebind is for, please?

@ncordon
I added some docs and put an example to explain it.
For example, I could execute a CREATE (:Article {content: '1234'});,
and then:

CALL apoc.periodic.iterate('MATCH (art:Article) RETURN art',
'CREATE (node:Category) with art, node call apoc.create.relationship(art, "CATEGORY", {b: 1}, node) yield rel return rel', {});

In this case the procedure fails because the apoc.periodic.iterate open a new transaction, and inside this I create a node Category and try creating a new relationship, via org.neo4j.graphdb.Node.createRelationshipTo(),
that is in this case nodeWithLabelArticle.createRelationshipTo(nodeWithLabelCategory).

This fails (org.neo4j.graphdb.NotFoundException: Node[7] is deleted and cannot be used to create a relationship": 1)
because nodeWithLabelArticle was created outside the transaction and hold a reference to that, because of this createRelationship(..) does not recognize that node.

So we can rebind the node, via apoc.node.rebind or MATCH (n) WHERE id(n) = idNodeArticle ... to make sure that the node is recognized inside the new transaction.

@ncordon
Copy link
Contributor

ncordon commented Oct 27, 2022

Thanks for the explanation 👍. I think this makes sense, but will hold off from approving given this is now out of Cypher's Surface responsibilities as long as it's in the extended part.

@ncordon ncordon removed their assignment Oct 27, 2022
@Apollo9999
Copy link

The Code provided in the files are the Enhancement which Great features

@conker84 conker84 merged commit 4e8ac25 into neo4j-contrib:4.4 Dec 12, 2022
that is `n1.createRelationshipTo(n2)`.


Is what happens when we perform the following operation:
Copy link
Member

Choose a reason for hiding this comment

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

Actually this is not the main issue, that also didn't work before, it's regular tx-visibility and I think that will also not be resolved by rebinding.

The issue that all the periodic-iterate tx-state is attached to the originating transaction if the node is not rebound to the new updating tx, which then leads to OOM issues. @vga91

}

@UserFunction("apoc.any.rebind")
@Description("apoc.any.rebind(Object) - to rebind any rel, node, path, map, list or combination of them (i.e. executing a Transaction.getNodeById(node.getId()) / Transaction.getRelationshipById(rel.getId()))")
Copy link
Member

Choose a reason for hiding this comment

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

we should have an explicit one for path, as otherwise we lose the type information and cypher doesn't recognize it as a path anymore

vga91 added a commit that referenced this pull request Jun 5, 2023
* Fixes #1582: rebind flag in apoc.periodic.iterate

* moved in full

* Added docs
vga91 added a commit that referenced this pull request Jun 5, 2023
* Fixes #1582: rebind flag in apoc.periodic.iterate

* moved in extended

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

Successfully merging this pull request may close these issues.

rebind flag in apoc.periodic.iterate
5 participants