-
-
Notifications
You must be signed in to change notification settings - Fork 118
Citus compatibility: replace jsonb_build_object
with immutable functions when immutable_expr_error?
is true
#639
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
Citus compatibility: replace jsonb_build_object
with immutable functions when immutable_expr_error?
is true
#639
Conversation
9fb97ba
to
2a8b8e4
Compare
2a8b8e4
to
57c3f3d
Compare
query, | ||
%Ash.Query.Function.Fragment{ | ||
arguments: [ | ||
raw: "jsonb_set(", |
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 is interesting 🤔 we can't use something like jsonb_build_object? We have to build a big nested "set" expression?
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... this was the cleanest solution I found that still kept everything immutable. It's only the expression values that add to the nesting, and in Ash core it basically maxes out at 2, so in practice I don't think the nesting should be too deep.
Ideally I would have been able to write a GUC-pinned wrapper around jsonb_build_object
, but postgres doesn't allow creating sql functions that accept a VARIADIC "any"
argument, which is what's needed to forward to jsonb_build_object
, it's only allowed for C extensions.
Open to other ideas if you have any, though!
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.
I see, thanks for the explanation 🙇
SET bytea_output TO 'hex' | ||
AS $function$ | ||
BEGIN | ||
RETURN COALESCE(to_jsonb(value), 'null'::jsonb); |
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.
The coalesce here seems strange to me. This would make this differ from to_jsonb
. Can this function not return NULL
if to_jsonb(value)
is NULL
?
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 is because jsonb_set
is different than jsonb_build_object
in that you must pass in a valid jsonb
value, otherwise the whole expression resolves to NULL. jsonb_build_object
is a bit nicer here in that it accepts any
and converts database NULLs to json nulls for you.
Maybe I should rename the function to remove that to_jsonb
association though, could change it to ash_jsonb_build_value_immutable
?
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.
🤔 I think it's fine. ash_..._immutable
makes it pretty clear we're doing a specific thing here, and the longer name doesn't really make it any clearer IMO.
🚀 Thank you for your contribution! 🚀 |
The previous PR (#633) added the IMMUTABLE
ash_raise_error_immutable
function and made raising errors immutable as long as the error payload was pre-encoded in the app.If the payload contains any expression values, it takes a different code path that uses
jsonb_build_object
, which is again not-IMMUTABLE and incompatible with Citus.This PR changes the way the json error payload is built (only when the repo opts-in to using the
immutable_expr_error?
callback).It adds a new IMMUTABLE function,
ash_to_jsonb_immutable
, which pins GUCs that affect json encoding and wrapsto_jsonb
. Then usesjsonb_set
(already IMMUTABLE) calls to add expression values to the error payload.This is the last step of the ash_raise_error/citus compatibility stuff I've been working on.
I added this extension to the
TestRepo
but it only opts-in for the new tests added here.Contributor checklist
Leave anything that you believe does not apply unchecked.