From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | jian he <jian(dot)universality(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: pgsql: Add more SQL/JSON constructor functions |
Date: | 2024-09-02 08:17:52 |
Message-ID: | CA+HiwqE7e8Hms7T27vv1f=kQ9KL5SQUtdLdv9NcVRf=GSaU49w@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Aug 30, 2024 at 4:32 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Aug 22, 2024 at 12:44 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Thu, Aug 22, 2024 at 11:02 jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >> On Tue, Jul 30, 2024 at 12:59 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> >> > On Fri, Jul 26, 2024 at 11:19 PM jian he <jian(dot)universality(at)gmail(dot)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.
Didn't as there's a release freeze in effect for the v17 branch. Will
push to both master and v17 once the freeze is over.
> I haven't had a chance to take a closer look at your patch to optimize
> the code in ExecInitJsonExpr() yet.
I've simplified your patch a bit and attached it as 0004.
--
Thanks, Amit Langote
Attachment | Content-Type | Size |
---|---|---|
v2-0002-SQL-JSON-Fix-default-ON-ERROR-behavior-for-JSON_T.patch | application/octet-stream | 5.9 KB |
v2-0004-SQL-JSON-Avoid-initializing-unnecessary-ON-ERROR-.patch | application/octet-stream | 3.3 KB |
v2-0003-SQL-JSON-Fix-JSON_TABLE-column-deparsing.patch | application/octet-stream | 5.5 KB |
v2-0001-Update-comment-about-ExprState.escontext.patch | application/octet-stream | 1.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2024-09-02 08:19:39 | Re: generic plans and "initial" pruning |
Previous Message | Peter Smith | 2024-09-02 08:06:44 | Re: Introduce XID age and inactive timeout based replication slot invalidation |