Support calling immutable version of ash_raise_error
#175
Conversation
This improves support for Citus (distributed Postgres) where it only supports immutable functions within CASE statements. A row-dependent token is passed in as an ignored argument to prevent the query planner from constant-folding the ash_raise_error_immutable call. This is opt-in only. The repo config must have an :immutable_expr_error? entry set to true (supported by ash_postgres currently).
lib/expr.ex
Outdated
| # Returns a row-dependent token to prevent constant-folding for immutable functions, or nil if | ||
| # the repo hasn't opted-in to immutable expr_errors. | ||
| # | ||
| # The expression doesn't matter, as long as it depends on the row to avoid constant-folding. |
There was a problem hiding this comment.
I worry that the expression needs to depend on the full primary key of the row actually, otherwise one row with a matching field value could be constant-folded to not raise an error.
There was a problem hiding this comment.
I guess in that case it wouldn't be constant folding, but it could just be cached.
There was a problem hiding this comment.
I don't think it needs to based on my testing, but I actually did update this from that earlier version I showed you. This one concats all the primary key columns together in a text expression, falling back to the first attribute if there is no primary key.
I can drop the comment if you think it adds confusion?
There was a problem hiding this comment.
oh I just misread the code. Can we create an array instead of concatenating as a string? not all primary keys are guaranteed to support concatenation like that.
There was a problem hiding this comment.
Hm, I'm not sure an array would work since the primary key attributes could use different types. But how about building up a ROW of the primary keys to pass in like ROW(D0."tenant_id", D0."row_id")?
There was a problem hiding this comment.
Just pushed a change that does that; I prefer this solution, it supports keys of varying types and doesn't depend on casting to ::text
lib/expr.ex
Outdated
| # the repo hasn't opted-in to immutable expr_errors. | ||
| defp immutable_error_expr_token(query, bindings) do | ||
| resource = query.__ash_bindings__.resource | ||
| repo_config = query.__ash_bindings__.sql_behaviour.repo(resource, :read).config() |
There was a problem hiding this comment.
I'm not a fan of the way this reaches into the repo to read something set by ash_postgres although we probably do it elsewhere like this? regardless, let's add a function to the sql_behaviour that controls this, i.e sql_behaviour.immutable_errors?(repo)
There was a problem hiding this comment.
Yeah, there were a couple different ways it integrates with the repos; the config approach was a looser dependency and I wasn't sure if you'd want to add this to the behaviour. Happy to switch though! I pushed another commit that does this.
|
Awesome, only some small notes on this PR and the other PR 👍 |
stevebrambilla
force-pushed
the
immutable-raise-error
branch
from
64f3b13 to
2ac5876
Compare
|
🚀 Thank you for your contribution! 🚀 |
2 participants
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_erroris currently marked as STABLE.The current
ash_raise_errorcould 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 works together with an AshPostgres change that adds a new function,
ash_raise_error_immutable, which accepts a third argument that is expected to be a row-dependent value so it prevents the planner from caching. This is opt-in viaAshPostgres.Repo, which sets a value on the repo config.If that config value is true, then AshSql will use
ash_raise_error_immutableinstead ofash_raise_error, passing in a row-dependent value as the third argument. Otherwise it behaves exactly as before by callingash_raise_error.Related AshPostgres PR: ash-project/ash_postgres#620
Contributor checklist
Leave anything that you believe does not apply unchecked.