dresende / node-orm2

Object Relational Mapping
http://github.com/dresende/node-orm2
MIT License
3.07k stars 376 forks source link

ChainFind remove stack overflow #518

Open kapouer opened 10 years ago

kapouer commented 10 years ago

Hi,

owner.getPets().remove() 37000 rows from database fails on PostgreSQL with a stack overflow. I see that you get a list of ids using find(), then remove each id with a simple remove(). Isn't it possible to implement a more efficient delete (using db possibilities) ? (i might have missed some real blocker).

PS: i have a lot of kittens :)

kapouer commented 10 years ago

As a workaround i'm using db.driver.remove('pets', {owner_id: owner.id}, cb), but the question is not about finding a way around.

dxg commented 10 years ago

Reminds me of #471 but kind of opposite. Remove is called the efficient way sometimes. Other times it isn't.

What are your thoughts on the suggested fireHooks: false API?

kapouer commented 10 years ago

I'm not fond of using an optional argument. Maybe it's possible to use the existing orm semantics:

dxg commented 10 years ago

That approach sounds good.

It would be pretty easy to make the .remove code in ChainFind more efficient but it wouldn't work in all cases. The catch is that whilst node-sql-query's find can deal with join tables, remove can't. I suspect that may be why it was done the way it was.

Join tables happen when chaning off a hasMany get accessor. Will need to first modify sql-query and go from there.

The kitten genocide will have to wait.

dxg commented 10 years ago

Whilst investigating I found & fixed some other issues, as well as adding a failing test case. Just gotta find time to actually fix it.

syzer commented 10 years ago

+1