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

Add set_constraints helper for PostgreSQL #49187

Merged
merged 1 commit into from Oct 28, 2023

Conversation

ccutrer
Copy link
Contributor

@ccutrer ccutrer commented Sep 7, 2023

The docs already talk about how to set up deferrable constraints, but then rely on tthe user crafting custom SQL to actually use the feature. The helpers makes it easier to handle juggling multiple specific constraints and quoting issues.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@yahonda
Copy link
Member

yahonda commented Sep 19, 2023

@alpaca-tc What do you think about this pull request? I'm asking this question because you have authored #46192 and #47655 .

@alpaca-tc
Copy link
Contributor

alpaca-tc commented Sep 20, 2023

@yahonda I think the #set_constraints support is a good idea. I also like the method interface was nice. 👍

As for #defer_constraints, I am not sure if it should be supported in Rails. I think it would be nice to let users extend it in their own applications.

    1. I feel uncomfortable that #immediate_constraints, which does the opposite of #defer_constraints, does not exist.
    1. As in the following code, depending on the kind of exception that occurs in the #defer_constraints block, the behavior is not as expected.
begin
  Post.connection.defer_constraints do
    raise 'a'
  end
rescue
  # The user expects the constraint to be back to IMMEDIATE, but it remains DEFERRABLE.
end

@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 9, 2023

  1. As in the following code, depending on the kind of exception that occurs in the #defer_constraints block, the behavior is not as expected.

I actually went through several iterations of what to do here, before simply leaving it very simple. In most cases, any exception will end up rolling back the transaction anyway. One of my attempts was to inspect the connection's state and see if a transaction was open, and if it wasn't avoid rolling it back. But then you have to special case Rollback, to not set the deferred state, since the transaction is going to rollback anyway, it's just a wasted command. And then realized that for most other exceptions, that's also the case. And technically just because we set constraints to deferred, doesn't mean it was initially immediate, so setting it "back" to immediate may be wrong. It all seems rather complicated. In light of all that, I think it would probably be best to get rid of the defer_constraints helper, and only add set_constraints. Thoughts?

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@zzak
Copy link
Member

zzak commented Oct 13, 2023

Sorry I left some knit-picks on the ChangeLog, that are not super important. Upon thinking about this though, I think I like the idea of just adding set_constraints here, and to think about defer_constraints more after.

@alpaca-tc
Copy link
Contributor

alpaca-tc commented Oct 22, 2023

I think I like the idea of just adding set_constraints here, and to think about defer_constraints more after.

agree 👍

The docs already talk about how to set up deferrable constraints, but
then rely on the user crafting custom SQL to actually use the feature.
The helper makes it easier to handle juggling multiple specific
constraints and quoting issues.
@ccutrer ccutrer changed the title Add set_constraints and defer_constraints helpers for PostgreSQL Add set_constraints helper for PostgreSQL Oct 24, 2023
@ccutrer
Copy link
Contributor Author

ccutrer commented Oct 24, 2023

rebased, defer_constraints helper removed, and tests and changelog updated appropriately

@yahonda yahonda merged commit 6c63b9b into rails:main Oct 28, 2023
4 checks passed
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.

None yet

5 participants