-
-
Notifications
You must be signed in to change notification settings - Fork 114
Add immutable version of ash_raise_error
function to support extensions like Citus
#620
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 immutable version of ash_raise_error
function to support extensions like Citus
#620
Conversation
|
||
def drop(2) do | ||
""" | ||
#{ash_raise_error()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😓 good catch.
So I like this, but I also hesitate to define this function automatically in everyone's databases when its only need by a few. My suggestion would be to create a new "extension" (not an Ash extension, just one of these postgres extension modules) that users can optionally add to define this function. |
3f56800
to
ab4b99a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, thanks for reminding me about those custom extensions, def. prefer this solution over adding it to ash-functions.
``` | ||
""" | ||
|
||
use AshPostgres.CustomExtension, name: "immutable_raise_error", latest_version: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be the first AshPostgres.CustomExtension
offered by AshPostgres — is this what you had in mind, or would you prefer special-casing this in the migrator like "ash-functions"
and using a string value for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to special case, "ash-functions"
existed before we had this.
lib/sql_implementation.ex
Outdated
end | ||
|
||
# Conditionally apply @impl to keep compatibility across ash_sql versions | ||
if {:immutable_errors?, 1} in AshSql.Implementation.behaviour_info(:callbacks) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is overkill? Would you prefer to just bump the AshSql dependency to the next release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's just bump it. For now, replace the dep with a dependency on ash_sql main
and we can get it merged, and then hex will make me point at a proper version before releasing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, done!
3d7fa3d
to
1ebb95b
Compare
🚀 Thank you for your contribution! 🚀 |
We use the Citus extension for sharding our Postgres database, which adds a requirement that any functions used within CASE or COALESCE expressions must be IMMUTABLE. Currently this means that we can't use error expressions at all because
ash_raise_error
is currently marked as STABLE.The current
ash_raise_error
could technically be IMMUTABLE as it meets all of the requirements. However, the postgres planner will constant-fold the function call, making all expressions that use it immediately raise an exception. To prevent this we need to make the function call dependent on the row so it can't be constant-folded.This PR adds a new function,
ash_raise_error_immutable
, that is marked IMMUTABLE and accepts a third argument that is simply ignored. This works together with a change in AshSql that adds a row-dependent argument, preventing the caching.It's opt-in only via an
AshPostgres.Repo
callback, which stores a value in the repo config. AshSql looks for that config value when building the query and decides whether to call the existingash_raise_error
function, or the newash_raise_error_immutable
.Related AshSql PR: ash-project/ash_sql#175
Contributor checklist
Leave anything that you believe does not apply unchecked.