On Thu, Aug 22, 2024 at 12:44 PM Amit Langote <amitlangot...@gmail.com> wrote: > On Thu, Aug 22, 2024 at 11:02 jian he <jian.universal...@gmail.com> wrote: >> On Tue, Jul 30, 2024 at 12:59 PM Amit Langote <amitlangot...@gmail.com> >> wrote: >> > On Fri, Jul 26, 2024 at 11:19 PM jian he <jian.universal...@gmail.com> >> > wrote: >> > > { >> > > ... >> > > /* >> > > * For expression nodes that support soft errors. Should be set to >> > > NULL >> > > * before calling ExecInitExprRec() if the caller wants errors >> > > thrown. >> > > */ >> > > ErrorSaveContext *escontext; >> > > } ExprState; >> > > >> > > i believe by default makeNode will set escontext to NULL. >> > > So the comment should be, if you want to catch the soft errors, make >> > > sure the escontext pointing to an allocated ErrorSaveContext. >> > > or maybe just say, default is NULL. >> > > >> > > Otherwise, the original comment's meaning feels like: we need to >> > > explicitly set it to NULL >> > > for certain operations, which I believe is false? >> > >> > OK, I'll look into updating this.
See 0001. >> > > json_behavior_type: >> > > ERROR_P { $$ = JSON_BEHAVIOR_ERROR; } >> > > | NULL_P { $$ = JSON_BEHAVIOR_NULL; } >> > > | TRUE_P { $$ = JSON_BEHAVIOR_TRUE; } >> > > | FALSE_P { $$ = JSON_BEHAVIOR_FALSE; } >> > > | UNKNOWN { $$ = JSON_BEHAVIOR_UNKNOWN; } >> > > | EMPTY_P ARRAY { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; } >> > > | EMPTY_P OBJECT_P { $$ = JSON_BEHAVIOR_EMPTY_OBJECT; } >> > > /* non-standard, for Oracle compatibility only */ >> > > | EMPTY_P { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; } >> > > ; >> > > >> > > EMPTY_P behaves the same as EMPTY_P ARRAY >> > > so for function GetJsonBehaviorConst, the following "case >> > > JSON_BEHAVIOR_EMPTY:" is wrong? >> > > >> > > case JSON_BEHAVIOR_NULL: >> > > case JSON_BEHAVIOR_UNKNOWN: >> > > case JSON_BEHAVIOR_EMPTY: >> > > val = (Datum) 0; >> > > isnull = true; >> > > typid = INT4OID; >> > > len = sizeof(int32); >> > > isbyval = true; >> > > break; >> > > >> > > also src/backend/utils/adt/ruleutils.c >> > > if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY) >> > > get_json_behavior(jexpr->on_error, context, "ERROR"); >> > >> > Something like the attached makes sense? While this meaningfully >> > changes the deparsing output, there is no behavior change for >> > JsonTable top-level path execution. That's because the behavior when >> > there's an error in the execution of the top-level path is to throw it >> > or return an empty set, which is handled in jsonpath_exec.c, not >> > execExprInterp.c. See 0002. I'm also attaching 0003 to fix a minor annoyance that JSON_TABLE() columns' default ON ERROR, ON EMPTY behaviors are unnecessarily emitted in the deparsed output when the top-level ON ERROR behavior is ERROR. Will push these on Monday. I haven't had a chance to take a closer look at your patch to optimize the code in ExecInitJsonExpr() yet. -- Thanks, Amit Langote
v1-0002-SQL-JSON-Fix-default-ON-ERROR-behavior-for-JSON_T.patch
Description: Binary data
v1-0001-Update-comment-about-ExprState.escontext.patch
Description: Binary data
v1-0003-SQL-JSON-Fix-JSON_TABLE-column-deparsing.patch
Description: Binary data