Обсуждение: SQL/JSON revisited
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
Вложения
- v1-0007-JSON_TABLE.patch
- v1-0010-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v1-0009-Documentation-for-SQL-JSON-features.patch
- v1-0008-PLAN-clauses-for-JSON_TABLE.patch
- v1-0006-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v1-0005-SQL-JSON-functions.patch
- v1-0004-SQL-JSON-query-functions.patch
- v1-0001-Common-SQL-JSON-clauses.patch
- v1-0003-IS-JSON-predicate.patch
- v1-0002-SQL-JSON-constructors.patch
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
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. >
Вложения
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
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
>
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
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
Вложения
- v2-0003-IS-JSON-predicate.patch
- v2-0005-SQL-JSON-functions.patch
- v2-0001-Common-SQL-JSON-clauses.patch
- v2-0002-SQL-JSON-constructors.patch
- v2-0004-SQL-JSON-query-functions.patch
- v2-0007-JSON_TABLE.patch
- v2-0006-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v2-0009-Documentation-for-SQL-JSON-features.patch
- v2-0008-PLAN-clauses-for-JSON_TABLE.patch
- v2-0010-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v2-0011-Proposed-reworking-of-SQL-JSON-documentaion.patch
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
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
Вложения
- v3-0011-Proposed-reworking-of-SQL-JSON-documentaion.patch
- v3-0010-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v3-0009-Documentation-for-SQL-JSON-features.patch
- v3-0007-JSON_TABLE.patch
- v3-0008-PLAN-clauses-for-JSON_TABLE.patch
- v3-0006-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v3-0005-SQL-JSON-functions.patch
- v3-0003-IS-JSON-predicate.patch
- v3-0004-SQL-JSON-query-functions.patch
- v3-0002-SQL-JSON-constructors.patch
- v3-0001-Common-SQL-JSON-clauses.patch
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
Вложения
- v4-0010-Proposed-reworking-of-SQL-JSON-documentaion.patch
- v4-0008-Documentation-for-SQL-JSON-features.patch
- v4-0006-JSON_TABLE.patch
- v4-0009-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v4-0007-PLAN-clauses-for-JSON_TABLE.patch
- v4-0005-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v4-0004-SQL-JSON-functions.patch
- v4-0003-SQL-JSON-query-functions.patch
- v4-0001-SQL-JSON-constructors.patch
- v4-0002-IS-JSON-predicate.patch
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.
>
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
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
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
Вложения
- v5-0007-PLAN-clauses-for-JSON_TABLE.patch
- v5-0009-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v5-0010-Proposed-reworking-of-SQL-JSON-documentaion.patch
- v5-0008-Documentation-for-SQL-JSON-features.patch
- v5-0006-JSON_TABLE.patch
- v5-0005-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v5-0002-IS-JSON-predicate.patch
- v5-0003-SQL-JSON-query-functions.patch
- v5-0004-SQL-JSON-functions.patch
- v5-0001-SQL-JSON-constructors.patch
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
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
Вложения
- v6-0009-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v6-0007-PLAN-clauses-for-JSON_TABLE.patch
- v6-0005-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v6-0008-Documentation-for-SQL-JSON-features.patch
- v6-0006-JSON_TABLE.patch
- v6-0004-SQL-JSON-functions.patch
- v6-0003-SQL-JSON-query-functions.patch
- v6-0001-SQL-JSON-constructors.patch
- v6-0002-IS-JSON-predicate.patch
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
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
Вложения
- v7-0005-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v7-0006-JSON_TABLE.patch
- v7-0009-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v7-0007-PLAN-clauses-for-JSON_TABLE.patch
- v7-0008-Documentation-for-SQL-JSON-features.patch
- v7-0004-SQL-JSON-functions.patch
- v7-0003-SQL-JSON-query-functions.patch
- v7-0001-SQL-JSON-constructors.patch
- v7-0002-IS-JSON-predicate.patch
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
Вложения
- v8-0009-Claim-SQL-standard-compliance-for-SQL-JSON-featur.patch
- v8-0007-PLAN-clauses-for-JSON_TABLE.patch
- v8-0005-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v8-0008-Documentation-for-SQL-JSON-features.patch
- v8-0006-JSON_TABLE.patch
- v8-0004-SQL-JSON-functions.patch
- v8-0003-SQL-JSON-query-functions.patch
- v8-0002-IS-JSON-predicate.patch
- v8-0001-SQL-JSON-constructors.patch
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
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.
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
Вложения
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
Вложения
- v10-0001-SQL-JSON-constructors.patch
- v10-0002-reduce-number-of-new-grammar-non-terminal-symbols.patch
- v10-0003-IS-JSON-predicate.patch
- v10-0004-SQL-JSON-query-functions.patch
- v10-0005-remove-more-non-terminal-grammar-symbols.patch
- v10-0006-SQL-JSON-functions.patch
- v10-0007-RETURNING-clause-for-JSON-and-JSON_SCALAR.patch
- v10-0008-JSON_TABLE.patch
- v10-0009-Claim-SQL-standard-compliance-for-SQL-JSON-features.patch
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"
Вложения
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)
Вложения
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
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
Вложения
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.
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
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"
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. >
Вложения
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
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)
Вложения
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)
Вложения
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
Вложения
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
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
Вложения
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)
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)
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
Вложения
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
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)
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
Вложения
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
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)
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
Matthias
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