Thread
Commits
-
Add SQL/JSON query functions
- 6185c9737cf4 17.0 landed
-
Add soft error handling to some expression nodes
- aaaf9449ec6b 17.0 landed
-
Adjust populate_record_field() to handle errors softly
- 1edb3b491bee 17.0 landed
-
Refactor code used by jsonpath executor to fetch variables
- faa2b953ba3b 17.0 landed
-
Code review for recent SQL/JSON commits
- 71bfd1543f8b 16.0 landed
-
Fix inconsistencies and style issues in new SQL/JSON code
- 60966f56c3e4 16.0 landed
-
Don't install postmaster symlink anymore
- 37e267335068 16.0 cited
-
Revert SQL/JSON features
- 2f2b18bd3f55 16.0 cited
-
Doc: standardize markup a bit more.
- 47046763c3ed 13.0 cited
-
SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2022-12-28T07:28:29Z
Hi, Rebased the SQL/JSON patches over the latest HEAD. I've decided to keep the same division of code into individual commits as that mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in that list into the appropriate feature commits. The main difference from the patches as they were committed into v15 is that JsonExpr evaluation no longer needs to use sub-transactions, thanks to the work done recently to handle type errors softly. I've made the new code pass an ErrorSaveContext into the type-conversion related functions as needed and also added an ExecEvalExprSafe() to evaluate sub-expressions of JsonExpr that might contain expressions that call type-conversion functions, such as CoerceViaIO contained in JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches that Nikita Glukhov had submitted in a previous discussion about redesigning SQL/JSON expression evaluation [1]. Though, I think that new interface will become unnecessary after I have finished rebasing my patches to remove subsidiary ExprStates of JsonExprState that we had also discussed back in [2]. Adding this to January CF. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/message-id/c3b315b6-1e9f-6aa4-8708-daa19cf3f1a3%40postgrespro.ru [2] https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2022-12-28T07:31:23Z
On Wed, Dec 28, 2022 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi, > > Rebased the SQL/JSON patches over the latest HEAD. I've decided to > keep the same division of code into individual commits as that > mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in > that list into the appropriate feature commits. > > The main difference from the patches as they were committed into v15 > is that JsonExpr evaluation no longer needs to use sub-transactions, > thanks to the work done recently to handle type errors softly. I've > made the new code pass an ErrorSaveContext into the type-conversion > related functions as needed and also added an ExecEvalExprSafe() to > evaluate sub-expressions of JsonExpr that might contain expressions > that call type-conversion functions, such as CoerceViaIO contained in > JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches > that Nikita Glukhov had submitted in a previous discussion about > redesigning SQL/JSON expression evaluation [1]. Though, I think that > new interface will become unnecessary after I have finished rebasing > my patches to remove subsidiary ExprStates of JsonExprState that we > had also discussed back in [2]. > > Adding this to January CF. Done. https://commitfest.postgresql.org/41/4086/ -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Elena Indrupskaya <e.indrupskaya@postgrespro.ru> — 2023-01-10T12:51:04Z
Hi, The Postgres Pro documentation team prepared another SQL/JSON documentation patch (attached), to apply on top of v1-0009-Documentation-for-SQL-JSON-features.patch. The new patch: - Fixes minor typos - Does some rewording agreed with Nikita Glukhov - Updates Docbook markup to make tags consistent across SQL/JSON documentation and across func.sgml, and in particular, consistent with the XMLTABLE function, which resembles SQL/JSON functions pretty much. -- Elena Indrupskaya Lead Technical Writer Postgres Professional http://www.postgrespro.com On 28.12.2022 10:28, Amit Langote wrote: > Hi, > > Rebased the SQL/JSON patches over the latest HEAD. I've decided to > keep the same division of code into individual commits as that > mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in > that list into the appropriate feature commits. > > The main difference from the patches as they were committed into v15 > is that JsonExpr evaluation no longer needs to use sub-transactions, > thanks to the work done recently to handle type errors softly. I've > made the new code pass an ErrorSaveContext into the type-conversion > related functions as needed and also added an ExecEvalExprSafe() to > evaluate sub-expressions of JsonExpr that might contain expressions > that call type-conversion functions, such as CoerceViaIO contained in > JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches > that Nikita Glukhov had submitted in a previous discussion about > redesigning SQL/JSON expression evaluation [1]. Though, I think that > new interface will become unnecessary after I have finished rebasing > my patches to remove subsidiary ExprStates of JsonExprState that we > had also discussed back in [2]. > > Adding this to January CF. >
-
Re: SQL/JSON revisited
Andrew Dunstan <andrew@dunslane.net> — 2023-01-10T15:03:20Z
On 2023-01-10 Tu 07:51, Elena Indrupskaya wrote: > Hi, > > The Postgres Pro documentation team prepared another SQL/JSON > documentation patch (attached), to apply on top of > v1-0009-Documentation-for-SQL-JSON-features.patch. > The new patch: > - Fixes minor typos > - Does some rewording agreed with Nikita Glukhov > - Updates Docbook markup to make tags consistent across SQL/JSON > documentation and across func.sgml, and in particular, consistent with > the XMLTABLE function, which resembles SQL/JSON functions pretty much. > That's nice, but please don't post incremental patches like this. It upsets the cfbot. (I wish there were a way to tell the cfbot to ignore patches) Also, I'm fairly certain that a good many of your changes are not according to project style. The rule as I understand it is that <parameter> is used for things that are parameters and <replaceable> is only used for things that are not parameters. (I'm not sure where that's documented other than the comment on commit 47046763c3, but it's what I attempted to do with the earlier doc tidy up.) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
-
Re: SQL/JSON revisited
Elena Indrupskaya <e.indrupskaya@postgrespro.ru> — 2023-01-11T07:02:02Z
Tags in the patch follow the markup of the XMLTABLE function: <function>XMLTABLE</function> ( <optional> <literal>XMLNAMESPACES</literal> ( <replaceable>namespace_uri</replaceable> <literal>AS</literal> <replaceable>namespace_name</replaceable> <optional>, ...</optional> ), </optional> <replaceable>row_expression</replaceable> <literal>PASSING</literal> <optional><literal>BY</literal> {<literal>REF</literal>|<literal>VALUE</literal>}</optional> <replaceable>document_expression</replaceable> <optional><literal>BY</literal> {<literal>REF</literal>|<literal>VALUE</literal>}</optional> <literal>COLUMNS</literal> <replaceable>name</replaceable> { <replaceable>type</replaceable> <optional><literal>PATH</literal> <replaceable>column_expression</replaceable></optional> <optional><literal>DEFAULT</literal> <replaceable>default_expression</replaceable></optional> <optional><literal>NOT NULL</literal> | <literal>NULL</literal></optional> | <literal>FOR ORDINALITY</literal> } <optional>, ...</optional> ) <returnvalue>setof record</returnvalue> In the above, as well as in the signatures of SQL/JSON functions, there are no exact parameter names; otherwise, they should have been followed by the <type> tag, which is not the case. There are no parameter names in the functions' code either. Therefore, <replaceable> tags seem more appropriate, according to the comment to commit 47046763c3. Sorry for upsetting your bot. :( -- Elena Indrupskaya Lead Technical Writer Postgres Professional http://www.postgrespro.com > On 2023-01-10 Tu 07:51, Elena Indrupskaya wrote: >> Hi, >> >> The Postgres Pro documentation team prepared another SQL/JSON >> documentation patch (attached), to apply on top of >> v1-0009-Documentation-for-SQL-JSON-features.patch. >> The new patch: >> - Fixes minor typos >> - Does some rewording agreed with Nikita Glukhov >> - Updates Docbook markup to make tags consistent across SQL/JSON >> documentation and across func.sgml, and in particular, consistent with >> the XMLTABLE function, which resembles SQL/JSON functions pretty much. >> > That's nice, but please don't post incremental patches like this. It > upsets the cfbot. (I wish there were a way to tell the cfbot to ignore > patches) > > Also, I'm fairly certain that a good many of your changes are not > according to project style. The rule as I understand it is that > <parameter> is used for things that are parameters and <replaceable> is > only used for things that are not parameters. (I'm not sure where that's > documented other than the comment on commit 47046763c3, but it's what I > attempted to do with the earlier doc tidy up.) > > > cheers > > > andrew > > -- > Andrew Dunstan > EDB: https://www.enterprisedb.com > -
Re: SQL/JSON revisited
John Naylor <john.naylor@enterprisedb.com> — 2023-01-11T07:18:21Z
On Wed, Jan 11, 2023 at 2:02 PM Elena Indrupskaya < e.indrupskaya@postgrespro.ru> wrote: > > Sorry for upsetting your bot. :( What I do in these cases is save the incremental patch as a .txt file -- that way people can read it, but the cf bot doesn't try to launch a CI run. And if I forget that detail, well it's not a big deal, it happens sometimes. -- John Naylor EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-01-17T13:31:23Z
On Wed, Dec 28, 2022 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi, > > Rebased the SQL/JSON patches over the latest HEAD. I've decided to > keep the same division of code into individual commits as that > mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in > that list into the appropriate feature commits. > > The main difference from the patches as they were committed into v15 > is that JsonExpr evaluation no longer needs to use sub-transactions, > thanks to the work done recently to handle type errors softly. I've > made the new code pass an ErrorSaveContext into the type-conversion > related functions as needed and also added an ExecEvalExprSafe() to > evaluate sub-expressions of JsonExpr that might contain expressions > that call type-conversion functions, such as CoerceViaIO contained in > JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches > that Nikita Glukhov had submitted in a previous discussion about > redesigning SQL/JSON expression evaluation [1]. Though, I think that > new interface will become unnecessary after I have finished rebasing > my patches to remove subsidiary ExprStates of JsonExprState that we > had also discussed back in [2]. And I've just finished doing that. In the attached updated 0004, which adds the JsonExpr node, its evaluation code is now broken into ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior expression nodes that previously used ExprState for recursive evaluation. Andres didn't like the latter as previously discussed at [1]. I've also attached the patch that Elena has proposed as the patch 0011. I haven't managed to review it yet, though once I do, I'll merge it into the main documentation patch 0009. Thanks Elena. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de
-
Re: SQL/JSON revisited
vignesh C <vignesh21@gmail.com> — 2023-01-27T14:27:10Z
On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Dec 28, 2022 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > > Hi, > > > > Rebased the SQL/JSON patches over the latest HEAD. I've decided to > > keep the same division of code into individual commits as that > > mentioned in the revert commit 2f2b18bd3f, squashing fixup commits in > > that list into the appropriate feature commits. > > > > The main difference from the patches as they were committed into v15 > > is that JsonExpr evaluation no longer needs to use sub-transactions, > > thanks to the work done recently to handle type errors softly. I've > > made the new code pass an ErrorSaveContext into the type-conversion > > related functions as needed and also added an ExecEvalExprSafe() to > > evaluate sub-expressions of JsonExpr that might contain expressions > > that call type-conversion functions, such as CoerceViaIO contained in > > JsonCoercion nodes. ExecExprEvalSafe() is based on one of the patches > > that Nikita Glukhov had submitted in a previous discussion about > > redesigning SQL/JSON expression evaluation [1]. Though, I think that > > new interface will become unnecessary after I have finished rebasing > > my patches to remove subsidiary ExprStates of JsonExprState that we > > had also discussed back in [2]. > > And I've just finished doing that. In the attached updated 0004, > which adds the JsonExpr node, its evaluation code is now broken into > ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior > expression nodes that previously used ExprState for recursive > evaluation. Andres didn't like the latter as previously discussed at > [1]. > > I've also attached the patch that Elena has proposed as the patch > 0011. I haven't managed to review it yet, though once I do, I'll > merge it into the main documentation patch 0009. Thanks Elena. The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID 37e267335068059ac9bd4ec5d06b493afb4b73e8 === === applying patch ./v2-0001-Common-SQL-JSON-clauses.patch .... can't find file to patch at input line 717 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |diff --git a/src/backend/utils/misc/queryjumble.c b/src/backend/utils/misc/queryjumble.c |index 328995a7dc..2361845a62 100644 |--- a/src/backend/utils/misc/queryjumble.c |+++ b/src/backend/utils/misc/queryjumble.c -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored [1] - http://cfbot.cputube.org/patch_41_4086.log Regards, Vignesh
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-01-30T06:39:12Z
On Fri, Jan 27, 2023 at 11:27 PM vignesh C <vignesh21@gmail.com> wrote: > On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote: > > And I've just finished doing that. In the attached updated 0004, > > which adds the JsonExpr node, its evaluation code is now broken into > > ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior > > expression nodes that previously used ExprState for recursive > > evaluation. Andres didn't like the latter as previously discussed at > > [1]. > > > > I've also attached the patch that Elena has proposed as the patch > > 0011. I haven't managed to review it yet, though once I do, I'll > > merge it into the main documentation patch 0009. Thanks Elena. > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: Thanks for the heads up. Here's a rebased version. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-02-20T07:35:52Z
Hi, On Mon, Jan 30, 2023 at 3:39 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Fri, Jan 27, 2023 at 11:27 PM vignesh C <vignesh21@gmail.com> wrote: > > On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote: > > > And I've just finished doing that. In the attached updated 0004, > > > which adds the JsonExpr node, its evaluation code is now broken into > > > ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior > > > expression nodes that previously used ExprState for recursive > > > evaluation. Andres didn't like the latter as previously discussed at > > > [1]. > > > > > > I've also attached the patch that Elena has proposed as the patch > > > 0011. I haven't managed to review it yet, though once I do, I'll > > > merge it into the main documentation patch 0009. Thanks Elena. > > > > The patch does not apply on top of HEAD as in [1], please post a rebased patch: > > Thanks for the heads up. Here's a rebased version. Rebased again over queryjumble overhaul. I decided to squash what was "[PATCH v3 01/11] Common SQL/JSON clauses" into "[PATCH v3 02/11] SQL/JSON constructors", because I noticed "useless productions" warnings against its gram.y additions when building just 0001. I also looked at squashing "[PATCH v3 11/11] Proposed reworking of SQL/JSON documentaion" into "[PATCH v3 09/11] Documentation for SQL/JSON features", but didn't, again, because I am still not sure which one of <parameter> and <replaceable> is correct for the SQL/JSON function constructs. Maybe it's the latter looking at the markup for some text on [1], such as exists ( path_expression ) → boolean, but Andrew sounded doubtful about that upthread. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com [1] https://www.postgresql.org/docs/15/functions-json.html
-
Re: SQL/JSON revisited
Elena Indrupskaya <e.indrupskaya@postgrespro.ru> — 2023-02-20T09:47:54Z
Hi Amit and Andrew, Regarding not squashing [PATCH v3 11/11] Proposed reworking of SQL/JSON documentaion, here is exactly what Tom Lane wrote in the comment to commit 47046763c3: Use <parameter> consistently for things that are in fact names of parameters (including OUT parameters), reserving <replaceable> for things that aren't. Following this, <parameter> tags should be replaced with <replaceable> because the SQL/JSON functions' code does not explicitly specify those tagged variables as function parameters. Doesn't it convince you to look at the patch again? Thank you. On 20.02.2023 10:35, Amit Langote wrote: >> no parameter names in the functions' code either > Hi, > > On Mon, Jan 30, 2023 at 3:39 PM Amit Langote <amitlangote09@gmail.com> wrote: >> On Fri, Jan 27, 2023 at 11:27 PM vignesh C <vignesh21@gmail.com> wrote: >>> On Tue, 17 Jan 2023 at 19:01, Amit Langote <amitlangote09@gmail.com> wrote: >>>> And I've just finished doing that. In the attached updated 0004, >>>> which adds the JsonExpr node, its evaluation code is now broken into >>>> ExprEvalSteps to handle the subsidiary JsonCoercion and JsonBehavior >>>> expression nodes that previously used ExprState for recursive >>>> evaluation. Andres didn't like the latter as previously discussed at >>>> [1]. >>>> >>>> I've also attached the patch that Elena has proposed as the patch >>>> 0011. I haven't managed to review it yet, though once I do, I'll >>>> merge it into the main documentation patch 0009. Thanks Elena. >>> The patch does not apply on top of HEAD as in [1], please post a rebased patch: >> Thanks for the heads up. Here's a rebased version. > Rebased again over queryjumble overhaul. > > I decided to squash what was "[PATCH v3 01/11] Common SQL/JSON > clauses" into "[PATCH v3 02/11] SQL/JSON constructors", because I > noticed "useless productions" warnings against its gram.y additions > when building just 0001. > > I also looked at squashing "[PATCH v3 11/11] Proposed reworking of > SQL/JSON documentaion" into "[PATCH v3 09/11] Documentation for > SQL/JSON features", but didn't, again, because I am still not sure > which one of <parameter> and <replaceable> is correct for the SQL/JSON > function constructs. Maybe it's the latter looking at the markup for > some text on [1], such as exists ( path_expression ) → boolean, but > Andrew sounded doubtful about that upthread. > -
Re: SQL/JSON revisited
Erik Rijkers <er@xs4all.nl> — 2023-02-20T14:41:36Z
Op 20-02-2023 om 08:35 schreef Amit Langote: >> > > Rebased again over queryjumble overhaul. > Hi, But the following statement is a problem. It does not crash but it goes off, half-freezing the machine, and only comes back after fanatic Ctrl-C'ing. select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object on error); Can you have a look? Thanks, Erik Rijkers PS Log doesn't really have anything interesting: 2023-02-20 14:57:06.073 CET 1336 LOG: server process (PID 1493) was terminated by signal 9: Killed 2023-02-20 14:57:06.073 CET 1336 DETAIL: Failed process was running: select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object on error); 2023-02-20 14:57:06.359 CET 1336 LOG: terminating any other active server processes 2023-02-20 14:57:06.667 CET 1336 LOG: all server processes terminated; reinitializing 2023-02-20 14:57:11.870 CET 1556 LOG: database system was interrupted; last known up at 2023-02-20 14:44:43 CET
-
Re: SQL/JSON revisited
Andres Freund <andres@anarazel.de> — 2023-02-20T17:24:56Z
Hi, On 2023-02-20 16:35:52 +0900, Amit Langote wrote: > Subject: [PATCH v4 03/10] SQL/JSON query functions > +/* > + * Evaluate a JSON error/empty behavior result. > + */ > +static Datum > +ExecEvalJsonBehavior(JsonBehavior *behavior, bool *is_null) > +{ > + *is_null = false; > + > + switch (behavior->btype) > + { > + case JSON_BEHAVIOR_EMPTY_ARRAY: > + return JsonbPGetDatum(JsonbMakeEmptyArray()); > + > + case JSON_BEHAVIOR_EMPTY_OBJECT: > + return JsonbPGetDatum(JsonbMakeEmptyObject()); > + > + case JSON_BEHAVIOR_TRUE: > + return BoolGetDatum(true); > + > + case JSON_BEHAVIOR_FALSE: > + return BoolGetDatum(false); > + > + case JSON_BEHAVIOR_NULL: > + case JSON_BEHAVIOR_UNKNOWN: > + *is_null = true; > + return (Datum) 0; > + > + case JSON_BEHAVIOR_DEFAULT: > + /* Always handled in the caller. */ > + Assert(false); > + return (Datum) 0; > + > + default: > + elog(ERROR, "unrecognized SQL/JSON behavior %d", behavior->btype); > + return (Datum) 0; > + } > +} Does this actually need to be evaluated at expression eavluation time? Couldn't we just emit the proper constants in execExpr.c? > +/* ---------------------------------------------------------------- > + * ExecEvalJson > + * ---------------------------------------------------------------- > + */ > +void > +ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext) Pointless comment. > +{ > + JsonExprState *jsestate = op->d.jsonexpr.jsestate; > + JsonExprPreEvalState *pre_eval = &jsestate->pre_eval; > + JsonExprPostEvalState *post_eval = &jsestate->post_eval; > + JsonExpr *jexpr = jsestate->jsexpr; > + Datum item; > + Datum res = (Datum) 0; > + JsonPath *path; > + bool throwErrors = jexpr->on_error->btype == JSON_BEHAVIOR_ERROR; > + > + *op->resnull = true; /* until we get a result */ > + *op->resvalue = (Datum) 0; > + > + item = pre_eval->formatted_expr.value; > + path = DatumGetJsonPathP(pre_eval->pathspec.value); > + > + /* Reset JsonExprPostEvalState for this evaluation. */ > + memset(post_eval, 0, sizeof(*post_eval)); > + > + res = ExecEvalJsonExpr(op, econtext, path, item, op->resnull, > + !throwErrors ? &post_eval->error : NULL); > + > + *op->resvalue = res; > +} I really don't like having both ExecEvalJson() and ExecEvalJsonExpr(). There's really no way to know what which version does, just based on the name. > --- a/src/backend/parser/gram.y > +++ b/src/backend/parser/gram.y This stuff adds quite a bit of complexity to the parser. Do we realy need like a dozen new rules here? > +json_behavior_empty_array: > + EMPTY_P ARRAY { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > + /* non-standard, for Oracle compatibility only */ > + | EMPTY_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > + ; Do we really want to add random oracle compat crud here? > +/* > + * Evaluate a JSON path variable caching computed value. > + */ > +int > +EvalJsonPathVar(void *cxt, char *varName, int varNameLen, > + JsonbValue *val, JsonbValue *baseObject) Missing static? > +{ > + JsonPathVariableEvalContext *var = NULL; > + List *vars = cxt; > + ListCell *lc; > + int id = 1; > + > + if (!varName) > + return list_length(vars); > + > + foreach(lc, vars) > + { > + var = lfirst(lc); > + > + if (!strncmp(var->name, varName, varNameLen)) > + break; > + > + var = NULL; > + id++; > + } > + > + if (!var) > + return -1; > + > + /* > + * When belonging to a JsonExpr, path variables are computed with the > + * JsonExpr's ExprState (var->estate is NULL), so don't need to be computed > + * here. In some other cases, such as when the path variables belonging > + * to a JsonTable instead, those variables must be evaluated on their own, > + * without the enclosing JsonExpr itself needing to be evaluated, so must > + * be handled here. > + */ > + if (var->estate && !var->evaluated) > + { > + Assert(var->econtext != NULL); > + var->value = ExecEvalExpr(var->estate, var->econtext, &var->isnull); > + var->evaluated = true; Uh, so this continues to do recursive expression evaluation, as ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar) I'm getting grumpy here. This is wrong, has been pointed out many times. The only thing that changes is that the point of recursion is moved around. > + > +/* > + * ExecEvalExprSafe > + * > + * Like ExecEvalExpr(), though this allows the caller to pass an > + * ErrorSaveContext to declare its intenion to catch any errors that occur when > + * executing the expression, such as when calling type input functions that may > + * be present in it. > + */ > +static inline Datum > +ExecEvalExprSafe(ExprState *state, > + ExprContext *econtext, > + bool *isNull, > + Node *escontext, > + bool *error) Afaict there's no caller of this? > > +/* > + * ExecInitExprWithCaseValue > + * > + * This is the same as ExecInitExpr, except the caller passes the Datum and > + * bool pointers that it would like the ExprState.innermost_caseval > + * and ExprState.innermost_casenull, respectively, to be set to. That way, > + * it can pass an input value to evaluate the expression via a CaseTestExpr. > + */ > +ExprState * > +ExecInitExprWithCaseValue(Expr *node, PlanState *parent, > + Datum *caseval, bool *casenull) > +{ > + ExprState *state; > + ExprEvalStep scratch = {0}; > + > + /* Special case: NULL expression produces a NULL ExprState pointer */ > + if (node == NULL) > + return NULL; > + > + /* Initialize ExprState with empty step list */ > + state = makeNode(ExprState); > + state->expr = node; > + state->parent = parent; > + state->ext_params = NULL; > + state->innermost_caseval = caseval; > + state->innermost_casenull = casenull; > + > + /* Insert EEOP_*_FETCHSOME steps as needed */ > + ExecInitExprSlots(state, (Node *) node); > + > + /* Compile the expression proper */ > + ExecInitExprRec(node, state, &state->resvalue, &state->resnull); > + > + /* Finally, append a DONE step */ > + scratch.opcode = EEOP_DONE; > + ExprEvalPushStep(state, &scratch); > + > + ExecReadyExpr(state); > + > + return state; > +struct JsonTableJoinState > +{ > + union > + { > + struct > + { > + JsonTableJoinState *left; > + JsonTableJoinState *right; > + bool advanceRight; > + } join; > + JsonTableScanState scan; > + } u; > + bool is_join; > +}; A join state that unions the join member with a scan, and has a is_join field? > +/* > + * JsonTableInitOpaque > + * Fill in TableFuncScanState->opaque for JsonTable processor > + */ > +static void > +JsonTableInitOpaque(TableFuncScanState *state, int natts) > +{ > + JsonTableContext *cxt; > + PlanState *ps = &state->ss.ps; > + TableFuncScan *tfs = castNode(TableFuncScan, ps->plan); > + TableFunc *tf = tfs->tablefunc; > + JsonExpr *ci = castNode(JsonExpr, tf->docexpr); > + JsonTableParent *root = castNode(JsonTableParent, tf->plan); > + List *args = NIL; > + ListCell *lc; > + int i; > + > + cxt = palloc0(sizeof(JsonTableContext)); > + cxt->magic = JSON_TABLE_CONTEXT_MAGIC; > + > + if (ci->passing_values) > + { > + ListCell *exprlc; > + ListCell *namelc; > + > + forboth(exprlc, ci->passing_values, > + namelc, ci->passing_names) > + { > + Expr *expr = (Expr *) lfirst(exprlc); > + String *name = lfirst_node(String, namelc); > + JsonPathVariableEvalContext *var = palloc(sizeof(*var)); > + > + var->name = pstrdup(name->sval); > + var->typid = exprType((Node *) expr); > + var->typmod = exprTypmod((Node *) expr); > + var->estate = ExecInitExpr(expr, ps); > + var->econtext = ps->ps_ExprContext; > + var->mcxt = CurrentMemoryContext; > + var->evaluated = false; > + var->value = (Datum) 0; > + var->isnull = true; > + > + args = lappend(args, var); > + } > + } > + > + cxt->colexprs = palloc(sizeof(*cxt->colexprs) * > + list_length(tf->colvalexprs)); > + > + JsonTableInitScanState(cxt, &cxt->root, root, NULL, args, > + CurrentMemoryContext); > + > + i = 0; > + > + foreach(lc, tf->colvalexprs) > + { > + Expr *expr = lfirst(lc); > + > + cxt->colexprs[i].expr = > + ExecInitExprWithCaseValue(expr, ps, > + &cxt->colexprs[i].scan->current, > + &cxt->colexprs[i].scan->currentIsNull); > + > + i++; > + } > + > + state->opaque = cxt; > +} Evaluating N expressions for a json table isn't a good approach, both memory and CPU efficiency wise. Why don't you just emit the proper expression directly, insted of the CaseTestExpr stuff, that you then separately evaluate? Greetings, Andres Freund -
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-02-21T03:09:15Z
On Mon, Feb 20, 2023 at 11:41 PM Erik Rijkers <er@xs4all.nl> wrote: > Op 20-02-2023 om 08:35 schreef Amit Langote: > > Rebased again over queryjumble overhaul. > But the following statement is a problem. It does not crash but it goes > off, half-freezing the machine, and only comes back after fanatic > Ctrl-C'ing. > > select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object > on error); > > Can you have a look? Thanks for the test case. It caused ExecInterpExpr() to enter an infinite loop, which I've fixed in the attached updated version. I've also merged Elena's documentation changes; I can see that <replaceable> is more correct. Now looking at Andres' comments, though, posting a version containing a fix for the above case so Erik may continue the testing in the meantime. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-02-21T05:40:09Z
On Tue, Feb 21, 2023 at 12:09 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Mon, Feb 20, 2023 at 11:41 PM Erik Rijkers <er@xs4all.nl> wrote: > > Op 20-02-2023 om 08:35 schreef Amit Langote: > > > Rebased again over queryjumble overhaul. > > But the following statement is a problem. It does not crash but it goes > > off, half-freezing the machine, and only comes back after fanatic > > Ctrl-C'ing. > > > > select json_query(jsonb '[3,4]', '$[*]' returning bigint[] empty object > > on error); > > > > Can you have a look? > > Thanks for the test case. It caused ExecInterpExpr() to enter an > infinite loop, which I've fixed in the attached updated version. I've > also merged Elena's documentation changes; I can see that > <replaceable> is more correct. Oops, I hadn't actually done the latter. Will do when posting the next version. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-02-27T07:45:49Z
Thanks a lot for taking a look at this and sorry about the delay in response. On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote: > On 2023-02-20 16:35:52 +0900, Amit Langote wrote: > > Subject: [PATCH v4 03/10] SQL/JSON query functions > > +/* > > + * Evaluate a JSON error/empty behavior result. > > + */ > > +static Datum > > +ExecEvalJsonBehavior(JsonBehavior *behavior, bool *is_null) > > +{ > > + *is_null = false; > > + > > + switch (behavior->btype) > > + { > > + case JSON_BEHAVIOR_EMPTY_ARRAY: > > + return JsonbPGetDatum(JsonbMakeEmptyArray()); > > + > > + case JSON_BEHAVIOR_EMPTY_OBJECT: > > + return JsonbPGetDatum(JsonbMakeEmptyObject()); > > + > > + case JSON_BEHAVIOR_TRUE: > > + return BoolGetDatum(true); > > + > > + case JSON_BEHAVIOR_FALSE: > > + return BoolGetDatum(false); > > + > > + case JSON_BEHAVIOR_NULL: > > + case JSON_BEHAVIOR_UNKNOWN: > > + *is_null = true; > > + return (Datum) 0; > > + > > + case JSON_BEHAVIOR_DEFAULT: > > + /* Always handled in the caller. */ > > + Assert(false); > > + return (Datum) 0; > > + > > + default: > > + elog(ERROR, "unrecognized SQL/JSON behavior %d", behavior->btype); > > + return (Datum) 0; > > + } > > +} > > Does this actually need to be evaluated at expression eavluation time? > Couldn't we just emit the proper constants in execExpr.c? Yes, done that way in the updated patch. > > +/* ---------------------------------------------------------------- > > + * ExecEvalJson > > + * ---------------------------------------------------------------- > > + */ > > +void > > +ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext) > > Pointless comment. Removed this function altogether in favor of merging the body with ExecEvalJsonExpr(), which does have a more sensible comment > > +{ > > + JsonExprState *jsestate = op->d.jsonexpr.jsestate; > > + JsonExprPreEvalState *pre_eval = &jsestate->pre_eval; > > + JsonExprPostEvalState *post_eval = &jsestate->post_eval; > > + JsonExpr *jexpr = jsestate->jsexpr; > > + Datum item; > > + Datum res = (Datum) 0; > > + JsonPath *path; > > + bool throwErrors = jexpr->on_error->btype == JSON_BEHAVIOR_ERROR; > > + > > + *op->resnull = true; /* until we get a result */ > > + *op->resvalue = (Datum) 0; > > + > > + item = pre_eval->formatted_expr.value; > > + path = DatumGetJsonPathP(pre_eval->pathspec.value); > > + > > + /* Reset JsonExprPostEvalState for this evaluation. */ > > + memset(post_eval, 0, sizeof(*post_eval)); > > + > > + res = ExecEvalJsonExpr(op, econtext, path, item, op->resnull, > > + !throwErrors ? &post_eval->error : NULL); > > + > > + *op->resvalue = res; > > +} > > I really don't like having both ExecEvalJson() and ExecEvalJsonExpr(). There's > really no way to know what which version does, just based on the name. Yes, having two functions is no longer necessary. > > --- a/src/backend/parser/gram.y > > +++ b/src/backend/parser/gram.y > > This stuff adds quite a bit of complexity to the parser. Do we realy need like > a dozen new rules here? >> > > +json_behavior_empty_array: > > + EMPTY_P ARRAY { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > > + /* non-standard, for Oracle compatibility only */ > > + | EMPTY_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > > + ; > > Do we really want to add random oracle compat crud here? Hmm, sorry, but I haven't familiarized myself with the grammar side of things as much as I perhaps should have, so I am not sure whether a more simplified grammar would suffice for offering a standard-compliant functionality. Maybe we could take out the oracle-compatibility bit, but I'd appreciate it if someone who has been involved with SQL/JSON from the beginning can comment on the above 2 points. > > +/* > > + * Evaluate a JSON path variable caching computed value. > > + */ > > +int > > +EvalJsonPathVar(void *cxt, char *varName, int varNameLen, > > + JsonbValue *val, JsonbValue *baseObject) > > Missing static? Fixed. > > +{ > > + JsonPathVariableEvalContext *var = NULL; > > + List *vars = cxt; > > + ListCell *lc; > > + int id = 1; > > + > > + if (!varName) > > + return list_length(vars); > > + > > + foreach(lc, vars) > > + { > > + var = lfirst(lc); > > + > > + if (!strncmp(var->name, varName, varNameLen)) > > + break; > > + > > + var = NULL; > > + id++; > > + } > > + > > + if (!var) > > + return -1; > > + > > + /* > > + * When belonging to a JsonExpr, path variables are computed with the > > + * JsonExpr's ExprState (var->estate is NULL), so don't need to be computed > > + * here. In some other cases, such as when the path variables belonging > > + * to a JsonTable instead, those variables must be evaluated on their own, > > + * without the enclosing JsonExpr itself needing to be evaluated, so must > > + * be handled here. > > + */ > > + if (var->estate && !var->evaluated) > > + { > > + Assert(var->econtext != NULL); > > + var->value = ExecEvalExpr(var->estate, var->econtext, &var->isnull); > > + var->evaluated = true; > > Uh, so this continues to do recursive expression evaluation, as > ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar) > > I'm getting grumpy here. This is wrong, has been pointed out many times. The > only thing that changes is that the point of recursion is moved around. Actually, these JSON path vars, along with other sub-expressions of JsonExpr, *are* computed non-recursively as ExprEvalSteps of the JsonExprState, at least in the cases where the vars are to be computed as part of evaluating the JsonExpr itself. So, the code path you've shown above perhaps as a hypothetical doesn't really exist, though there *is* an instance where these path vars are computed *outside* the context of evaluating the parent JsonExpr, such as in JsonTableResetContextItem(). Maybe there's a cleaner way of doing that though... > > + > > +/* > > + * ExecEvalExprSafe > > + * > > + * Like ExecEvalExpr(), though this allows the caller to pass an > > + * ErrorSaveContext to declare its intenion to catch any errors that occur when > > + * executing the expression, such as when calling type input functions that may > > + * be present in it. > > + */ > > +static inline Datum > > +ExecEvalExprSafe(ExprState *state, > > + ExprContext *econtext, > > + bool *isNull, > > + Node *escontext, > > + bool *error) > > Afaict there's no caller of this? Oops, removed. This was used in a previous version of the patch that still had nested ExprStates inside JsonExprState. > > +/* > > + * ExecInitExprWithCaseValue > > + * > > + * This is the same as ExecInitExpr, except the caller passes the Datum and > > + * bool pointers that it would like the ExprState.innermost_caseval > > + * and ExprState.innermost_casenull, respectively, to be set to. That way, > > + * it can pass an input value to evaluate the expression via a CaseTestExpr. > > + */ > > +ExprState * > > +ExecInitExprWithCaseValue(Expr *node, PlanState *parent, > > + Datum *caseval, bool *casenull) > > +{ > > + ExprState *state; > > + ExprEvalStep scratch = {0}; > > + > > + /* Special case: NULL expression produces a NULL ExprState pointer */ > > + if (node == NULL) > > + return NULL; > > + > > + /* Initialize ExprState with empty step list */ > > + state = makeNode(ExprState); > > + state->expr = node; > > + state->parent = parent; > > + state->ext_params = NULL; > > + state->innermost_caseval = caseval; > > + state->innermost_casenull = casenull; > > + > > + /* Insert EEOP_*_FETCHSOME steps as needed */ > > + ExecInitExprSlots(state, (Node *) node); > > + > > + /* Compile the expression proper */ > > + ExecInitExprRec(node, state, &state->resvalue, &state->resnull); > > + > > + /* Finally, append a DONE step */ > > + scratch.opcode = EEOP_DONE; > > + ExprEvalPushStep(state, &scratch); > > + > > + ExecReadyExpr(state); > > + > > + return state; > > > +struct JsonTableJoinState > > +{ > > + union > > + { > > + struct > > + { > > + JsonTableJoinState *left; > > + JsonTableJoinState *right; > > + bool advanceRight; > > + } join; > > + JsonTableScanState scan; > > + } u; > > + bool is_join; > > +}; > > A join state that unions the join member with a scan, and has a is_join field? Yeah, I agree that's not the best form for what it is. I've replaced that with the following: +/* Structures for JSON_TABLE execution */ + +typedef enum JsonTablePlanStateType +{ + JSON_TABLE_SCAN_STATE = 0, + JSON_TABLE_JOIN_STATE +} JsonTablePlanStateType; + +typedef struct JsonTablePlanState +{ + JsonTablePlanStateType type; + + struct JsonTablePlanState *parent; + struct JsonTablePlanState *nested; +} JsonTablePlanState; + +typedef struct JsonTableScanState +{ + JsonTablePlanState plan; + + MemoryContext mcxt; + JsonPath *path; + List *args; + JsonValueList found; + JsonValueListIterator iter; + Datum current; + int ordinal; + bool currentIsNull; + bool outerJoin; + bool errorOnError; + bool advanceNested; + bool reset; +} JsonTableScanState; + +typedef struct JsonTableJoinState +{ + JsonTablePlanState plan; + + JsonTablePlanState *left; + JsonTablePlanState *right; + bool advanceRight; +} JsonTableJoinState; I considered using NodeTag but decided not to, because this stuff is local to jsonpath_exec.c. > > + * JsonTableInitOpaque > > + * Fill in TableFuncScanState->opaque for JsonTable processor > > + */ > > +static void > > +JsonTableInitOpaque(TableFuncScanState *state, int natts) > > +{ > > + JsonTableContext *cxt; > > + PlanState *ps = &state->ss.ps; > > + TableFuncScan *tfs = castNode(TableFuncScan, ps->plan); > > + TableFunc *tf = tfs->tablefunc; > > + JsonExpr *ci = castNode(JsonExpr, tf->docexpr); > > + JsonTableParent *root = castNode(JsonTableParent, tf->plan); > > + List *args = NIL; > > + ListCell *lc; > > + int i; > > + > > + cxt = palloc0(sizeof(JsonTableContext)); > > + cxt->magic = JSON_TABLE_CONTEXT_MAGIC; > > + > > + if (ci->passing_values) > > + { > > + ListCell *exprlc; > > + ListCell *namelc; > > + > > + forboth(exprlc, ci->passing_values, > > + namelc, ci->passing_names) > > + { > > + Expr *expr = (Expr *) lfirst(exprlc); > > + String *name = lfirst_node(String, namelc); > > + JsonPathVariableEvalContext *var = palloc(sizeof(*var)); > > + > > + var->name = pstrdup(name->sval); > > + var->typid = exprType((Node *) expr); > > + var->typmod = exprTypmod((Node *) expr); > > + var->estate = ExecInitExpr(expr, ps); > > + var->econtext = ps->ps_ExprContext; > > + var->mcxt = CurrentMemoryContext; > > + var->evaluated = false; > > + var->value = (Datum) 0; > > + var->isnull = true; > > + > > + args = lappend(args, var); > > + } > > + } > > + > > + cxt->colexprs = palloc(sizeof(*cxt->colexprs) * > > + list_length(tf->colvalexprs)); > > + > > + JsonTableInitScanState(cxt, &cxt->root, root, NULL, args, > > + CurrentMemoryContext); > > + > > + i = 0; > > + > > + foreach(lc, tf->colvalexprs) > > + { > > + Expr *expr = lfirst(lc); > > + > > + cxt->colexprs[i].expr = > > + ExecInitExprWithCaseValue(expr, ps, > > + &cxt->colexprs[i].scan->current, > > + &cxt->colexprs[i].scan->currentIsNull); > > + > > + i++; > > + } > > + > > + state->opaque = cxt; > > +} > > Why don't you just emit the proper expression directly, insted of the > CaseTestExpr stuff, that you then separately evaluate? I suppose you mean emitting the expression that supplies the value given by scan->current and scan->currentIsNull into the same ExprState that holds the steps for a given colvalexpr. If so, I don't really see a way of doing that given the current model of JSON_TABLE execution. The former is computed as part of TableFuncRoutine.FetchRow(scan), which sets scan.current (and currentIsNull) and the letter is computer as part of TableFuncRoutine.GetValue(scan, colnum). > Evaluating N expressions for a json table isn't a good approach, both memory > and CPU efficiency wise. Are you referring to JsonTableInitOpaque() initializing all these sub-expressions of JsonTableParent, especially colvalexprs, using N *independent* ExprStates? That could perhaps be made to work by making JsonTableParent be an expression recognized by execExpr.c, so that a single ExprState can store the steps for all its sub-expressions, much like JsonExpr is. I'll give that a try, though I wonder if the semantics of making this work in a single ExecEvalExpr() call will mismatch that of the current way, because different sub-expressions are currently evaluated under different APIs of TableFuncRoutine. In the meantime, I'm attaching a version of the patchset with a few things fixed as mentioned above. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com -
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-02-27T11:13:55Z
On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote: > > Evaluating N expressions for a json table isn't a good approach, both memory > > and CPU efficiency wise. > > Are you referring to JsonTableInitOpaque() initializing all these > sub-expressions of JsonTableParent, especially colvalexprs, using N > *independent* ExprStates? That could perhaps be made to work by > making JsonTableParent be an expression recognized by execExpr.c, so > that a single ExprState can store the steps for all its > sub-expressions, much like JsonExpr is. I'll give that a try, though > I wonder if the semantics of making this work in a single > ExecEvalExpr() call will mismatch that of the current way, because > different sub-expressions are currently evaluated under different APIs > of TableFuncRoutine. I was looking at this and realized that using N ExprStates for various subsidiary expressions is not something specific to JSON_TABLE implementation. I mean we already have bunch of ExprStates being created in ExecInitTableFuncScan(): scanstate->ns_uris = ExecInitExprList(tf->ns_uris, (PlanState *) scanstate); scanstate->docexpr = ExecInitExpr((Expr *) tf->docexpr, (PlanState *) scanstate); scanstate->rowexpr = ExecInitExpr((Expr *) tf->rowexpr, (PlanState *) scanstate); scanstate->colexprs = ExecInitExprList(tf->colexprs, (PlanState *) scanstate); scanstate->coldefexprs = ExecInitExprList(tf->coldefexprs, (PlanState *) scanstate); Or maybe you're worried about jsonpath_exec.c using so many ExprStates *privately* to put into TableFuncScanState.opaque? -- Thanks, Amit Langote EDB: http://www.enterprisedb.com -
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-02-28T11:36:33Z
On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote: > > Uh, so this continues to do recursive expression evaluation, as > > ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar) > > > > I'm getting grumpy here. This is wrong, has been pointed out many times. The > > only thing that changes is that the point of recursion is moved around. > > Actually, these JSON path vars, along with other sub-expressions of > JsonExpr, *are* computed non-recursively as ExprEvalSteps of the > JsonExprState, at least in the cases where the vars are to be computed > as part of evaluating the JsonExpr itself. So, the code path you've > shown above perhaps as a hypothetical doesn't really exist, though > there *is* an instance where these path vars are computed *outside* > the context of evaluating the parent JsonExpr, such as in > JsonTableResetContextItem(). Maybe there's a cleaner way of doing > that though... > > > > + * JsonTableInitOpaque > > > + * Fill in TableFuncScanState->opaque for JsonTable processor > > > + */ > > > +static void > > > +JsonTableInitOpaque(TableFuncScanState *state, int natts) > > > +{ > > > + JsonTableContext *cxt; > > > + PlanState *ps = &state->ss.ps; > > > + TableFuncScan *tfs = castNode(TableFuncScan, ps->plan); > > > + TableFunc *tf = tfs->tablefunc; > > > + JsonExpr *ci = castNode(JsonExpr, tf->docexpr); > > > + JsonTableParent *root = castNode(JsonTableParent, tf->plan); > > > + List *args = NIL; > > > + ListCell *lc; > > > + int i; > > > + > > > + cxt = palloc0(sizeof(JsonTableContext)); > > > + cxt->magic = JSON_TABLE_CONTEXT_MAGIC; > > > + > > > + if (ci->passing_values) > > > + { > > > + ListCell *exprlc; > > > + ListCell *namelc; > > > + > > > + forboth(exprlc, ci->passing_values, > > > + namelc, ci->passing_names) > > > + { > > > + Expr *expr = (Expr *) lfirst(exprlc); > > > + String *name = lfirst_node(String, namelc); > > > + JsonPathVariableEvalContext *var = palloc(sizeof(*var)); > > > + > > > + var->name = pstrdup(name->sval); > > > + var->typid = exprType((Node *) expr); > > > + var->typmod = exprTypmod((Node *) expr); > > > + var->estate = ExecInitExpr(expr, ps); > > > + var->econtext = ps->ps_ExprContext; > > > + var->mcxt = CurrentMemoryContext; > > > + var->evaluated = false; > > > + var->value = (Datum) 0; > > > + var->isnull = true; > > > + > > > + args = lappend(args, var); > > > + } > > > + } > > > + > > > + cxt->colexprs = palloc(sizeof(*cxt->colexprs) * > > > + list_length(tf->colvalexprs)); > > > + > > > + JsonTableInitScanState(cxt, &cxt->root, root, NULL, args, > > > + CurrentMemoryContext); > > > + > > > + i = 0; > > > + > > > + foreach(lc, tf->colvalexprs) > > > + { > > > + Expr *expr = lfirst(lc); > > > + > > > + cxt->colexprs[i].expr = > > > + ExecInitExprWithCaseValue(expr, ps, > > > + &cxt->colexprs[i].scan->current, > > > + &cxt->colexprs[i].scan->currentIsNull); > > > + > > > + i++; > > > + } > > > + > > > + state->opaque = cxt; > > > +} > > > > Why don't you just emit the proper expression directly, insted of the > > CaseTestExpr stuff, that you then separately evaluate? > > I suppose you mean emitting the expression that supplies the value > given by scan->current and scan->currentIsNull into the same ExprState > that holds the steps for a given colvalexpr. If so, I don't really > see a way of doing that given the current model of JSON_TABLE > execution. The former is computed as part of > TableFuncRoutine.FetchRow(scan), which sets scan.current (and > currentIsNull) and the letter is computer as part of > TableFuncRoutine.GetValue(scan, colnum). I looked around for another way to pass the value of evaluating one expression (JsonTableParent.path) as input to the evaluation of another (an expression in TableFunc.colvalexprs). The only thing that came to mind is to use PARAM_EXEC parameters instead of CaseTestExpr placeholders, though I'm not sure whether that is simpler or whether that would really make things better? > > Evaluating N expressions for a json table isn't a good approach, both memory > > and CPU efficiency wise. > > Are you referring to JsonTableInitOpaque() initializing all these > sub-expressions of JsonTableParent, especially colvalexprs, using N > *independent* ExprStates? That could perhaps be made to work by > making JsonTableParent be an expression recognized by execExpr.c, so > that a single ExprState can store the steps for all its > sub-expressions, much like JsonExpr is. I'll give that a try, though > I wonder if the semantics of making this work in a single > ExecEvalExpr() call will mismatch that of the current way, because > different sub-expressions are currently evaluated under different APIs > of TableFuncRoutine. Hmm, the idea to turn JSON_TABLE into a single expression turned out to be a non-starter after all, because, unlike JsonExpr, it can produce multiple values. So there must be an ExprState for computing each column of its output rows. As I mentioned in my other reply, TableFuncScanState has a list of ExprStates anyway for TableFunc.colexprs. What we could do is move the ExprStates of TableFunc.colvalexprs into TableFuncScanState instead of making that part of the JSON_TABLE opaque state, as I've done in the attached updated patch. I also found a way to not require ExecInitExprWithCaseValue() for the initialization of those expressions by moving the responsibility of passing the value of CaseTestExpr placeholder contained in those expressions to the time of evaluating the expressions rather than initialization time. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com -
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-03-06T03:18:39Z
On Tue, Feb 28, 2023 at 8:36 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote: > > > Evaluating N expressions for a json table isn't a good approach, both memory > > > and CPU efficiency wise. > > > > Are you referring to JsonTableInitOpaque() initializing all these > > sub-expressions of JsonTableParent, especially colvalexprs, using N > > *independent* ExprStates? That could perhaps be made to work by > > making JsonTableParent be an expression recognized by execExpr.c, so > > that a single ExprState can store the steps for all its > > sub-expressions, much like JsonExpr is. I'll give that a try, though > > I wonder if the semantics of making this work in a single > > ExecEvalExpr() call will mismatch that of the current way, because > > different sub-expressions are currently evaluated under different APIs > > of TableFuncRoutine. > > Hmm, the idea to turn JSON_TABLE into a single expression turned out > to be a non-starter after all, because, unlike JsonExpr, it can > produce multiple values. So there must be an ExprState for computing > each column of its output rows. As I mentioned in my other reply, > TableFuncScanState has a list of ExprStates anyway for > TableFunc.colexprs. What we could do is move the ExprStates of > TableFunc.colvalexprs into TableFuncScanState instead of making that > part of the JSON_TABLE opaque state, as I've done in the attached > updated patch. Here's another version in which I've also moved the ExprStates of PASSING args into TableFuncScanState instead of keeping them in JSON_TABLE opaque state. That means all the subsidiary ExprStates of TableFuncScanState are now initialized only once during ExecInitTableFuncScan(). Previously, JSON_TABLE related ones would be initialized on every call of JsonTableInitOpaque(). I've also done some cosmetic changes such as renaming the JsonTableContext to JsonTableParseContext in parse_jsontable.c and to JsonTableExecContext in jsonpath_exec.c. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Andrew Dunstan <andrew@dunslane.net> — 2023-03-08T21:40:05Z
On 2023-03-05 Su 22:18, Amit Langote wrote: > On Tue, Feb 28, 2023 at 8:36 PM Amit Langote <amitlangote09@gmail.com> wrote: >> On Mon, Feb 27, 2023 at 4:45 PM Amit Langote <amitlangote09@gmail.com> wrote: >>> On Tue, Feb 21, 2023 at 2:25 AM Andres Freund <andres@anarazel.de> wrote: >>>> Evaluating N expressions for a json table isn't a good approach, both memory >>>> and CPU efficiency wise. >>> Are you referring to JsonTableInitOpaque() initializing all these >>> sub-expressions of JsonTableParent, especially colvalexprs, using N >>> *independent* ExprStates? That could perhaps be made to work by >>> making JsonTableParent be an expression recognized by execExpr.c, so >>> that a single ExprState can store the steps for all its >>> sub-expressions, much like JsonExpr is. I'll give that a try, though >>> I wonder if the semantics of making this work in a single >>> ExecEvalExpr() call will mismatch that of the current way, because >>> different sub-expressions are currently evaluated under different APIs >>> of TableFuncRoutine. >> Hmm, the idea to turn JSON_TABLE into a single expression turned out >> to be a non-starter after all, because, unlike JsonExpr, it can >> produce multiple values. So there must be an ExprState for computing >> each column of its output rows. As I mentioned in my other reply, >> TableFuncScanState has a list of ExprStates anyway for >> TableFunc.colexprs. What we could do is move the ExprStates of >> TableFunc.colvalexprs into TableFuncScanState instead of making that >> part of the JSON_TABLE opaque state, as I've done in the attached >> updated patch. > Here's another version in which I've also moved the ExprStates of > PASSING args into TableFuncScanState instead of keeping them in > JSON_TABLE opaque state. That means all the subsidiary ExprStates of > TableFuncScanState are now initialized only once during > ExecInitTableFuncScan(). Previously, JSON_TABLE related ones would be > initialized on every call of JsonTableInitOpaque(). > > I've also done some cosmetic changes such as renaming the > JsonTableContext to JsonTableParseContext in parse_jsontable.c and to > JsonTableExecContext in jsonpath_exec.c. > > Hi, I have just spent some time going through the first five patches (i.e. those that precede the JSONTABLE patches) and Andres's comments in <https://postgr.es/m/20230220172456.q3oshnvfk3wyhm5l@awork3.anarazel.de> AFAICT there are only two possible matters of concern that remain, both regarding the grammar. First is this general complaint: > This stuff adds quite a bit of complexity to the parser. Do we realy need like > a dozen new rules here? I mentioned that more than a year ago, I think, without anybody taking the matter up, so I didn't pursue it. I guess I should have. There are probably some fairly easy opportunities to reduce the number of non-terminals introduced here (e.g. I think json_aggregate_func could possibly be expanded in place without introducing json_object_aggregate_constructor and json_array_aggregate_constructor). I'm going to make an attempt at that, at least to pick some low hanging fruit. But in the end I think we are going to be left with a significant expansion of the grammar rules, more or less forced on us by the way the SQL Standards Committee rather profligately invents new ways of contorting the grammar. Second is this complaint: > +json_behavior_empty_array: > + EMPTY_P ARRAY { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > + /* non-standard, for Oracle compatibility only */ > + | EMPTY_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > + ; > Do we really want to add random oracle compat crud here? > I think this case is pretty harmless, and it literally involves one line of code, so I'm inclined to leave it. These both seem like things not worth holding up progress for, and I think it would be good to get these patches committed as soon as possible. My intention is to commit them (after some grammar adjustments) plus their documentation in the next few days. That would leave the JSONTABLE patches still to go. They are substantial, but a far more manageable chunk of work for some committer (not me) once we get this foundational piece in. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com -
Re: SQL/JSON revisited
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-09T13:08:27Z
On 08.03.23 22:40, Andrew Dunstan wrote: > There are probably some fairly easy opportunities to reduce the number > of non-terminals introduced here (e.g. I think json_aggregate_func could > possibly be expanded in place without introducing > json_object_aggregate_constructor and json_array_aggregate_constructor). > I'm going to make an attempt at that, at least to pick some low hanging > fruit. But in the end I think we are going to be left with a significant > expansion of the grammar rules, more or less forced on us by the way the > SQL Standards Committee rather profligately invents new ways of > contorting the grammar. I browsed these patches, and I agree that the grammar is the thing that sticks out as something that could be tightened up a bit. Try to reduce the number of different symbols, and check that the keywords are all in alphabetical order. There are also various bits of code that are commented out, in some cases because they can't be implemented, in some cases without explanation. I think these should all be removed. Otherwise, whoever needs to touch this code next would be under some sort of obligation to keep the commented-out code up to date with surrounding changes, which would be awkward. We can find better ways to explain missing functionality and room for improvement. Also, perhaps we can find better names for the new test files. Like, what does "sqljson.sql" mean, as opposed to, say, "json.sql"? Maybe something like "json_functions", "json_expressions", etc. would be clearer. (Starting it with "json" would also group the files better.) > These both seem like things not worth holding up progress for, and I > think it would be good to get these patches committed as soon as > possible. My intention is to commit them (after some grammar > adjustments) plus their documentation in the next few days. If possible, the documentation for each incremental part should be part of that patch, not a separate all-in-one patch.
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-03-15T12:49:49Z
Hi, On Thu, Mar 9, 2023 at 10:08 PM Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote: > On 08.03.23 22:40, Andrew Dunstan wrote: > > These both seem like things not worth holding up progress for, and I > > think it would be good to get these patches committed as soon as > > possible. My intention is to commit them (after some grammar > > adjustments) plus their documentation in the next few days. > > If possible, the documentation for each incremental part should be part > of that patch, not a separate all-in-one patch. Here's a version that includes documentation of the individual bits in their own commits. I've also merged the patch to add the PLAN clause to JSON_TABLE into the patch that adds JSON_TABLE itself. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Andrew Dunstan <andrew@dunslane.net> — 2023-03-17T01:14:35Z
On 2023-03-15 We 08:49, Amit Langote wrote: > Hi, > > On Thu, Mar 9, 2023 at 10:08 PM Peter Eisentraut > <peter.eisentraut@enterprisedb.com> wrote: >> On 08.03.23 22:40, Andrew Dunstan wrote: >>> These both seem like things not worth holding up progress for, and I >>> think it would be good to get these patches committed as soon as >>> possible. My intention is to commit them (after some grammar >>> adjustments) plus their documentation in the next few days. >> If possible, the documentation for each incremental part should be part >> of that patch, not a separate all-in-one patch. > Here's a version that includes documentation of the individual bits in > their own commits. I've also merged the patch to add the PLAN clause > to JSON_TABLE into the patch that adds JSON_TABLE itself. Hi, I have taken these and done some surgery to reduce the explosion on grammar symbols. The attached set is just Amit's patches with some of this surgery done - nothing other than gram.y has been touched. Patches 2 and 5 in the series could be sanely squashed onto patches 1 and 4 respectively. I haven't done anything significant yet with the JSONTABLE patch, there is probably some more low hanging fruit there, and possibly some still in the earlier patches. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-03-22T12:18:46Z
On 2023-Mar-16, Andrew Dunstan wrote: > Hi, I have taken these and done some surgery to reduce the explosion on > grammar symbols. The attached set is just Amit's patches with some of this > surgery done - nothing other than gram.y has been touched. Patches 2 and 5 > in the series could be sanely squashed onto patches 1 and 4 respectively. I > haven't done anything significant yet with the JSONTABLE patch, there is > probably some more low hanging fruit there, and possibly some still in the > earlier patches. Hello, It looks as if the grammar for this was originally written following the SQL standard's description to the letter. AFAICS reducing the number of nonterminals as you have done is a good thing. So I started from that point (0001+0002) to see what else is missing to make that independently committable. One thing I noticed is that a number of grammar hacks are not necessary until the IS JSON patch, so I've removed them from 0001 (the constructors patch) in order to make things easier to comprehend. We can put them back together with IS JSON. For the time being, 0001 is already large enough. So here's v11 of this (0001+0002 plus some changes of my own). At this point, the main thing I'm unhappy about is the fact that the documentation addition puts the new contents at the end of the chapter, which makes no sense. So we now have: 9.16.1. Processing and Creating JSON Data 9.16.2. The SQL/JSON Path Language 9.16.3. SQL/JSON Functions and Expressions where the standard functions are in 9.16.3 and describe functions that are for creating JSON data, so they should naturally be in 9.16.1. I'll see about reformulating the whole chapter so that it makes sense. I added an ECPG test file, to make sure that the weird grammar productions parse correctly. There are other minor things too, which I'll see about. Once I get this one done, I'll rebase and repost the rest of the series. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-03-27T18:54:56Z
Docs amended as I threatened. Other than that, this has required more changes than I thought it would. For one thing, I've continued to adjust the grammar a little bit, hopefully without breaking anything (at least, the tests continue to work). Even so, I was unable to get bison to accept the 'KEY name VALUES blah' syntax; it might be a fun/challenging project to change the productions that we use for JSON names and values: +json_name_and_value: +/* Supporting this syntax seems to require major surgery + KEY c_expr VALUE_P json_value_expr + { $$ = makeJsonKeyValue($2, $4); } + | +*/ + c_expr VALUE_P json_value_expr + { $$ = makeJsonKeyValue($1, $3); } + | + a_expr ':' json_value_expr + { $$ = makeJsonKeyValue($1, $3); } + ; If we uncomment the KEY bit there, a bunch of conflicts emerge. Also, the fact that we have a_expr on the third one but c_expr on the second bothers me on consistency grounds; but really we should have a separate production for things that can be JSON field names. (I also removed json_object_constructor_args_opt and json_object_args as separate productions, because I didn't see that they got us anything.) I also noticed that we had a "string of skipped keys" thing in json.c that was supposed to be used to detect keys already used but with only NULL values; however, that stuff had already been rewritten by Nikita on July 2020 to use a hash table, so the string itself was being built but uselessly so AFAICS. Removed that one. I added a bunch of comments in several places, and postponed addition of a couple of structs that are not needed for this part of the features. Some of these will have to come back with the IS JSON support (0002 in the original set). Anyway, barring objections or further problems, I intend to get this one pushed early tomorrow. For the curious, I've pushed it here https://github.com/alvherre/postgres/tree/sqljson-constructors and the tests are currently running here: https://cirrus-ci.com/build/5468150918021120 -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "La experiencia nos dice que el hombre peló millones de veces las patatas, pero era forzoso admitir la posibilidad de que en un caso entre millones, las patatas pelarían al hombre" (Ijon Tichy) -
Re: SQL/JSON revisited
Justin Pryzby <pryzby@telsasoft.com> — 2023-03-27T21:18:41Z
I ran sqlsmith on this patch for a short while, and reduced one of its appalling queries to this: postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8); ERROR: unexpected jsonb type as object key postgres=# \errverbose ERROR: XX000: unexpected jsonb type as object key UBICACIÓN: JsonbIteratorNext, jsonb_util.c:958 As you know, it's considered bad if elog()s are reachable, user-facing errors. 2023-03-27 15:46:47.351 CDT client backend[13361] psql ERROR: unexpected jsonb type as object key 2023-03-27 15:46:47.351 CDT client backend[13361] psql BACKTRACE: postgres: pryzbyj postgres [local] SELECT(JsonbIteratorNext+0x1e5) [0x5638fa11ba82] postgres: pryzbyj postgres [local] SELECT(+0x4ff951) [0x5638fa114951] postgres: pryzbyj postgres [local] SELECT(JsonbToCString+0x12) [0x5638fa116584] postgres: pryzbyj postgres [local] SELECT(jsonb_out+0x24) [0x5638fa1165ad] postgres: pryzbyj postgres [local] SELECT(FunctionCall1Coll+0x51) [0x5638fa1ef585] postgres: pryzbyj postgres [local] SELECT(OutputFunctionCall+0x15) [0x5638fa1f067d] postgres: pryzbyj postgres [local] SELECT(+0xe7ef7) [0x5638f9cfcef7] postgres: pryzbyj postgres [local] SELECT(+0x2b4271) [0x5638f9ec9271] postgres: pryzbyj postgres [local] SELECT(standard_ExecutorRun+0x146) [0x5638f9ec9402] What might indicate a worse problem is that with debug_discard_caches=1, it does something different: postgres=# \errverbose ERROR: XX000: invalid jsonb scalar type UBICACIÓN: convertJsonbScalar, jsonb_util.c:1865 2023-03-27 15:51:21.788 CDT client backend[15939] psql ERROR: invalid jsonb scalar type 2023-03-27 15:51:21.788 CDT client backend[15939] psql CONTEXT: parallel worker 2023-03-27 15:51:21.788 CDT client backend[15939] psql BACKTRACE: postgres: pryzbyj postgres [local] SELECT(ThrowErrorData+0x2a6) [0x5638fa1ec8f3] postgres: pryzbyj postgres [local] SELECT(+0x194820) [0x5638f9da9820] postgres: pryzbyj postgres [local] SELECT(HandleParallelMessages+0x15d) [0x5638f9daac95] postgres: pryzbyj postgres [local] SELECT(ProcessInterrupts+0x906) [0x5638fa094873] postgres: pryzbyj postgres [local] SELECT(+0x2d202b) [0x5638f9ee702b] postgres: pryzbyj postgres [local] SELECT(+0x2d2206) [0x5638f9ee7206] postgres: pryzbyj postgres [local] SELECT(+0x2d245a) [0x5638f9ee745a] postgres: pryzbyj postgres [local] SELECT(+0x2bbcec) [0x5638f9ed0cec] postgres: pryzbyj postgres [local] SELECT(+0x2b4240) [0x5638f9ec9240] postgres: pryzbyj postgres [local] SELECT(standard_ExecutorRun+0x146) [0x5638f9ec9402] +valgrind indicates this: ==14095== Use of uninitialised value of size 8 ==14095== at 0x60D1C9: convertJsonbScalar (jsonb_util.c:1822) ==14095== by 0x60D44F: convertJsonbObject (jsonb_util.c:1741) ==14095== by 0x60D630: convertJsonbValue (jsonb_util.c:1611) ==14095== by 0x60D903: convertToJsonb (jsonb_util.c:1565) ==14095== by 0x60F272: JsonbValueToJsonb (jsonb_util.c:117) ==14095== by 0x60A504: jsonb_object_agg_finalfn (jsonb.c:2057) ==14095== by 0x3D0806: finalize_aggregate (nodeAgg.c:1119) ==14095== by 0x3D2210: finalize_aggregates (nodeAgg.c:1353) ==14095== by 0x3D2E7F: agg_retrieve_direct (nodeAgg.c:2512) ==14095== by 0x3D32DC: ExecAgg (nodeAgg.c:2172) ==14095== by 0x3C3CEB: ExecProcNodeFirst (execProcnode.c:464) ==14095== by 0x3BC23F: ExecProcNode (executor.h:272) ==14095== by 0x3BC23F: ExecutePlan (execMain.c:1633) And then it shows a different error: 2023-03-27 16:00:10.072 CDT standalone backend[14095] ERROR: unknown type of jsonb container to convert In the docs: + The <parameter>key</parameter> can not be null. If the + <parameter>value</parameter> is null then the entry is skipped, s/can not/cannot/ The "," is dangling. -- Justin -
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-03-28T03:29:23Z
On Tue, Mar 28, 2023 at 6:18 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > I ran sqlsmith on this patch for a short while, and reduced one of its > appalling queries to this: > > postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8); > ERROR: unexpected jsonb type as object key I think this may have to do with the following changes to uniqueifyJsonbObject() that the patch makes: @@ -1936,7 +1942,7 @@ lengthCompareJsonbPair(const void *a, const void *b, void *binequal) * Sort and unique-ify pairs in JsonbValue object */ static void -uniqueifyJsonbObject(JsonbValue *object) +uniqueifyJsonbObject(JsonbValue *object, bool unique_keys, bool skip_nulls) { bool hasNonUniq = false; @@ -1946,15 +1952,32 @@ uniqueifyJsonbObject(JsonbValue *object) qsort_arg(object->val.object.pairs, object->val.object.nPairs, sizeof(JsonbPair), lengthCompareJsonbPair, &hasNonUniq); - if (hasNonUniq) + if (hasNonUniq && unique_keys) + ereport(ERROR, + errcode(ERRCODE_DUPLICATE_JSON_OBJECT_KEY_VALUE), + errmsg("duplicate JSON object key value")); + + if (hasNonUniq || skip_nulls) { - JsonbPair *ptr = object->val.object.pairs + 1, - *res = object->val.object.pairs; + JsonbPair *ptr, + *res; + + while (skip_nulls && object->val.object.nPairs > 0 && + object->val.object.pairs->value.type == jbvNull) + { + /* If skip_nulls is true, remove leading items with null */ + object->val.object.pairs++; + object->val.object.nPairs--; + } + + ptr = object->val.object.pairs + 1; + res = object->val.object.pairs; The code below the while loop does not take into account the possibility that object->val.object.pairs would be pointing to garbage when object->val.object.nPairs is 0. Attached delta patch that applies on top of Alvaro's v12-0001 fixes the case for me: postgres=# SELECT jsonb_object_agg_unique_strict('', null::xid8); jsonb_object_agg_unique_strict -------------------------------- {} (1 row) postgres=# SELECT jsonb_object_agg_unique_strict('1', null::xid8); jsonb_object_agg_unique_strict -------------------------------- {} (1 row) SELECT jsonb_object_agg_unique_strict('1', '1'::xid8); jsonb_object_agg_unique_strict -------------------------------- {"1": "1"} (1 row) -- Thanks, Amit Langote EDB: http://www.enterprisedb.com -
Re: SQL/JSON revisited
Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-03-28T06:07:23Z
On 27.03.23 20:54, Alvaro Herrera wrote: > Even so, I was unable to get bison > to accept the 'KEY name VALUES blah' syntax; it might be a > fun/challenging project to change the productions that we use for JSON > names and values: > > +json_name_and_value: > +/* Supporting this syntax seems to require major surgery > + KEY c_expr VALUE_P json_value_expr > + { $$ = makeJsonKeyValue($2, $4); } > + | > +*/ > + c_expr VALUE_P json_value_expr > + { $$ = makeJsonKeyValue($1, $3); } > + | > + a_expr ':' json_value_expr > + { $$ = makeJsonKeyValue($1, $3); } > + ; > > If we uncomment the KEY bit there, a bunch of conflicts emerge. This is a known bug in the SQL standard. Because KEY is a non-reserved keyword, writing KEY (x) VALUE y is ambiguous because KEY could be the keyword for this clause or a function call key(x). It's ok to leave it like this for now. -
Re: SQL/JSON revisited
Erik Rijkers <er@xs4all.nl> — 2023-03-28T13:40:24Z
Op 3/27/23 om 20:54 schreef Alvaro Herrera: > Docs amended as I threatened. Other than that, this has required more > [v12-0001-SQL-JSON-constructors.patch] > [v12-0001-delta-uniqueifyJsonbObject-bugfix.patch] In doc/src/sgml/func.sgml, some minor stuff: 'which specify the data type returned' should be 'which specifies the data type returned' In the json_arrayagg() description, it says: 'If ABSENT ON NULL is specified, any NULL values are omitted.' That's true, but as omitting NULL values is the default (i.e., also without that clause) maybe it's better to say: 'Any NULL values are omitted unless NULL ON NULL is specified' I've found no bugs in functionality. Thanks, Erik Rijkers
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-03-29T10:27:58Z
On 2023-Mar-28, Erik Rijkers wrote: > In the json_arrayagg() description, it says: > 'If ABSENT ON NULL is specified, any NULL values are omitted.' > That's true, but as omitting NULL values is the default (i.e., also without > that clause) maybe it's better to say: > 'Any NULL values are omitted unless NULL ON NULL is specified' Doh, somehow I misread your report and modified the json_object() documentation instead after experimenting with it (so now the ABSENT/NULL ON NULL clause is inconsistenly described everywhere). Would you mind submitting a patch fixing this mistake? ... and pushed it now, after some more meddling. I'll rebase the rest of the series now. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Entristecido, Wutra (canción de Las Barreras) echa a Freyr a rodar y a nosotros al mar"
-
Re: SQL/JSON revisited
Erik Rijkers <er@xs4all.nl> — 2023-03-29T12:49:40Z
Op 3/29/23 om 12:27 schreef Alvaro Herrera: > On 2023-Mar-28, Erik Rijkers wrote: > >> In the json_arrayagg() description, it says: >> 'If ABSENT ON NULL is specified, any NULL values are omitted.' >> That's true, but as omitting NULL values is the default (i.e., also without >> that clause) maybe it's better to say: >> 'Any NULL values are omitted unless NULL ON NULL is specified' > > Doh, somehow I misread your report and modified the json_object() > documentation instead after experimenting with it (so now the > ABSENT/NULL ON NULL clause is inconsistenly described everywhere). > Would you mind submitting a patch fixing this mistake? I think the json_object text was OK. Attached are some changes where they were needed IMHO. Erik > > ... and pushed it now, after some more meddling. > > I'll rebase the rest of the series now. >
-
Re: SQL/JSON revisited
Alexander Law <exclusion@gmail.com> — 2023-03-29T18:00:00Z
Hi, 29.03.2023 13:27, Alvaro Herrera wrote: > ... and pushed it now, after some more meddling. > > I'll rebase the rest of the series now. Please look at the several minor issues/inconsistencies, I've spotted in the commit: 1) s/JSON_ARRRAYAGG/JSON_ARRAYAGG/ 2) check_key_uniqueness vs check_unique IIUC, these are different names of the same entity. 3) elog(ERROR, "invalid JsonConstructorExprType %d", ctor->type); vs elog(ERROR, "invalid JsonConstructorExpr type %d", ctor->type); I'd choose the latter spelling as the JsonConstructorExprType entity does not exist. 4) In the block: else { res = (Datum) 0; elog(ERROR, "invalid JsonConstructorExpr type %d", ctor->type); } res is assigned but never used. 5) (expr [FORMAT json_format]) ->? (expr [FORMAT JsonFormat]) (json_format not found anywhere else) Best regards, Alexander -
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-03-29T18:17:08Z
On 2023-Mar-29, Alexander Lakhin wrote: > Hi, > > 29.03.2023 13:27, Alvaro Herrera wrote: > > ... and pushed it now, after some more meddling. > > > > I'll rebase the rest of the series now. > > Please look at the several minor issues/inconsistencies, > I've spotted in the commit: Thanks, I'll look at this tomorrow. In the meantime, here's the next two patches of the series: IS JSON and the "query" functions. I think this is as much as I can get done for this release, so the last two pieces of functionality would have to wait for 17. I still need to clean these up some more. These are not thoroughly tested either; 0001 compiles and passes regression tests, but I didn't verify 0003 other than there being no Git conflicts and bison doesn't complain. Also, notable here is that I realized that I need to backtrack on my change of the WITHOUT_LA: the original patch had it for TIME (in WITHOUT TIME ZONE), and I changed to be for UNIQUE. But now that I've done "JSON query functions" I realize that it needed to be the other way for the WITHOUT ARRAY WRAPPER clause too. So 0002 reverts that choice. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "Crear es tan difícil como ser libre" (Elsa Triolet)
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-03-31T16:49:40Z
Here's v14 of the IS JSON predicate. I find no further problems here and intend to get it pushed later today. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-04-03T17:16:47Z
On 2023-Mar-29, Alvaro Herrera wrote: > In the meantime, here's the next two patches of the series: IS JSON and > the "query" functions. I think this is as much as I can get done for > this release, so the last two pieces of functionality would have to wait > for 17. I still need to clean these up some more. These are not > thoroughly tested either; 0001 compiles and passes regression tests, but > I didn't verify 0003 other than there being no Git conflicts and bison > doesn't complain. > > Also, notable here is that I realized that I need to backtrack on my > change of the WITHOUT_LA: the original patch had it for TIME (in WITHOUT > TIME ZONE), and I changed to be for UNIQUE. But now that I've done > "JSON query functions" I realize that it needed to be the other way for > the WITHOUT ARRAY WRAPPER clause too. So 0002 reverts that choice. So I pushed 0001 on Friday, and here are 0002 (which I intend to push shortly, since it shouldn't be controversial) and the "JSON query functions" patch as 0003. After looking at it some more, I think there are some things that need to be addressed by one of the authors: - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. I think we could make that stuff use something similar to ConstraintAttributeSpec with an accompanying post-processing function. That would reduce the number of ad-hoc hacks, which seem excessive. - the changes in formatting.h have no explanation whatsoever. At the very least, the new function should have a comment in the .c file. (And why is it at end of file? I bet there's a better location) - some nasty hacks are being used in the ECPG grammar with no tests at all. It's easy to add a few lines to the .pgc file I added in prior commits. - Some functions in jsonfuncs.c have changed from throwing hard errors into soft ones. I think this deserves more commentary. - func.sgml: The new functions are documented in a separate table for no reason that I can see. Needs to be merged into one of the existing tables. I didn't actually review the docs. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "I'm impressed how quickly you are fixing this obscure issue. I came from MS SQL and it would be hard for me to put into words how much of a better job you all are doing on [PostgreSQL]." Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php
-
Re: SQL/JSON revisited
Alexander Law <exclusion@gmail.com> — 2023-04-03T20:00:00Z
Hi Alvaro, 03.04.2023 20:16, Alvaro Herrera wrote: > So I pushed 0001 on Friday, and here are 0002 (which I intend to push > shortly, since it shouldn't be controversial) and the "JSON query > functions" patch as 0003. After looking at it some more, I think there > are some things that need to be addressed by one of the authors: > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. > I think we could make that stuff use something similar to > ConstraintAttributeSpec with an accompanying post-processing function. > That would reduce the number of ad-hoc hacks, which seem excessive. > > - the changes in formatting.h have no explanation whatsoever. At the > very least, the new function should have a comment in the .c file. > (And why is it at end of file? I bet there's a better location) > > - some nasty hacks are being used in the ECPG grammar with no tests at > all. It's easy to add a few lines to the .pgc file I added in prior > commits. > > - Some functions in jsonfuncs.c have changed from throwing hard errors > into soft ones. I think this deserves more commentary. > > - func.sgml: The new functions are documented in a separate table for no > reason that I can see. Needs to be merged into one of the existing > tables. I didn't actually review the docs. Please take a look at the following minor issues in v15-0002-SQL-JSON-query-functions.patch: 1) s/addreess/address/ 2) ECPGColLabelCommon gone with 83f1c7b74, but is still mentioned in ecpg.trailer. 3) s/ExecEvalJsonCoercion/ExecEvalJsonExprCoercion/ ? (there is no ExecEvalJsonCoercion() function) 4) json_table mentioned in func.sgml: <xref linkend="functions-sqljson-querying"/> details the SQL/JSON functions that can be used to query JSON data, except for <function>json_table</function>. but if JSON_TABLE not going to be committed in v16, maybe remove that reference to it. There is also a reference to JSON_TABLE in src/backend/parser/README: parse_jsontable.c handle JSON_TABLE (It was added with 9853bf6ab and survived the revert of SQL JSON last year somehow.) Best regards, Alexander
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-04-04T11:09:38Z
Hi Alvaro, On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2023-Mar-29, Alvaro Herrera wrote: > > In the meantime, here's the next two patches of the series: IS JSON and > > the "query" functions. I think this is as much as I can get done for > > this release, so the last two pieces of functionality would have to wait > > for 17. I still need to clean these up some more. These are not > > thoroughly tested either; 0001 compiles and passes regression tests, but > > I didn't verify 0003 other than there being no Git conflicts and bison > > doesn't complain. > > > > Also, notable here is that I realized that I need to backtrack on my > > change of the WITHOUT_LA: the original patch had it for TIME (in WITHOUT > > TIME ZONE), and I changed to be for UNIQUE. But now that I've done > > "JSON query functions" I realize that it needed to be the other way for > > the WITHOUT ARRAY WRAPPER clause too. So 0002 reverts that choice. > > So I pushed 0001 on Friday, and here are 0002 (which I intend to push > shortly, since it shouldn't be controversial) and the "JSON query > functions" patch as 0003. After looking at it some more, I think there > are some things that need to be addressed by one of the authors: > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. > I think we could make that stuff use something similar to > ConstraintAttributeSpec with an accompanying post-processing function. > That would reduce the number of ad-hoc hacks, which seem excessive. Do you mean the solution involving the JsonBehavior node? > - the changes in formatting.h have no explanation whatsoever. At the > very least, the new function should have a comment in the .c file. > (And why is it at end of file? I bet there's a better location) > > - some nasty hacks are being used in the ECPG grammar with no tests at > all. It's easy to add a few lines to the .pgc file I added in prior > commits. > > - Some functions in jsonfuncs.c have changed from throwing hard errors > into soft ones. I think this deserves more commentary. > > - func.sgml: The new functions are documented in a separate table for no > reason that I can see. Needs to be merged into one of the existing > tables. I didn't actually review the docs. I made the jsonfuncs.c changes to use soft error handling when needed, so I took a stab at that; attached a delta patch, which also fixes the problematic comments mentioned by Alexander in his comments 1 and 3. I'll need to spend some time to address other points. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-04-04T12:36:25Z
On 2023-Apr-04, Amit Langote wrote: > On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. > > I think we could make that stuff use something similar to > > ConstraintAttributeSpec with an accompanying post-processing function. > > That would reduce the number of ad-hoc hacks, which seem excessive. > > Do you mean the solution involving the JsonBehavior node? Right. It has spilled as the separate on_behavior struct in the core parser %union in addition to the raw jsbehavior, which is something we've gone 30 years without having, and I don't see why we should start now. This stuff is terrible: json_exists_error_clause_opt: json_exists_error_behavior ON ERROR_P { $$ = $1; } | /* EMPTY */ { $$ = NULL; } ; json_exists_error_behavior: ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); } | TRUE_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_TRUE, NULL); } | FALSE_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_FALSE, NULL); } | UNKNOWN { $$ = makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN, NULL); } ; json_value_behavior: NULL_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); } | ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); } | DEFAULT a_expr { $$ = makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); } ; json_value_on_behavior_clause_opt: json_value_behavior ON EMPTY_P { $$.on_empty = $1; $$.on_error = NULL; } | json_value_behavior ON EMPTY_P json_value_behavior ON ERROR_P { $$.on_empty = $1; $$.on_error = $4; } | json_value_behavior ON ERROR_P { $$.on_empty = NULL; $$.on_error = $1; } | /* EMPTY */ { $$.on_empty = NULL; $$.on_error = NULL; } ; json_query_behavior: ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, NULL); } | NULL_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); } | EMPTY_P ARRAY { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } /* non-standard, for Oracle compatibility only */ | EMPTY_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } | EMPTY_P OBJECT_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL); } | DEFAULT a_expr { $$ = makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); } ; json_query_on_behavior_clause_opt: json_query_behavior ON EMPTY_P { $$.on_empty = $1; $$.on_error = NULL; } | json_query_behavior ON EMPTY_P json_query_behavior ON ERROR_P { $$.on_empty = $1; $$.on_error = $4; } | json_query_behavior ON ERROR_P { $$.on_empty = NULL; $$.on_error = $1; } | /* EMPTY */ { $$.on_empty = NULL; $$.on_error = NULL; } ; Surely this can be made cleaner. By the way -- that comment about clauses being non-standard, can you spot exactly *which* clauses that comment applies to? -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "El número de instalaciones de UNIX se ha elevado a 10, y se espera que este número aumente" (UPM, 1972) -
Re: SQL/JSON revisited
Nikita Malakhov <hukutoc@gmail.com> — 2023-04-04T19:40:16Z
Hi hackers! The latest SQL standard contains dot notation for JSON. Are there any plans to include it into Pg 16? Or maybe we should start a separate thread for it? Thanks! On Tue, Apr 4, 2023 at 3:36 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2023-Apr-04, Amit Langote wrote: > > > On Tue, Apr 4, 2023 at 2:16 AM Alvaro Herrera <alvherre@alvh.no-ip.org> > wrote: > > > > - the gram.y solution to the "ON ERROR/ON EMPTY" clauses is quite ugly. > > > I think we could make that stuff use something similar to > > > ConstraintAttributeSpec with an accompanying post-processing > function. > > > That would reduce the number of ad-hoc hacks, which seem excessive. > > > > Do you mean the solution involving the JsonBehavior node? > > Right. It has spilled as the separate on_behavior struct in the core > parser %union in addition to the raw jsbehavior, which is something > we've gone 30 years without having, and I don't see why we should start > now. > > This stuff is terrible: > > json_exists_error_clause_opt: > json_exists_error_behavior ON ERROR_P { $$ = $1; } > | /* EMPTY */ { $$ = NULL; } > ; > > json_exists_error_behavior: > ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, > NULL); } > | TRUE_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_TRUE, > NULL); } > | FALSE_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_FALSE, > NULL); } > | UNKNOWN { $$ = makeJsonBehavior(JSON_BEHAVIOR_UNKNOWN, > NULL); } > ; > > json_value_behavior: > NULL_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, NULL); > } > | ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, > NULL); } > | DEFAULT a_expr { $$ = > makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); } > ; > > json_value_on_behavior_clause_opt: > json_value_behavior ON EMPTY_P > { $$.on_empty = $1; $$.on_error = > NULL; } > | json_value_behavior ON EMPTY_P json_value_behavior ON ERROR_P > { $$.on_empty = $1; $$.on_error = $4; } > | json_value_behavior ON ERROR_P > { $$.on_empty = NULL; $$.on_error = > $1; } > | /* EMPTY */ > { $$.on_empty = NULL; $$.on_error = > NULL; } > ; > > json_query_behavior: > ERROR_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_ERROR, > NULL); } > | NULL_P { $$ = makeJsonBehavior(JSON_BEHAVIOR_NULL, > NULL); } > | EMPTY_P ARRAY { $$ = > makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > /* non-standard, for Oracle compatibility only */ > | EMPTY_P { $$ = > makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); } > | EMPTY_P OBJECT_P { $$ = > makeJsonBehavior(JSON_BEHAVIOR_EMPTY_OBJECT, NULL); } > | DEFAULT a_expr { $$ = > makeJsonBehavior(JSON_BEHAVIOR_DEFAULT, $2); } > ; > > json_query_on_behavior_clause_opt: > json_query_behavior ON EMPTY_P > { $$.on_empty = $1; $$.on_error = > NULL; } > | json_query_behavior ON EMPTY_P json_query_behavior ON ERROR_P > { $$.on_empty = $1; $$.on_error = $4; } > | json_query_behavior ON ERROR_P > { $$.on_empty = NULL; $$.on_error = > $1; } > | /* EMPTY */ > { $$.on_empty = NULL; $$.on_error = > NULL; } > ; > > Surely this can be made cleaner. > > By the way -- that comment about clauses being non-standard, can you > spot exactly *which* clauses that comment applies to? > > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ > "El número de instalaciones de UNIX se ha elevado a 10, > y se espera que este número aumente" (UPM, 1972) > > > -- Regards, Nikita Malakhov Postgres Professional The Russian Postgres Company https://postgrespro.ru/ -
Re: SQL/JSON revisited
Jonathan S. Katz <jkatz@postgresql.org> — 2023-04-04T19:44:05Z
On 4/4/23 3:40 PM, Nikita Malakhov wrote: > Hi hackers! > > The latest SQL standard contains dot notation for JSON. Are there any > plans to include it into Pg 16? > Or maybe we should start a separate thread for it? I would recommend starting a new thread to discuss the dot notation. Thanks, Jonathan
-
Re: SQL/JSON revisited
Andrew Dunstan <andrew@dunslane.net> — 2023-04-04T20:05:47Z
On 2023-04-04 Tu 08:36, Alvaro Herrera wrote: > > Surely this can be made cleaner. > > By the way -- that comment about clauses being non-standard, can you > spot exactly *which* clauses that comment applies to? > Sadly, I don't think we're going to be able to make further progress before feature freeze. Thanks to Alvaro for advancing us a way down the field. I hope we can get the remainder committed in the July CF. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-04-05T07:23:29Z
On 2023-Apr-04, Andrew Dunstan wrote: > On 2023-04-04 Tu 08:36, Alvaro Herrera wrote: > > > > Surely this can be made cleaner. > > > > By the way -- that comment about clauses being non-standard, can you > > spot exactly *which* clauses that comment applies to? > > Sadly, I don't think we're going to be able to make further progress before > feature freeze. Thanks to Alvaro for advancing us a way down the field. I > hope we can get the remainder committed in the July CF. Okay, I've marked the CF entry as committed then. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ “Cuando no hay humildad las personas se degradan” (A. Christie)
-
Re: SQL/JSON revisited (documentation)
Erik Rijkers <er@xs4all.nl> — 2023-04-12T04:43:23Z
Hi, IS JSON is documented as: expression IS [ NOT ] JSON [ { VALUE | SCALAR | ARRAY | OBJECT } ] [ { WITH | WITHOUT } UNIQUE [ KEYS ] ] which is fine but 'VALUE' is nowhere mentioned (except in the commit-message as: IS JSON [VALUE] ) Unless I'm mistaken 'VALUE' does indeed not change an IS JSON statement, so to document we could simply insert this line (as in the attached): "The VALUE key word is optional noise." Somewhere in its text in func.sgml, which is now: "This predicate tests whether expression can be parsed as JSON, possibly of a specified type. If SCALAR or ARRAY or OBJECT is specified, the test is whether or not the JSON is of that particular type. If WITH UNIQUE KEYS is specified, then any object in the expression is also tested to see if it has duplicate keys." Erik Rijkers -
Re: SQL/JSON revisited
Matthias Kurz <m.kurz@irregular.at> — 2023-05-03T16:51:56Z
On Wed, 5 Apr 2023 at 09:53, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > Okay, I've marked the CF entry as committed then. > This was marked as commited in the 2023-03 commitfest, however there are still patches missing (for example the JSON_TABLE one). However, I can not see an entry in the current 2023-07 Commitfest. I think it would be a good idea for a new entry in the current commitfest, just to not forget about the not-yet-commited features. Thanks! Matthias
-
Re: SQL/JSON revisited
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-03T18:17:32Z
On 2023-May-03, Matthias Kurz wrote: > On Wed, 5 Apr 2023 at 09:53, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > > Okay, I've marked the CF entry as committed then. > > This was marked as commited in the 2023-03 commitfest, however there are > still patches missing (for example the JSON_TABLE one). > However, I can not see an entry in the current 2023-07 Commitfest. > I think it would be a good idea for a new entry in the current commitfest, > just to not forget about the not-yet-commited features. Yeah ... These remaining patches have to be rebased, and a few things fixed (I left a few review comments somewhere). I would suggest to start a new thread with updated patches, and then a new commitfest entry can be created with those. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Las mujeres son como hondas: mientras más resistencia tienen, más lejos puedes llegar con ellas" (Jonas Nightingale, Leap of Faith)
-
Re: SQL/JSON revisited
Matthias Kurz <m.kurz@irregular.at> — 2023-05-03T18:58:02Z
On Wed, 3 May 2023 at 20:17, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > I would suggest to start a new thread with updated patches, and then a new > commitfest entry can be created with those. > Whoever starts that new thread, please link link it here, I am keen to follow it ;) Thanks a lot! Thanks a lot for all your hard work btw, it's highly appreciated! Best, Matthias
-
Re: SQL/JSON revisited
Amit Langote <amitlangote09@gmail.com> — 2023-06-19T08:35:34Z
Hi, On Thu, May 4, 2023 at 3:58 AM Matthias Kurz <m.kurz@irregular.at> wrote: > On Wed, 3 May 2023 at 20:17, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> I would suggest to start a new thread with updated patches, and then a new commitfest entry can be created with those. > > Whoever starts that new thread, please link link it here, I am keen to follow it ;) Thanks a lot! > Thanks a lot for all your hard work btw, it's highly appreciated! Just created a new thread: https://www.postgresql.org/message-id/CA%2BHiwqE4XTdfb1nW%3DOjoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg%40mail.gmail.com CF entry: https://commitfest.postgresql.org/43/4377/ -- Thanks, Amit Langote EDB: http://www.enterprisedb.com