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

delete_all triggers error "relation "my_cte" does not exist" #6

Open
nbrustein opened this issue Sep 10, 2019 · 5 comments
Open

delete_all triggers error "relation "my_cte" does not exist" #6

nbrustein opened this issue Sep 10, 2019 · 5 comments

Comments

@nbrustein
Copy link
Contributor

Using ActiveRecord 5.2.3, the following code works (with the generated sql included below)

Widget.with(my_cte: "select id from widgets limit 1").joins('join my_cte using (id)').delete_all

DELETE FROM "widgets"
WHERE "widgets"."id" IN ( 
    WITH "my_cte" AS (
            SELECT id
            FROM widgets
            LIMIT 1
    )
    SELECT "widgets"."id"
    FROM "widgets"
    JOIN my_cte USING (id)
)

If I do the same thing without the join, however, it generates the error "relation "my_cte" does not exist" (with the generated sql included below)

Widget.with(my_cte: "select id from widgets limit 1").where("id in (select id from my_cte)").delete_all

DELETE FROM 
"widgets" WHERE 
(id in (select id from my_cte))

The issue seems to have something to do with the following lines in activerecord-5.2.3/lib/active_record/relation.rb#delete_all. It behaves differently if there are joins present or not:

stmt = Arel::DeleteManager.new
stmt.from(table)
if has_join_values? || has_limit_or_offset?
  @klass.connection.join_to_delete(stmt, arel, arel_attribute(primary_key))
else
  stmt.wheres = arel.constraints
end
affected = @klass.connection.delete(stmt, "#{@klass} Destroy")

Using ActiveRecord 6.0.0, the delete_all method has been completely refactored, and neither one of the approaches described above work. Both generate the error "relation "my_cte" does not exist".

Here is the relevant section of activerecord-6.0.0/lib/active_record/relation.rb#delete_all

stmt = Arel::DeleteManager.new
stmt.from(arel.join_sources.empty? ? table : arel.source)
stmt.key = arel_attribute(primary_key)
stmt.take(arel.limit)
stmt.offset(arel.offset)
stmt.order(*arel.orders)
stmt.wheres = arel.constraints

affected = @klass.connection.delete(stmt, "#{@klass} Destroy")

I'm not sure if it makes sense to think of this as a bug in this gem, or a bug in activerecord. I don't totally understand the issue, but it seems like this gem is using features that are supported in arel, but activerecord is doing things in such a way as to break those arel features. Is that right?

@kmurph73
Copy link
Owner

Hmm, yeah that's a strange edge case. Thanks for the detailed diagnosis. I'm not really sure what the proper solution is. Will have to muck around with the gem a bit. Am certainly open to ideas 🤷‍♂️

@booleanbetrayal
Copy link

@kmurph73 - is it worth trying to organize support around CTEs in the flagship ActiveRecord PostgreSQL adapter? Seems like this project would be a good reference point. I am no way familiar with the AR internals, but wouldn't mind helping to rally support / sponsor bounties / etc.

@kmurph73
Copy link
Owner

@booleanbetrayal I definitely agree that CTEs should be in AR standard since they're so powerful. I'm not sure of the best way to campaign for that... do we need to bug somebody like Aaron Patterson? I worry that throwing another GH issue onto the pile would go nowhere fast.

@simi
Copy link
Contributor

simi commented Dec 14, 2019

Hello!

ℹ️ I'm not Aaron Patterson (even I dream about that sometimes 😄), but I tried to port with to active record (for now without recursive support). I hope it will get merged. 🙏 Feel free to take a look and suggest any changes.

rails/rails#37973

@simi
Copy link
Contributor

simi commented Dec 15, 2019

I have found out the problem with delete_all/update_all and fixed it in my pull request. It needs to patch some Arel classes. I'm not sure if it is worth it to backport here.

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

No branches or pull requests

4 participants