Re: general purpose array_sort
Hi Junwang,
On Wed, Oct 2, 2024 at 11:46 PM Junwang Zhao wrote:
> On Wed, Oct 2, 2024 at 9:51 AM jian he wrote:
> >
> > > >
> > > > + typentry = (TypeCacheEntry *) fcinfo->flinfo->fn_extra;
> > > > + if (typentry == NULL || typentry->type_id != elmtyp)
> > > > + {
> > > > + typentry = lookup_type_cache(elmtyp, sort_asc ? TYPECACHE_LT_OPR :
> > > > TYPECACHE_GT_OPR);
> > > > + fcinfo->flinfo->fn_extra = (void *) typentry;
> > > > + }
> > > > you need to one-time check typentry->lt_opr or typentry->gt_opr exists?
> > > > see CreateStatistics.
> > > > /* Disallow data types without a less-than operator */
> > > > type = lookup_type_cache(attForm->atttypid,
> > > > TYPECACHE_LT_OPR);
> > > > if (type->lt_opr == InvalidOid)
> > > > ereport(ERROR,
> > > > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > > errmsg("column \"%s\" cannot be used in
> > > > statistics because its type %s has no default btree operator class",
> > > > attname,
> > > > format_type_be(attForm->atttypid;
> > >
> > > I added an Assert for this part, not sure if that is enough.
> > >
> >
> > i think it really should be:
> >
> > if (typentry == NULL || typentry->type_id != elmtyp)
> > {
> > typentry = lookup_type_cache(elmtyp, sort_asc ? TYPECACHE_LT_OPR :
> > TYPECACHE_GT_OPR);
> > fcinfo->flinfo->fn_extra = (void *) typentry;
> > if ((sort_asc && !OidIsValid(typentry->lt_opr) || (!sort_as &&
> > OidIsValid(typentry->gt_opr));
> > ereport(ERROR,)
> > }
> >
> > Imagine a type that doesn't have TYPECACHE_LT_OPR or TYPECACHE_GT_OPR
> > then we cannot do the sort, we should just error out.
> >
> > I just tried this colour type [1] with (CREATE TYPE colour (INPUT =
> > colour_in, OUTPUT = colour_out, LIKE = pg_catalog.int4);
> >
> > select array_sort('{#FF, #FF}'::colour[]);
> > of course it will segfault with your new Assert.
> >
> >
> > [1] https://github.com/hlinnaka/colour-datatype/blob/master/colour.c
>
> Make sense, PFA v5 with Jian's suggestion.
Have you noticed that the tests have failed on Cirrus CI runs of this patch?
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F5277
It might be related to the test machines having a different *default*
locale than your local environment, which could result in a different
sort order for the test data. You may need to add an explicit COLLATE
clause to the tests to ensure consistent sorting across systems.
--
Thanks, Amit Langote
Re: Address the -Wuse-after-free warning in ATExecAttachPartition()
On Tue, Oct 1, 2024 at 3:23 PM Amit Langote wrote: > Hi, > > On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao wrote: > > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > > wrote: > > > > > > In [1], Andres reported a -Wuse-after-free bug in the > > > ATExecAttachPartition() function. I've created a patch to address it > > > with pointers from Amit offlist. > > > > > > The issue was that the partBoundConstraint variable was utilized after > > > the list_concat() function. This could potentially lead to accessing > > > the partBoundConstraint variable after its memory has been freed. > > > > > > The issue was resolved by using the return value of the list_concat() > > > function, instead of using the list1 argument of list_concat(). I > > > copied the partBoundConstraint variable to a new variable named > > > partConstraint and used it for the previous references before invoking > > > get_proposed_default_constraint(). I confirmed that the > > > eval_const_expressions(), make_ands_explicit(), > > > map_partition_varattnos(), QueuePartitionConstraintValidation() > > > functions do not modify the memory location pointed to by the > > > partBoundConstraint variable. Therefore, it is safe to use it for the > > > next reference in get_proposed_default_constraint() > > > > > > Attaching the patch. Please review and share the comments if any. > > > Thanks to Andres for spotting the bug and some off-list advice on how > > > to reproduce it. > > > > The patch LGTM. > > I reviewed the faulty code in ATExecAttachPartition() that this patch > aims to address, and it seems that the fix suggested by Alvaro in the > original report [1] might be more appropriate. It also allows us to > resolve a minor issue related to how partBoundConstraint is handled > later in the function. Specifically, the constraint checked against > the default partition, if one exists on the parent table, should not > include the negated version of the parent's constraint, which the > current code mistakenly does. This occurs because the list_concat() > operation modifies the partBoundConstraint when combining it with the > parent table’s partition constraint. Although this doesn't cause a > live bug, the inclusion of the redundant parent constraint introduces > unnecessary complexity in the combined constraint expression. This can > cause slight delays in evaluating the constraint, as the redundant > check always evaluates to false for rows in the default partition. > Ultimately, the constraint failure is still determined by the negated > version of the partition constraint of the table being attached. > > I'm inclined to apply the attached patch only to the master branch as > the case for applying this to back-branches doesn't seem all that > strong to me. Thoughts? I've pushed this to the master branch now. Thanks Nitin for the report. -- Thanks, Amit Langote
Re: Address the -Wuse-after-free warning in ATExecAttachPartition()
Hi, On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao wrote: > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > wrote: > > > > In [1], Andres reported a -Wuse-after-free bug in the > > ATExecAttachPartition() function. I've created a patch to address it > > with pointers from Amit offlist. > > > > The issue was that the partBoundConstraint variable was utilized after > > the list_concat() function. This could potentially lead to accessing > > the partBoundConstraint variable after its memory has been freed. > > > > The issue was resolved by using the return value of the list_concat() > > function, instead of using the list1 argument of list_concat(). I > > copied the partBoundConstraint variable to a new variable named > > partConstraint and used it for the previous references before invoking > > get_proposed_default_constraint(). I confirmed that the > > eval_const_expressions(), make_ands_explicit(), > > map_partition_varattnos(), QueuePartitionConstraintValidation() > > functions do not modify the memory location pointed to by the > > partBoundConstraint variable. Therefore, it is safe to use it for the > > next reference in get_proposed_default_constraint() > > > > Attaching the patch. Please review and share the comments if any. > > Thanks to Andres for spotting the bug and some off-list advice on how > > to reproduce it. > > The patch LGTM. I reviewed the faulty code in ATExecAttachPartition() that this patch aims to address, and it seems that the fix suggested by Alvaro in the original report [1] might be more appropriate. It also allows us to resolve a minor issue related to how partBoundConstraint is handled later in the function. Specifically, the constraint checked against the default partition, if one exists on the parent table, should not include the negated version of the parent's constraint, which the current code mistakenly does. This occurs because the list_concat() operation modifies the partBoundConstraint when combining it with the parent table’s partition constraint. Although this doesn't cause a live bug, the inclusion of the redundant parent constraint introduces unnecessary complexity in the combined constraint expression. This can cause slight delays in evaluating the constraint, as the redundant check always evaluates to false for rows in the default partition. Ultimately, the constraint failure is still determined by the negated version of the partition constraint of the table being attached. I'm inclined to apply the attached patch only to the master branch as the case for applying this to back-branches doesn't seem all that strong to me. Thoughts? -- Thanks, Amit Langote v2-0001-Fix-expression-list-handling-in-ATExecAttachParti.patch Description: Binary data
Re: ALTER TABLE ONLY .. DROP CONSTRAINT on partitioned tables
Hi Alvaro,
On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera wrote:
> While studying a review note from Jian He on not-null constraints, I
> came across some behavior introduced by commit 9139aa19423b[1] that I
> think is mistaken. Consider the following example:
>
> CREATE TABLE parted (a int CONSTRAINT the_check CHECK (a > 0)) PARTITION BY
> LIST (a);
> CREATE TABLE parted_1 PARTITION OF parted FOR VALUES IN (1);
> ALTER TABLE ONLY parted DROP CONSTRAINT the_check;
>
> The ALTER TABLE fails with the following message:
>
> ERROR: cannot remove constraint from only the partitioned table when
> partitions exist
> HINT: Do not specify the ONLY keyword.
>
> and the relevant code in ATExecDropConstraint is:
>
> /*
> * For a partitioned table, if partitions exist and we are told not to
> * recurse, it's a user error. It doesn't make sense to have a
> constraint
> * be defined only on the parent, especially if it's a partitioned
> table.
> */
> if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
> children != NIL && !recurse)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> errmsg("cannot remove constraint from only
> the partitioned table when partitions exist"),
> errhint("Do not specify the ONLY
> keyword.")));
>
> Note that the comment here is confused: it talks about a constraint that
> would "be defined only on the parent", but that's bogus: the end result
> would be that the constraint no longer exist on the parent but would
> continue to exist on the children. Indeed it's not entirely
> unimaginable that you start with a partitioned table with a bunch of
> constraints which are enforced on all partitions, then you later decide
> that you want this constraint to apply only to some of the partitions,
> not the whole partitioned table. To implement that, you would drop the
> constraint on the parent using ONLY, then drop it on a few of the
> partitions, but still keep it on the other partitions. This would work
> just fine if not for this ereport(ERROR).
>
> Also, you can achieve the same end result by creating the constraint on
> only some of the partitions and not on the partitioned table to start
> with.
>
> This also applies to ALTER TABLE ONLY ... DROP NOT NULL.
>
> Of course, *adding* a constraint in this fashion is also forbidden, but
> that makes perfect sense. Both restrictions were added as part of the
> same commit, so I suppose we thought they were symmetrical behaviors and
> failed to notice they weren't.
>
> The DROP of such constraints can already be done on a table with legacy
> inheritance children; it's just partitioned tables that have this
> weirdness.
Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.
So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.
> It doesn't surprise me that nobody has reported this inconsistency,
> because it seems an unusual enough situation. For the same reason, I
> wouldn't propose to backpatch this change. But I put forward the
> attached patch, which removes the ereport(ERROR)s.
The patch looks good to me.
--
Thanks, Amit Langote
Re: json_query conditional wrapper bug
On Thu, Sep 12, 2024 at 8:12 PM Amit Langote wrote: > > Hi Andreas, > > On Thu, Sep 12, 2024 at 7:08 PM Andreas Ulbrich > wrote: > > > > Salvete! > > > > > > Sorry for my out of the rules replay, but I'm not at home, and also I can't > > verify and check your patch. > > > > But I think you have missed the docu in your fix: > > > > Must the example there not also be changed: > > > > From > > > > JSON_QUERY(jsonb '[1,[2,3],null]', 'lax $[*][$off]' PASSING 1 AS off WITH > > CONDITIONAL WRAPPER) → [3] > > > > to > > > > JSON_QUERY(jsonb '[1,[2,3],null]', 'lax $[*][$off]' PASSING 1 AS off WITH > > CONDITIONAL WRAPPER) → 3 > > You're right, good catch. > > I had checked whether the documentation text needed fixing, but failed > to notice that an example > is using CONDITIONAL. Will fix, thanks for the report. I have pushed the fix. Thanks Andreas for the report. -- Thanks, Amit Langote
Re: json_query conditional wrapper bug
On Wed, Sep 11, 2024 at 8:56 PM Peter Eisentraut wrote: > On 11.09.24 13:25, Amit Langote wrote: > > On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut > > wrote: > >> On 11.09.24 09:51, Amit Langote wrote: > >>>>> I've updated your patch to include updated test outputs and a nearby > >>>>> code comment expanded. Do you intend to commit it or do you prefer > >>>>> that I do? > >>>> > >>>> This change looks unrelated: > >>>> > >>>> -ERROR: new row for relation "test_jsonb_constraints" violates check > >>>> constraint "test_jsonb_constraint4" > >>>> +ERROR: new row for relation "test_jsonb_constraints" violates check > >>>> constraint "test_jsonb_constraint5" > >>>> > >>>> Is this some randomness in the way these constraints are evaluated? > >>> > >>> The result of JSON_QUERY() in the CHECK constraint changes, so the > >>> constraint that previously failed now succeeds after this change, > >>> because the comparison looked like this before and after: > >>> > >>> -- before > >>> postgres=# select jsonb '[10]' < jsonb '[10]'; > >>>?column? > >>> -- > >>>f > >>> (1 row) > >>> > >>> -- after > >>> postgres=# select jsonb '10' < jsonb '[10]'; > >>>?column? > >>> -- > >>>t > >>> (1 row) > >>> > >>> That causes the next constraint to be evaluated and its failure > >>> reported instead. > >>> > >>> In the attached, I've adjusted the constraint for the test case to be > >>> a bit more relevant and removed a nearby somewhat redundant test, > >>> mainly because its output changes after the adjustment. > >> > >> Ok, that looks good. Good that we could clear that up a bit. > > > > Thanks for checking. Would you like me to commit it? > > Please do. Done. Thanks for the report and the patch. -- Thanks, Amit Langote
Re: json_query conditional wrapper bug
On Wed, Sep 11, 2024 at 6:57 PM Peter Eisentraut wrote: > On 11.09.24 09:51, Amit Langote wrote: > >>> I've updated your patch to include updated test outputs and a nearby > >>> code comment expanded. Do you intend to commit it or do you prefer > >>> that I do? > >> > >> This change looks unrelated: > >> > >> -ERROR: new row for relation "test_jsonb_constraints" violates check > >> constraint "test_jsonb_constraint4" > >> +ERROR: new row for relation "test_jsonb_constraints" violates check > >> constraint "test_jsonb_constraint5" > >> > >> Is this some randomness in the way these constraints are evaluated? > > > > The result of JSON_QUERY() in the CHECK constraint changes, so the > > constraint that previously failed now succeeds after this change, > > because the comparison looked like this before and after: > > > > -- before > > postgres=# select jsonb '[10]' < jsonb '[10]'; > > ?column? > > -- > > f > > (1 row) > > > > -- after > > postgres=# select jsonb '10' < jsonb '[10]'; > > ?column? > > -- > > t > > (1 row) > > > > That causes the next constraint to be evaluated and its failure > > reported instead. > > > > In the attached, I've adjusted the constraint for the test case to be > > a bit more relevant and removed a nearby somewhat redundant test, > > mainly because its output changes after the adjustment. > > Ok, that looks good. Good that we could clear that up a bit. Thanks for checking. Would you like me to commit it? -- Thanks, Amit Langote
Re: json_query conditional wrapper bug
On Wed, Sep 11, 2024 at 5:15 AM Peter Eisentraut wrote:
> On 10.09.24 10:00, Amit Langote wrote:
> > Sorry for missing this report and thanks Andrew for the offlist heads up.
> >
> > On Wed, Sep 4, 2024 at 7:16 PM Peter Eisentraut
> > wrote:
> >> On 28.08.24 11:21, Peter Eisentraut wrote:
> >>> These are ok:
> >>>
> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper);
> >>>json_query
> >>>
> >>>42
> >>>
> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with
> >>> unconditional wrapper);
> >>>json_query
> >>>
> >>>[42]
> >>>
> >>> But this appears to be wrong:
> >>>
> >>> select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional
> >>> wrapper);
> >>>json_query
> >>>
> >>>[42]
> >>>
> >>> This should return an unwrapped 42.
> >>
> >> If I make the code change illustrated in the attached patch, then I get
> >> the correct result here. And various regression test results change,
> >> which, to me, all look more correct after this patch. I don't know what
> >> the code I removed was supposed to accomplish, but it seems to be wrong
> >> somehow. In the current implementation, the WITH CONDITIONAL WRAPPER
> >> clause doesn't appear to work correctly in any case I could identify.
> >
> > Agreed that this looks wrong.
> >
> > I've wondered why the condition was like that but left it as-is,
> > because I thought at one point that that's needed to ensure that the
> > returned single scalar SQL/JSON item is valid jsonb.
> >
> > I've updated your patch to include updated test outputs and a nearby
> > code comment expanded. Do you intend to commit it or do you prefer
> > that I do?
>
> This change looks unrelated:
>
> -ERROR: new row for relation "test_jsonb_constraints" violates check
> constraint "test_jsonb_constraint4"
> +ERROR: new row for relation "test_jsonb_constraints" violates check
> constraint "test_jsonb_constraint5"
>
> Is this some randomness in the way these constraints are evaluated?
The result of JSON_QUERY() in the CHECK constraint changes, so the
constraint that previously failed now succeeds after this change,
because the comparison looked like this before and after:
-- before
postgres=# select jsonb '[10]' < jsonb '[10]';
?column?
--
f
(1 row)
-- after
postgres=# select jsonb '10' < jsonb '[10]';
?column?
--
t
(1 row)
That causes the next constraint to be evaluated and its failure
reported instead.
In the attached, I've adjusted the constraint for the test case to be
a bit more relevant and removed a nearby somewhat redundant test,
mainly because its output changes after the adjustment.
--
Thanks, Amit Langote
v3-0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch
Description: Binary data
Re: json_query conditional wrapper bug
Sorry for missing this report and thanks Andrew for the offlist heads up.
On Wed, Sep 4, 2024 at 7:16 PM Peter Eisentraut wrote:
> On 28.08.24 11:21, Peter Eisentraut wrote:
> > These are ok:
> >
> > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' without wrapper);
> > json_query
> >
> > 42
> >
> > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with
> > unconditional wrapper);
> > json_query
> >
> > [42]
> >
> > But this appears to be wrong:
> >
> > select json_query('{"a": 1, "b": 42}'::jsonb, 'lax $.b' with conditional
> > wrapper);
> > json_query
> >
> > [42]
> >
> > This should return an unwrapped 42.
>
> If I make the code change illustrated in the attached patch, then I get
> the correct result here. And various regression test results change,
> which, to me, all look more correct after this patch. I don't know what
> the code I removed was supposed to accomplish, but it seems to be wrong
> somehow. In the current implementation, the WITH CONDITIONAL WRAPPER
> clause doesn't appear to work correctly in any case I could identify.
Agreed that this looks wrong.
I've wondered why the condition was like that but left it as-is,
because I thought at one point that that's needed to ensure that the
returned single scalar SQL/JSON item is valid jsonb.
I've updated your patch to include updated test outputs and a nearby
code comment expanded. Do you intend to commit it or do you prefer
that I do?
--
Thanks, Amit Langote
v2-0001-WIP-Fix-JSON_QUERY-WITH-CONDITIONAL-WRAPPER.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Fri, Sep 6, 2024 at 1:34 PM Amit Langote wrote: > On Fri, Sep 6, 2024 at 12:07 PM Amit Langote wrote: > > On Thu, Sep 5, 2024 at 9:58 PM Amit Langote wr > > Pushed. > > Reverted 0002-0004 from both master and REL_17_STABLE due to BF failures. > > 0002-0003 are easily fixed by changing the newly added tests to not > use EXPLAIN VERBOSE to test deparsing related changes, so will re-push > those shortly. Done. > 0004 perhaps doesn't play nicely with LLVM compilation but I don't yet > understand why. Attached is an updated patch that takes care of the issue. The bug was that llvm_compile_expr() didn't like that jump_error, jump_empty, and jump_end could all point to the same step. In the attached, jump_empty / jump_error are left to be -1 if ON ERROR, ON EMPTY steps are not added, instead of making them also point to the step address that jump_end points to. ExecEvalJsonExprPath() are also updated to check if jump_error or jump_empty is -1 and return jump_end if so. -- Thanks, Amit Langote v4-0001-SQL-JSON-Avoid-initializing-unnecessary-ON-ERROR-.patch Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Fri, Sep 6, 2024 at 12:07 PM Amit Langote wrote:
> On Thu, Sep 5, 2024 at 9:58 PM Amit Langote wrote:
> > On Tue, Sep 3, 2024 at 6:05 PM jian he wrote:
> > > On Mon, Sep 2, 2024 at 4:18 PM Amit Langote
> > > wrote:
> > > v2-0001 looks good to me.
> > > +-- Test JSON_TABLE() column deparsing -- don't emit default ON ERROR /
> > > EMPTY
> > > +-- behavior
> > > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text PATH
> > > '$'));
> > > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > > PATH '$') ERROR ON ERROR);
> > >
> > > Are these tests duplicated? appears both in v2-0002 and v2-0003.
> > >
> > > 0002 output is:
> > > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > > PATH '$') ERROR ON ERROR);
> > > +
> > > QUERY PLAN
> > > +
> > > + Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> > > + Output: a
> > > + Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > > json_table_path_0 COLUMNS (a text PATH '$' NULL ON EMPTY NULL ON
> > > ERROR) ERROR ON ERROR)
> > > +(3 rows)
> > >
> > > 0003 output is:
> > > EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > > PATH '$') ERROR ON ERROR);
> > > QUERY PLAN
> > >
> > > Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> > >Output: a
> > >Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > > json_table_path_0 COLUMNS (a text PATH '$') ERROR ON ERROR)
> > > (3 rows)
> > >
> > > two patches with different output,
> > > overall we should merge 0002 and 0003?
> >
> > Looks like I ordered the patches wrong. I'd like to commit the two
> > separately.
> >
> > > if (jsexpr->on_error &&
> > > jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > > (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL ||
> > > returning_domain))
> > >
> > > we can be simplified as
> > > if ( jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > > (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL ||
> > > returning_domain))
> > >
> > > since if jsexpr->on_error is NULL, then segfault will appear at the
> > > beginning of
> > > ExecInitJsonExpr
> >
> > Ok, done.
> >
> > > + *
> > > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > > + * domain type to check the constraints, if any.
> > > */
> > > + jsestate->jump_error = state->steps_len;
> > > if (jsexpr->on_error &&
> > > - jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
> > > + jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > > + (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> > >
> > > + *
> > > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > > + * domain type to check the constraints, if any.
> > > */
> > > + jsestate->jump_empty = state->steps_len;
> > > if (jsexpr->on_empty != NULL &&
> > > - jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
> > > + jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR &&
> > > + (jsexpr->on_empty->btype != JSON_BEHAVIOR_NULL || returning_domain))
> > >
> > > I am a little bit confused with the comments.
> > > not sure the "NULL-valued expression" refers to.
> >
> > It refers to on_error/on_empty->expr, which is a Const node encoding
> > the NULL (constisnull is true) for that behavior.
> >
> > That's actually the case also for behaviors UNKNOWN and EMPTY, so the
> > condition should actually be checking whether the expr is a
> > NULL-valued expression.
> >
> > I've updated the patch.
> >
> > > i think it is:
> > > implicitly default for ON EMPTY | ERROR clause is NULL
> > > (JSON_BEHAVIOR_NULL)
> > > for that situation, we can skip the json coercion process,
> > > but this only applies when the returning type of JsonExpr is not domain,
> >
> > I've reworded the comment to mention that this speeds up the default
> > ON ERROR / EMPTY handling for JSON_QUERY() and JSON_VALUE().
> >
> > Plan to push these tomorrow.
>
> Pushed.
Reverted 0002-0004 from both master and REL_17_STABLE due to BF failures.
0002-0003 are easily fixed by changing the newly added tests to not
use EXPLAIN VERBOSE to test deparsing related changes, so will re-push
those shortly.
0004 perhaps doesn't play nicely with LLVM compilation but I don't yet
understand why.
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
On Thu, Sep 5, 2024 at 9:58 PM Amit Langote wrote:
> On Tue, Sep 3, 2024 at 6:05 PM jian he wrote:
> > On Mon, Sep 2, 2024 at 4:18 PM Amit Langote wrote:
> > v2-0001 looks good to me.
> > +-- Test JSON_TABLE() column deparsing -- don't emit default ON ERROR /
> > EMPTY
> > +-- behavior
> > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text PATH
> > '$'));
> > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > PATH '$') ERROR ON ERROR);
> >
> > Are these tests duplicated? appears both in v2-0002 and v2-0003.
> >
> > 0002 output is:
> > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > PATH '$') ERROR ON ERROR);
> > +
> > QUERY PLAN
> > +
> > + Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> > + Output: a
> > + Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > json_table_path_0 COLUMNS (a text PATH '$' NULL ON EMPTY NULL ON
> > ERROR) ERROR ON ERROR)
> > +(3 rows)
> >
> > 0003 output is:
> > EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > PATH '$') ERROR ON ERROR);
> > QUERY PLAN
> >
> > Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> >Output: a
> >Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > json_table_path_0 COLUMNS (a text PATH '$') ERROR ON ERROR)
> > (3 rows)
> >
> > two patches with different output,
> > overall we should merge 0002 and 0003?
>
> Looks like I ordered the patches wrong. I'd like to commit the two
> separately.
>
> > if (jsexpr->on_error &&
> > jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > we can be simplified as
> > if ( jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL ||
> > returning_domain))
> >
> > since if jsexpr->on_error is NULL, then segfault will appear at the
> > beginning of
> > ExecInitJsonExpr
>
> Ok, done.
>
> > + *
> > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > + * domain type to check the constraints, if any.
> > */
> > + jsestate->jump_error = state->steps_len;
> > if (jsexpr->on_error &&
> > - jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
> > + jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > + (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > + *
> > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > + * domain type to check the constraints, if any.
> > */
> > + jsestate->jump_empty = state->steps_len;
> > if (jsexpr->on_empty != NULL &&
> > - jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
> > + jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR &&
> > + (jsexpr->on_empty->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > I am a little bit confused with the comments.
> > not sure the "NULL-valued expression" refers to.
>
> It refers to on_error/on_empty->expr, which is a Const node encoding
> the NULL (constisnull is true) for that behavior.
>
> That's actually the case also for behaviors UNKNOWN and EMPTY, so the
> condition should actually be checking whether the expr is a
> NULL-valued expression.
>
> I've updated the patch.
>
> > i think it is:
> > implicitly default for ON EMPTY | ERROR clause is NULL (JSON_BEHAVIOR_NULL)
> > for that situation, we can skip the json coercion process,
> > but this only applies when the returning type of JsonExpr is not domain,
>
> I've reworded the comment to mention that this speeds up the default
> ON ERROR / EMPTY handling for JSON_QUERY() and JSON_VALUE().
>
> Plan to push these tomorrow.
Pushed.
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
Hi,
On Tue, Sep 3, 2024 at 6:05 PM jian he wrote:
> On Mon, Sep 2, 2024 at 4:18 PM Amit Langote wrote:
> v2-0001 looks good to me.
> +-- Test JSON_TABLE() column deparsing -- don't emit default ON ERROR / EMPTY
> +-- behavior
> +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text PATH
> '$'));
> +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> PATH '$') ERROR ON ERROR);
>
> Are these tests duplicated? appears both in v2-0002 and v2-0003.
>
> 0002 output is:
> +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> PATH '$') ERROR ON ERROR);
> +
> QUERY PLAN
> +
> + Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> + Output: a
> + Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> json_table_path_0 COLUMNS (a text PATH '$' NULL ON EMPTY NULL ON
> ERROR) ERROR ON ERROR)
> +(3 rows)
>
> 0003 output is:
> EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> PATH '$') ERROR ON ERROR);
> QUERY PLAN
>
> Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
>Output: a
>Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> json_table_path_0 COLUMNS (a text PATH '$') ERROR ON ERROR)
> (3 rows)
>
> two patches with different output,
> overall we should merge 0002 and 0003?
Looks like I ordered the patches wrong. I'd like to commit the two separately.
> if (jsexpr->on_error &&
> jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
>
> we can be simplified as
> if ( jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
>
> since if jsexpr->on_error is NULL, then segfault will appear at the beginning
> of
> ExecInitJsonExpr
Ok, done.
> + *
> + * Only add the extra steps for a NULL-valued expression when RETURNING a
> + * domain type to check the constraints, if any.
> */
> + jsestate->jump_error = state->steps_len;
> if (jsexpr->on_error &&
> - jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
> + jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> + (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
>
> + *
> + * Only add the extra steps for a NULL-valued expression when RETURNING a
> + * domain type to check the constraints, if any.
> */
> + jsestate->jump_empty = state->steps_len;
> if (jsexpr->on_empty != NULL &&
> - jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
> + jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR &&
> + (jsexpr->on_empty->btype != JSON_BEHAVIOR_NULL || returning_domain))
>
> I am a little bit confused with the comments.
> not sure the "NULL-valued expression" refers to.
It refers to on_error/on_empty->expr, which is a Const node encoding
the NULL (constisnull is true) for that behavior.
That's actually the case also for behaviors UNKNOWN and EMPTY, so the
condition should actually be checking whether the expr is a
NULL-valued expression.
I've updated the patch.
> i think it is:
> implicitly default for ON EMPTY | ERROR clause is NULL (JSON_BEHAVIOR_NULL)
> for that situation, we can skip the json coercion process,
> but this only applies when the returning type of JsonExpr is not domain,
I've reworded the comment to mention that this speeds up the default
ON ERROR / EMPTY handling for JSON_QUERY() and JSON_VALUE().
Plan to push these tomorrow.
--
Thanks, Amit Langote
v3-0002-SQL-JSON-Fix-JSON_TABLE-column-deparsing.patch
Description: Binary data
v3-0001-Update-comment-about-ExprState.escontext.patch
Description: Binary data
v3-0004-SQL-JSON-Avoid-initializing-unnecessary-ON-ERROR-.patch
Description: Binary data
v3-0003-SQL-JSON-Fix-default-ON-ERROR-behavior-for-JSON_T.patch
Description: Binary data
Re: generic plans and "initial" pruning
On Sat, Aug 31, 2024 at 9:30 PM Junwang Zhao wrote:
> @@ -1241,7 +1244,7 @@ GetCachedPlan(CachedPlanSource *plansource,
> ParamListInfo boundParams,
> if (customplan)
> {
> /* Build a custom plan */
> - plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
> + plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv, true);
>
> Is the *true* here a typo? Seems it should be *false* for custom plan?
That's correct, thanks for catching that. Will fix.
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
On Fri, Aug 30, 2024 at 4:32 PM Amit Langote wrote:
> On Thu, Aug 22, 2024 at 12:44 PM Amit Langote wrote:
> > On Thu, Aug 22, 2024 at 11:02 jian he wrote:
> >> On Tue, Jul 30, 2024 at 12:59 PM Amit Langote
> >> wrote:
> >> > On Fri, Jul 26, 2024 at 11:19 PM jian he
> >> > wrote:
> >> > > {
> >> > > ...
> >> > > /*
> >> > > * For expression nodes that support soft errors. Should be set
> >> > > to NULL
> >> > > * before calling ExecInitExprRec() if the caller wants errors
> >> > > thrown.
> >> > > */
> >> > > ErrorSaveContext *escontext;
> >> > > } ExprState;
> >> > >
> >> > > i believe by default makeNode will set escontext to NULL.
> >> > > So the comment should be, if you want to catch the soft errors, make
> >> > > sure the escontext pointing to an allocated ErrorSaveContext.
> >> > > or maybe just say, default is NULL.
> >> > >
> >> > > Otherwise, the original comment's meaning feels like: we need to
> >> > > explicitly set it to NULL
> >> > > for certain operations, which I believe is false?
> >> >
> >> > OK, I'll look into updating this.
>
> See 0001.
>
> >> > > json_behavior_type:
> >> > > ERROR_P{ $$ = JSON_BEHAVIOR_ERROR; }
> >> > > | NULL_P{ $$ = JSON_BEHAVIOR_NULL; }
> >> > > | TRUE_P{ $$ = JSON_BEHAVIOR_TRUE; }
> >> > > | FALSE_P{ $$ = JSON_BEHAVIOR_FALSE; }
> >> > > | UNKNOWN{ $$ = JSON_BEHAVIOR_UNKNOWN; }
> >> > > | EMPTY_P ARRAY{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> >> > > | EMPTY_P OBJECT_P{ $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
> >> > > /* non-standard, for Oracle compatibility only */
> >> > > | EMPTY_P{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> >> > > ;
> >> > >
> >> > > EMPTY_P behaves the same as EMPTY_P ARRAY
> >> > > so for function GetJsonBehaviorConst, the following "case
> >> > > JSON_BEHAVIOR_EMPTY:" is wrong?
> >> > >
> >> > > case JSON_BEHAVIOR_NULL:
> >> > > case JSON_BEHAVIOR_UNKNOWN:
> >> > > case JSON_BEHAVIOR_EMPTY:
> >> > > val = (Datum) 0;
> >> > > isnull = true;
> >> > > typid = INT4OID;
> >> > > len = sizeof(int32);
> >> > > isbyval = true;
> >> > > break;
> >> > >
> >> > > also src/backend/utils/adt/ruleutils.c
> >> > > if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
> >> > > get_json_behavior(jexpr->on_error, context, "ERROR");
> >> >
> >> > Something like the attached makes sense? While this meaningfully
> >> > changes the deparsing output, there is no behavior change for
> >> > JsonTable top-level path execution. That's because the behavior when
> >> > there's an error in the execution of the top-level path is to throw it
> >> > or return an empty set, which is handled in jsonpath_exec.c, not
> >> > execExprInterp.c.
>
> See 0002.
>
> I'm also attaching 0003 to fix a minor annoyance that JSON_TABLE()
> columns' default ON ERROR, ON EMPTY behaviors are unnecessarily
> emitted in the deparsed output when the top-level ON ERROR behavior is
> ERROR.
>
> Will push these on Monday.
Didn't as there's a release freeze in effect for the v17 branch. Will
push to both master and v17 once the freeze is over.
> I haven't had a chance to take a closer look at your patch to optimize
> the code in ExecInitJsonExpr() yet.
I've simplified your patch a bit and attached it as 0004.
--
Thanks, Amit Langote
v2-0002-SQL-JSON-Fix-default-ON-ERROR-behavior-for-JSON_T.patch
Description: Binary data
v2-0004-SQL-JSON-Avoid-initializing-unnecessary-ON-ERROR-.patch
Description: Binary data
v2-0003-SQL-JSON-Fix-JSON_TABLE-column-deparsing.patch
Description: Binary data
v2-0001-Update-comment-about-ExprState.escontext.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Thu, Aug 22, 2024 at 12:44 PM Amit Langote wrote:
> On Thu, Aug 22, 2024 at 11:02 jian he wrote:
>> On Tue, Jul 30, 2024 at 12:59 PM Amit Langote
>> wrote:
>> > On Fri, Jul 26, 2024 at 11:19 PM jian he
>> > wrote:
>> > > {
>> > > ...
>> > > /*
>> > > * For expression nodes that support soft errors. Should be set to
>> > > NULL
>> > > * before calling ExecInitExprRec() if the caller wants errors
>> > > thrown.
>> > > */
>> > > ErrorSaveContext *escontext;
>> > > } ExprState;
>> > >
>> > > i believe by default makeNode will set escontext to NULL.
>> > > So the comment should be, if you want to catch the soft errors, make
>> > > sure the escontext pointing to an allocated ErrorSaveContext.
>> > > or maybe just say, default is NULL.
>> > >
>> > > Otherwise, the original comment's meaning feels like: we need to
>> > > explicitly set it to NULL
>> > > for certain operations, which I believe is false?
>> >
>> > OK, I'll look into updating this.
See 0001.
>> > > json_behavior_type:
>> > > ERROR_P{ $$ = JSON_BEHAVIOR_ERROR; }
>> > > | NULL_P{ $$ = JSON_BEHAVIOR_NULL; }
>> > > | TRUE_P{ $$ = JSON_BEHAVIOR_TRUE; }
>> > > | FALSE_P{ $$ = JSON_BEHAVIOR_FALSE; }
>> > > | UNKNOWN{ $$ = JSON_BEHAVIOR_UNKNOWN; }
>> > > | EMPTY_P ARRAY{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
>> > > | EMPTY_P OBJECT_P{ $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
>> > > /* non-standard, for Oracle compatibility only */
>> > > | EMPTY_P{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
>> > > ;
>> > >
>> > > EMPTY_P behaves the same as EMPTY_P ARRAY
>> > > so for function GetJsonBehaviorConst, the following "case
>> > > JSON_BEHAVIOR_EMPTY:" is wrong?
>> > >
>> > > case JSON_BEHAVIOR_NULL:
>> > > case JSON_BEHAVIOR_UNKNOWN:
>> > > case JSON_BEHAVIOR_EMPTY:
>> > > val = (Datum) 0;
>> > > isnull = true;
>> > > typid = INT4OID;
>> > > len = sizeof(int32);
>> > > isbyval = true;
>> > > break;
>> > >
>> > > also src/backend/utils/adt/ruleutils.c
>> > > if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
>> > > get_json_behavior(jexpr->on_error, context, "ERROR");
>> >
>> > Something like the attached makes sense? While this meaningfully
>> > changes the deparsing output, there is no behavior change for
>> > JsonTable top-level path execution. That's because the behavior when
>> > there's an error in the execution of the top-level path is to throw it
>> > or return an empty set, which is handled in jsonpath_exec.c, not
>> > execExprInterp.c.
See 0002.
I'm also attaching 0003 to fix a minor annoyance that JSON_TABLE()
columns' default ON ERROR, ON EMPTY behaviors are unnecessarily
emitted in the deparsed output when the top-level ON ERROR behavior is
ERROR.
Will push these on Monday.
I haven't had a chance to take a closer look at your patch to optimize
the code in ExecInitJsonExpr() yet.
--
Thanks, Amit Langote
v1-0002-SQL-JSON-Fix-default-ON-ERROR-behavior-for-JSON_T.patch
Description: Binary data
v1-0001-Update-comment-about-ExprState.escontext.patch
Description: Binary data
v1-0003-SQL-JSON-Fix-JSON_TABLE-column-deparsing.patch
Description: Binary data
Re: generic plans and "initial" pruning
On Wed, Aug 21, 2024 at 10:10 PM Robert Haas wrote: > On Wed, Aug 21, 2024 at 8:45 AM Amit Langote wrote: > > * The replanning aspect of the lock-in-the-executor design would be > > simpler if a CachedPlan contained the plan for a single query rather > > than a list of queries, as previously mentioned. This is particularly > > due to the requirements of the PORTAL_MULTI_QUERY case. However, this > > option might be impractical. > > It might be, but maybe it would be worth a try? I mean, > GetCachedPlan() seems to just call pg_plan_queries() which just loops > over the list of query trees and does the same thing for each one. If > we wanted to replan a single query, why couldn't we do > fake_querytree_list = list_make1(list_nth(querytree_list, n)) and then > call pg_plan_queries(fake_querytree_list)? Or something equivalent to > that. We could have a new GetCachedSinglePlan(cplan, n) to do this. I've been hacking to prototype this, and it's showing promise. It helps make the replan loop at the call sites that start the executor with an invalidatable plan more localized and less prone to action-at-a-distance issues. However, the interface and contract of the new function in my prototype are pretty specialized for the replan loop in this context—meaning it's not as general-purpose as GetCachedPlan(). Essentially, what you get when you call it is a 'throwaway' CachedPlan containing only the plan for the query that failed during ExecutorStart(), not a plan integrated into the original CachedPlanSource's stmt_list. A call site entering the replan loop will retry the execution with that throwaway plan, release it once done, and resume looping over the plans in the original list. The invalid plan that remains in the original list will be discarded and replanned in the next call to GetCachedPlan() using the same CachedPlanSource. While that may sound undesirable, I'm inclined to think it's not something that needs optimization, given that we're expecting this code path to be taken rarely. I'll post a version of a revamped locks-in-the-executor patch set using the above function after debugging some more. -- Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
On Thu, Aug 22, 2024 at 11:02 jian he wrote:
> On Tue, Jul 30, 2024 at 12:59 PM Amit Langote
> wrote:
> >
> > Hi,
> >
> > On Fri, Jul 26, 2024 at 11:19 PM jian he
> wrote:
> > > On Fri, Jul 26, 2024 at 4:53 PM Amit Langote
> wrote:
> > > >
> > > >
> > > > Pushed 0003-0005 ahead of 0001-0002. Will try to push them over the
> > > > weekend. Rebased for now.
> >
> > Pushed them now.
> >
> > > {
> > > ...
> > > /*
> > > * For expression nodes that support soft errors. Should be set
> to NULL
> > > * before calling ExecInitExprRec() if the caller wants errors
> thrown.
> > > */
> > > ErrorSaveContext *escontext;
> > > } ExprState;
> > >
> > > i believe by default makeNode will set escontext to NULL.
> > > So the comment should be, if you want to catch the soft errors, make
> > > sure the escontext pointing to an allocated ErrorSaveContext.
> > > or maybe just say, default is NULL.
> > >
> > > Otherwise, the original comment's meaning feels like: we need to
> > > explicitly set it to NULL
> > > for certain operations, which I believe is false?
> >
> > OK, I'll look into updating this.
> >
> > > struct
> > > {
> > > Oidtargettype;
> > > int32targettypmod;
> > > boolomit_quotes;
> > > boolexists_coerce;
> > > boolexists_cast_to_int;
> > > boolcheck_domain;
> > > void *json_coercion_cache;
> > > ErrorSaveContext *escontext;
> > > }jsonexpr_coercion;
> > > do we need to comment that "check_domain" is only used for
> JSON_EXISTS_OP?
> >
> > I've renamed it to exists_check_domain and added a comment that
> > exists_* fields are relevant only for JSON_EXISTS_OP.
> >
> > > json_behavior_type:
> > > ERROR_P{ $$ = JSON_BEHAVIOR_ERROR; }
> > > | NULL_P{ $$ = JSON_BEHAVIOR_NULL; }
> > > | TRUE_P{ $$ = JSON_BEHAVIOR_TRUE; }
> > > | FALSE_P{ $$ = JSON_BEHAVIOR_FALSE; }
> > > | UNKNOWN{ $$ = JSON_BEHAVIOR_UNKNOWN; }
> > > | EMPTY_P ARRAY{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> > > | EMPTY_P OBJECT_P{ $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
> > > /* non-standard, for Oracle compatibility only */
> > > | EMPTY_P{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> > > ;
> > >
> > > EMPTY_P behaves the same as EMPTY_P ARRAY
> > > so for function GetJsonBehaviorConst, the following "case
> > > JSON_BEHAVIOR_EMPTY:" is wrong?
> > >
> > > case JSON_BEHAVIOR_NULL:
> > > case JSON_BEHAVIOR_UNKNOWN:
> > > case JSON_BEHAVIOR_EMPTY:
> > > val = (Datum) 0;
> > > isnull = true;
> > > typid = INT4OID;
> > > len = sizeof(int32);
> > > isbyval = true;
> > > break;
> > >
> > > also src/backend/utils/adt/ruleutils.c
> > > if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
> > > get_json_behavior(jexpr->on_error, context, "ERROR");
> >
> > Something like the attached makes sense? While this meaningfully
> > changes the deparsing output, there is no behavior change for
> > JsonTable top-level path execution. That's because the behavior when
> > there's an error in the execution of the top-level path is to throw it
> > or return an empty set, which is handled in jsonpath_exec.c, not
> > execExprInterp.c.
> >
>
> hi amit.
> seems you forgot to attach the patch?
Yeah, I had planned to look at this after my vacation earlier this month,
but I context switched into working on another project and lost track of
this. I’ll make some time next week to fix whatever remains go be fixed
here. Thanks for the reminder.
>
Re: generic plans and "initial" pruning
On Tue, Aug 20, 2024 at 11:53 PM Robert Haas wrote: > On Tue, Aug 20, 2024 at 9:00 AM Amit Langote wrote: > > I think we'd modify plancache.c to postpone the locking of only > > prunable relations (i.e., partitions), so we're looking at only a > > handful of concurrent modifications that are going to cause execution > > errors. That's because we disallow many DDL modifications of > > partitions unless they are done via recursion from the parent, so the > > space of errors in practice would be smaller compared to if we were to > > postpone *all* cached plan locks to ExecInitNode() time. DROP INDEX > > a_partion_only_index comes to mind as something that might cause an > > error. I've not tested if other partition-only constraints can cause > > unsafe behaviors. > > This seems like a valid point to some extent, but in other contexts > we've had discussions about how we don't actually guarantee all that > much uniformity between a partitioned table and its partitions, and > it's been questioned whether we made the right decisions there. So I'm > not entirely sure that the surface area for problems here will be as > narrow as you're hoping -- I think we'd need to go through all of the > ALTER TABLE variants and think it through. But maybe the problems > aren't that bad. Many changeable properties that are reflected in the RelationData of a partition after getting the lock on it seem to cause no issues as long as the executor code only looks at RelationData, which is true for most Scan nodes. It also seems true for ModifyTable which looks into RelationData for relation properties relevant to insert/deletes. The two things that don't cope are: * Index Scan nodes with concurrent DROP INDEX of partition-only indexes. * Concurrent DROP CONSTRAINT of partition-only CHECK and NOT NULL constraints can lead to incorrect result as I write below. > It does seem like constraints can change the plan. Imagine the > partition had a CHECK(false) constraint before and now doesn't, or > something. Yeah, if the CHECK constraint gets dropped concurrently, any new rows that got added after that will not be returned by executing a stale cached plan, because the plan would have been created based on the assumption that such rows shouldn't be there due to the CHECK constraint. We currently don't explicitly check that the constraints that were used during planning still exist before executing the plan. Overall, I'm starting to feel less enthused by the idea throwing an error in the executor due to known and unknown hazards of trying to execute a stale plan. Even if we made a note in the docs of such hazards, any users who run into these rare errors are likely to head to -bugs or -hackers anyway. Tom said we should perhaps look at the hazards caused by intra-session locking, but we'd still be left with the hazards of missing index and constraints, AFAICS, due to DROP from other sessions. So, the options: * The replanning aspect of the lock-in-the-executor design would be simpler if a CachedPlan contained the plan for a single query rather than a list of queries, as previously mentioned. This is particularly due to the requirements of the PORTAL_MULTI_QUERY case. However, this option might be impractical. * Polish the patch for the old design of doing the initial pruning before AcquireExecutorLocks() and focus on hashing out any bugs and issues of that design. -- Thanks, Amit Langote
Re: generic plans and "initial" pruning
On Tue, Aug 20, 2024 at 3:21 AM Robert Haas wrote: > On Mon, Aug 19, 2024 at 1:52 PM Tom Lane wrote: > > Robert Haas writes: > > > But that seems somewhat incidental to what this thread is about. > > > > Perhaps. But if we're running into issues related to that, it might > > be good to set aside the long-term goal for a bit and come up with > > a cleaner answer for intra-session locking. That could allow the > > pruning problem to be solved more cleanly in turn, and it'd be > > an improvement even if not. > > Maybe, but the pieces aren't quite coming together for me. Solving > this would mean that if we execute a stale plan, we'd be more likely > to get a good error and less likely to get a bad, nasty-looking > internal error, or a crash. That's good on its own terms, but we don't > really want user queries to produce errors at all, so I don't think > we'd feel any more free to rearrange the order of operations than we > do today. Yeah, it's unclear whether executing a potentially stale plan is an acceptable tradeoff compared to replanning, especially if it occurs rarely. Personally, I would prefer that it is. > > > Do you have a view on what the way forward might be? > > > > I'm fresh out of ideas at the moment, other than having a hope that > > divide-and-conquer (ie, solving subproblems first) might pay off. > > Fair enough, but why do you think that the original approach of > creating a data structure from within the plan cache mechanism > (probably via a call into some new executor entrypoint) and then > feeding that through to ExecutorRun() time can't work? That would be ExecutorStart(). The data structure need not be referenced after ExecInitNode(). > Is it possible > you latched onto some non-optimal decisions that the early versions of > the patch made, rather than there being a fundamental problem with the > concept? > > I actually thought the do-it-at-executorstart-time approach sounded > pretty good, even though we might have to abandon planstate tree > initialization partway through, right up until Amit started talking > about moving ExecutorStart() from PortalRun() to PortalStart(), which > I have a feeling is going to create a bigger problem than we can > solve. I think if we want to save that approach, we should try to > figure out if we can teach the plancache to replan one query from a > list without replanning the others, which seems like it might allow us > to keep the order of major operations unchanged. Otherwise, it makes > sense to me to have another go at the other approach, at least to make > sure we understand clearly why it can't work. +1 -- Thanks, Amit Langote
Re: generic plans and "initial" pruning
On Tue, Aug 20, 2024 at 1:39 AM Robert Haas wrote: > On Fri, Aug 16, 2024 at 8:36 AM Amit Langote wrote: > > So it is possible for the executor to try to run a plan that has > > become invalid since it was created, so... > > I'm not sure what the "so what" here is. I meant that if the executor has to deal with broken plans anyway, we might as well lean into that fact by choosing not to handle only the cached plan case in a certain way. Yes, I understand that that's not a good justification. > > One perhaps crazy idea [1]: > > > > What if we remove AcquireExecutorLocks() and move the responsibility > > of taking the remaining necessary locks into the executor (those on > > any inheritance children that are added during planning and thus not > > accounted for by AcquirePlannerLocks()), like the patch already does, > > but don't make it also check if the plan has become invalid, which it > > can't do anyway unless it's from a CachedPlan. That means we instead > > let the executor throw any errors that occur when trying to either > > initialize the plan because of the changes that have occurred to the > > objects referenced in the plan, like what is happening in the above > > example. If that case is going to be rare anway, why spend energy on > > checking the validity and replan, especially if that's not an easy > > thing to do as we're finding out. In the above example, we could say > > that it's a user error to create a rule like that, so it should not > > happen in practice, but when it does, the executor seems to deal with > > it correctly by refusing to execute a broken plan . Perhaps it's more > > worthwhile to make the executor behave correctly in face of plan > > invalidation than teach the rest of the system to deal with the > > executor throwing its hands up when it runs into an invalid plan? > > Again, I think this may be a crazy line of thinking but just wanted to > > get it out there. > > I don't know whether this is crazy or not. I think there are two > issues. One, the set of checks that we have right now might not be > complete, and we might just not have realized that because it happens > infrequently enough that we haven't found all the bugs. If that's so, > then a change like this could be a good thing, because it might force > us to fix stuff we should be fixing anyway. I have a feeling that some > of the checks you hit there were added as bug fixes long after the > code was written originally, so my confidence that we don't have more > bugs isn't especially high. This makes sense. > And two, it matters a lot how frequent the errors will be in practice. > I think we normally try to replan rather than let a stale plan be used > because we want to not fail, because users don't like failure. If the > design you propose here would make failures more (or less) frequent, > then that's a problem (or awesome). I think we'd modify plancache.c to postpone the locking of only prunable relations (i.e., partitions), so we're looking at only a handful of concurrent modifications that are going to cause execution errors. That's because we disallow many DDL modifications of partitions unless they are done via recursion from the parent, so the space of errors in practice would be smaller compared to if we were to postpone *all* cached plan locks to ExecInitNode() time. DROP INDEX a_partion_only_index comes to mind as something that might cause an error. I've not tested if other partition-only constraints can cause unsafe behaviors. Perhaps, we can add the check for CachedPlan.is_valid after every table_open() and index_open() in the executor that takes a lock or at all the places we discussed previously and throw the error (say: "cached plan is no longer valid") if it's false. That's better than running into and throwing into some random error by soldiering ahead with its initialization / execution, but still a loss in terms of user experience because we're adding a new failure mode, however rare. -- Thanks, Amit Langote
Re: generic plans and "initial" pruning
On Fri, Aug 16, 2024 at 12:35 AM Robert Haas wrote: > On Thu, Aug 15, 2024 at 8:57 AM Amit Langote wrote: > > TBH, it's more of a hunch that people who are not involved in this > > development might find the new reality, whereby the execution is not > > racefree until ExecutorRun(), hard to reason about. > > I'm confused by what you mean here by "racefree". A race means > multiple sessions are doing stuff at the same time and the result > depends on who does what first, but the executor stuff is all > backend-private. Heavyweight locks are not backend-private, but those > would be taken in ExectorStart(), not ExecutorRun(), IIUC. Sorry, yes, I meant ExecutorStart(). A backend that wants to execute a plan tree from a CachedPlan is in a race with other backends that might modify tables before ExecutorStart() takes the remaining locks. That race window is bigger when it is ExecutorStart() that will take the locks, and I don't mean in terms of timing, but in terms of the other code that can run in between GetCachedPlan() returning a partially valid plan and ExecutorStart() takes the remaining locks depending on the calling module. > > With the patch, CreateQueryDesc() and ExecutorStart() are moved to > > PortalStart() so that QueryDescs including the PlanState trees for all > > queries are built before any is run. Why? So that if ExecutorStart() > > fails for any query in the list, we can simply throw out the QueryDesc > > and the PlanState trees of the previous queries (NOT run them) and ask > > plancache for a new CachedPlan for the list of queries. We don't have > > a way to ask plancache.c to replan only a given query in the list. > > I agree that moving this from PortalRun() to PortalStart() seems like > a bad idea, especially in view of what you write below. > > > * There's no longer CCI() between queries in PortalRunMulti() because > > the snapshots in each query's QueryDesc must have been adjusted to > > reflect the correct command counter. I've checked but can't really be > > sure if the value in the snapshot is all anyone ever uses if they want > > to know the current value of the command counter. > > I don't think anything stops somebody wanting to look at the current > value of the command counter. I also don't think you can remove the > CommandCounterIncrement() calls between successive queries, because > then they won't see the effects of earlier calls. So this sounds > broken to me. I suppose you mean CCI between "running" (calling ExecutorRun on) successive queries. Then the patch is indeed broken. If we're to make that right, the number of CCIs for the multi-query portals will have to double given the separation of ExecutorStart() and ExecutorRun() phases. > Also keep in mind that one of the queries could call a function which > does something that bumps the command counter again. I'm not sure if > that creates its own hazzard separate from the lack of CCIs, or > whether it's just another part of that same issue. But you can't > assume that each query's snapshot should have a command counter value > one more than the previous query. > > While this all seems bad for the partially-initialized-execution-tree > approach, I wonder if you don't have problems here with the other > design, too. Let's say you've the multi-query case and there are 2 > queries. The first one (Q1) is SELECT mysterious_function() and the > second one (Q2) is SELECT * FROM range_partitioned_table WHERE > key_column = 42. What if mysterious_function() performs DDL on > range_partitioned_table? I haven't tested this so maybe there are > things going on here that prevent trouble, but it seems like executing > Q1 can easily invalidate the plan for Q2. And then it seems like > you're basically back to the same problem. A rule (but not views AFAICS) can lead to the multi-query case (there might be other ways). I tried the following, and, yes, the plan for the query queued by the rule is broken by the execution of that for the 1st query: create table foo (a int); create table bar (a int); create or replace function foo_trig_func () returns trigger as $$ begin drop table bar cascade; return new.*; end; $$ language plpgsql; create trigger foo_trig before insert on foo execute function foo_trig_func(); create rule insert_foo AS ON insert TO foo do also insert into bar values (new.*); set plan_cache_mode to force_generic_plan ; prepare q as insert into foo values (1); execute q; NOTICE: drop cascades to rule insert_foo on table foo ERROR: relation with OID 16418 does not exist The ERROR comes from trying to run (actually "initialize") the cached plan for `insert into bar values (new.*);`
Re: generic plans and "initial" pruning
On Thu, Jun 20, 2024 at 2:09 AM Alvaro Herrera wrote: > I hope we can get this new executor code in 18. Thanks for doing the benchmark, Alvaro, and sorry for the late reply. Yes, I'm hoping to get *some* version of this into v18. I've been thinking how to move this forward and I'm starting to think that we should go back to or at least consider as an option the old approach of changing the plancache to do the initial runtime pruning instead of changing the executor to take locks, which is the design that the latest patch set tries to implement. Here are the challenges facing the implementation of the current design: 1. I went through many iterations of the changes to ExecInitNode() to return a partially initialized PlanState tree when it detects that the CachedPlan was invalidated after locking a child table and to ExecEndNode() to account for the PlanState tree sometimes being partially initialized, but it still seems fragile and bug-prone to me. It might be because this approach is fundamentally hard to get right or I haven't invested enough effort in becoming more confident in its robustness. 2. Refactoring needed due to the ExecutorStart() API change especially that pertaining to portals does not seem airtight. I'm especially worried about moving the ExecutorStart() call for the PORTAL_MULTI_QUERY case from where it is currently to PortalStart(). That requires additional bookkeeping in PortalData and I am not totally sure that the snapshot handling changes after that move are entirely correct. 3. The need to add *back* the fields to store the RT indexes of relations that are not looked at by ExecInitNode() traversal such as root partitioned tables and non-leaf partitions. I'm worried about #2 the most. One complaint about the previous design was that the interface changes to capture and pass the result of doing initial pruning in plancache.c to the executor did not look great. However, after having tried doing #2, the changes to pass the pruning result into the executor and changes to reuse it in ExecInit[Merge]Append() seem a tad bit simpler than the refactoring and adjustments needed to handle failed ExecutorStart() calls, at multiple code sites. About #1, I tend to agree with David that adding complexity around PlanState tree construction may not be a good idea, because we might want to rethink Plan initialization code and data structures in the not too distant future. One idea I thought of is to take the remaining locks (to wit, those on inheritance children if running a cached plan) at the beginning of InitPlan(), that is before ExecInitNode(), like we handle the permission checking, so that we don't need to worry about ever returning a partially initialized PlanState tree. However, we're still left with the tall task to implement #2 such that it doesn't break anything. Another concern about the old design was the unnecessary overhead of initializing bitmapset fields in PlannedStmt that are meant for the locking algorithm in AcquireExecutorLocks(). Andres suggested an idea offlist to either piggyback on cursorOptions argument of pg_plan_queries() or adding a new boolean parameter to let the planner know if the plan is one that might get cached and thus have AcquireExecutorLocks() called on it. Another idea David and I discussed offlist is inventing a RTELockInfo (cf RTEPermissionInfo) and only creating one for each RT entry that is un-prunable and do away with PlannedStmt.rtable. For partitioned tables, that entry will point to the PartitionPruneInfo that will contain the RT indexes of partitions (or maybe just OIDs) mapped from their subplan indexes that are returned by the pruning code. So AcquireExecutorLocks() will lock all un-prunable relations by referring to their RTELockInfo entries and for each entry that points to a PartitionPruneInfo with initial pruning steps, will only lock the partitions that survive the pruning. I am planning to polish that old patch set and post after playing with those new ideas. -- Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
Hi,
On Fri, Jul 26, 2024 at 11:19 PM jian he wrote:
> On Fri, Jul 26, 2024 at 4:53 PM Amit Langote wrote:
> >
> >
> > Pushed 0003-0005 ahead of 0001-0002. Will try to push them over the
> > weekend. Rebased for now.
Pushed them now.
> {
> ...
> /*
> * For expression nodes that support soft errors. Should be set to NULL
> * before calling ExecInitExprRec() if the caller wants errors thrown.
> */
> ErrorSaveContext *escontext;
> } ExprState;
>
> i believe by default makeNode will set escontext to NULL.
> So the comment should be, if you want to catch the soft errors, make
> sure the escontext pointing to an allocated ErrorSaveContext.
> or maybe just say, default is NULL.
>
> Otherwise, the original comment's meaning feels like: we need to
> explicitly set it to NULL
> for certain operations, which I believe is false?
OK, I'll look into updating this.
> struct
> {
> Oidtargettype;
> int32targettypmod;
> boolomit_quotes;
> boolexists_coerce;
> boolexists_cast_to_int;
> boolcheck_domain;
> void *json_coercion_cache;
> ErrorSaveContext *escontext;
> }jsonexpr_coercion;
> do we need to comment that "check_domain" is only used for JSON_EXISTS_OP?
I've renamed it to exists_check_domain and added a comment that
exists_* fields are relevant only for JSON_EXISTS_OP.
> json_behavior_type:
> ERROR_P{ $$ = JSON_BEHAVIOR_ERROR; }
> | NULL_P{ $$ = JSON_BEHAVIOR_NULL; }
> | TRUE_P{ $$ = JSON_BEHAVIOR_TRUE; }
> | FALSE_P{ $$ = JSON_BEHAVIOR_FALSE; }
> | UNKNOWN{ $$ = JSON_BEHAVIOR_UNKNOWN; }
> | EMPTY_P ARRAY{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> | EMPTY_P OBJECT_P{ $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
> /* non-standard, for Oracle compatibility only */
> | EMPTY_P{ $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> ;
>
> EMPTY_P behaves the same as EMPTY_P ARRAY
> so for function GetJsonBehaviorConst, the following "case
> JSON_BEHAVIOR_EMPTY:" is wrong?
>
> case JSON_BEHAVIOR_NULL:
> case JSON_BEHAVIOR_UNKNOWN:
> case JSON_BEHAVIOR_EMPTY:
> val = (Datum) 0;
> isnull = true;
> typid = INT4OID;
> len = sizeof(int32);
> isbyval = true;
> break;
>
> also src/backend/utils/adt/ruleutils.c
> if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
> get_json_behavior(jexpr->on_error, context, "ERROR");
Something like the attached makes sense? While this meaningfully
changes the deparsing output, there is no behavior change for
JsonTable top-level path execution. That's because the behavior when
there's an error in the execution of the top-level path is to throw it
or return an empty set, which is handled in jsonpath_exec.c, not
execExprInterp.c.
> for json_value, json_query, i believe we can save some circles in
> ExecInitJsonExpr
> if you don't specify on error, on empty
>
> can you please check the attached, based on your latest attachment.
Perhaps makes sense, though I haven't checked closely. I'll take a
look next week.
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
On Fri, Jul 26, 2024 at 11:12 AM Amit Langote wrote: > On Thu, Jul 25, 2024 at 11:16 PM Amit Langote wrote: > > On Wed, Jul 24, 2024 at 3:25 PM jian he wrote: > > > 2. domain over jsonb should fail just like domain over other types? > > > RETURNING djs keep quotes DEFAULT '"11"' ON empty > > > should fail as > > > ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING > > > type > > > DETAIL: value for domain djs violates check constraint "djs_check"" > > > > I think this should be fixed with the attached patch 0004. > > It is fixed but with the patch 0003, not 0004. > > Also, the test cases in 0004, which is a patch to fix a problem with > OMIT QUOTES being disregarded when RETURNING domain-over-jsonb, didn't > test that problem. So I have updated the test case to use a domain > over jsonb. Pushed 0003-0005 ahead of 0001-0002. Will try to push them over the weekend. Rebased for now. -- Thanks, Amit Langote 0001-SQL-JSON-Some-fixes-to-JsonBehavior-expression-casti.patch Description: Binary data 0002-SQL-JSON-Fix-casting-for-integer-EXISTS-columns-in-J.patch Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Thu, Jul 25, 2024 at 11:16 PM Amit Langote wrote: > On Wed, Jul 24, 2024 at 3:25 PM jian he wrote: > > 2. domain over jsonb should fail just like domain over other types? > > RETURNING djs keep quotes DEFAULT '"11"' ON empty > > should fail as > > ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type > > DETAIL: value for domain djs violates check constraint "djs_check"" > > I think this should be fixed with the attached patch 0004. It is fixed but with the patch 0003, not 0004. Also, the test cases in 0004, which is a patch to fix a problem with OMIT QUOTES being disregarded when RETURNING domain-over-jsonb, didn't test that problem. So I have updated the test case to use a domain over jsonb. -- Thanks, Amit Langote 0004-SQL-JSON-Respect-OMIT-QUOTES-when-RETURNING-domains-.patch Description: Binary data 0005-SQL-JSON-Remove-useless-code-in-ExecInitJsonExpr.patch Description: Binary data 0001-SQL-JSON-Some-fixes-to-JsonBehavior-expression-casti.patch Description: Binary data 0003-SQL-JSON-Fix-handling-of-errors-coercing-JsonBehavio.patch Description: Binary data 0002-SQL-JSON-Fix-casting-for-integer-EXISTS-columns-in-J.patch Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Wed, Jul 24, 2024 at 3:25 PM jian he wrote:
> transformJsonFuncExpr we have:
> case JSON_QUERY_OP:
> if (jsexpr->returning->typid != JSONBOID || jsexpr->omit_quotes)
> jsexpr->use_json_coercion = true;
>
> case JSON_VALUE_OP:
> if (jsexpr->returning->typid != TEXTOID)
> {
> if (get_typtype(jsexpr->returning->typid) == TYPTYPE_DOMAIN &&
> DomainHasConstraints(jsexpr->returning->typid))
> jsexpr->use_json_coercion = true;
> else
> jsexpr->use_io_coercion = true;
> }
>
> JSONBOID won't be a domain. for domain type, json_value, json_query
> will use jsexpr->use_json_coercion.
> jsexpr->use_json_coercion can handle whether the domain has constraints or
> not.
>
> so i don't know the purpose of following code in ExecInitJsonExpr
> if (get_typtype(jsexpr->returning->typid) == TYPTYPE_DOMAIN &&
> DomainHasConstraints(jsexpr->returning->typid))
> {
> Assert(jsexpr->use_json_coercion);
> scratch->opcode = EEOP_JUMP;
> scratch->d.jump.jumpdone = state->steps_len + 1;
> ExprEvalPushStep(state, scratch);
> }
Yeah, it's a useless JUMP. I forget why it's there. I have attached
a patch (0005) to remove it.
> json_table exits works fine with int4, not domain over int4. The
> following are test suites.
>
> drop domain if exists dint4, dint4_1,dint4_0;
> create domain dint4 as int;
> create domain dint4_1 as int check ( value <> 1 );
> create domain dint4_0 as int check ( value <> 0 );
> SELECT a, a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4
> EXISTS PATH '$.a' ));
> SELECT a, a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4
> EXISTS PATH '$.a' false ON ERROR));
> SELECT a, a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4
> EXISTS PATH '$.a' ERROR ON ERROR));
> SELECT a, a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4_0
> EXISTS PATH '$.a'));
> SELECT a, a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4_0
> EXISTS PATH '$'));
> SELECT a,a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4_1
> EXISTS PATH '$'));
> SELECT a,a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4_1
> EXISTS PATH '$.a'));
> SELECT a,a::bool FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a dint4_1
> EXISTS PATH '$.a' ERROR ON ERROR));
Domain-over-integer case should be fixed with the attached updated 0002.
> I found out 2 issues for the above tests.
> 1. RETURNING types is jsonb/domain over jsonb, default expression does
> not respect omit/keep quotes,
> but other RETURNING types do. Maybe this will be fine.
Yeah, I am not sure whether and how we could implement OMIT/KEEP
QUOTES for the DEFAULT expression. I might try later or simply
document that OMIT/KEEP QUOTE is only applied to the query result but
not the DEFAULT expression.
> 2. domain over jsonb should fail just like domain over other types?
> RETURNING djs keep quotes DEFAULT '"11"' ON empty
> should fail as
> ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type
> DETAIL: value for domain djs violates check constraint "djs_check""
I think this should be fixed with the attached patch 0004.
> errcode(ERRCODE_CANNOT_COERCE),
> errmsg("cannot cast behavior expression of
> type %s to %s",
>format_type_be(exprType(expr)),
>format_type_be(returning->typid)),
> errhint("You will need to cast the expression."),
> parser_errposition(pstate, exprLocation(expr)));
>
> maybe
> errhint("You will need to explicitly cast the expression to type %s",
> format_type_be(returning->typid))
OK, done.
Please check.
--
Thanks, Amit Langote
0002-SQL-JSON-Fix-casting-for-integer-EXISTS-columns-in-J.patch
Description: Binary data
0001-SQL-JSON-Some-fixes-to-JsonBehavior-expression-casti.patch
Description: Binary data
0005-SQL-JSON-Remove-useless-code-in-ExecInitJsonExpr.patch
Description: Binary data
0004-SQL-JSON-Respect-OMIT-QUOTES-when-RETURNING-domain_t.patch
Description: Binary data
0003-SQL-JSON-Fix-handling-of-errors-coercing-JsonBehavio.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Tue, Jul 23, 2024 at 11:45 AM jian he wrote: > On Mon, Jul 22, 2024 at 4:46 PM Amit Langote wrote: > > > > On Thu, Jul 18, 2024 at 3:04 PM jian he wrote: > > > we still have problem in transformJsonBehavior > > > > > > currently transformJsonBehavior: > > > SELECT JSON_VALUE(jsonb '1234', '$' RETURNING bit(3) DEFAULT 010111 ON > > > ERROR); > > > ERROR: cannot cast behavior expression of type text to bit > > > LINE 1: ...VALUE(jsonb '1234', '$' RETURNING bit(3) DEFAULT 010111 ON ... > > > > > > here, 010111 will default to int4, so "cannot cast behavior expression > > > of type text to bit" > > > is wrong? > > > also int4/int8 can be explicitly cast to bit(3), in this case, it > > > should return 111. > > > > I think we shouldn't try too hard in the code to "automatically" cast > > the DEFAULT expression, especially if that means having to add special > > case code for all sorts of source-target-type combinations. > > > > I'm inclined to just give a HINT to the user to cast the DEFAULT > > expression by hand, because they *can* do that with the syntax that > > exists. > > select typname, typinput, pg_get_function_identity_arguments(typinput) > from pg_type pt join pg_proc proc on proc.oid = pt.typinput > where typtype = 'b' and typarray <> 0 and proc.pronargs > 1; > > As you can see from the query result, we only need to deal with bit > and character type > in this context. > > SELECT JSON_VALUE(jsonb '1234', '$.a' RETURNING bit(3) DEFAULT 10111 ON > empty); > SELECT JSON_VALUE(jsonb '1234', '$.a' RETURNING char(3) DEFAULT 10111 > ON empty) ; > > the single quote literal ', no explicit cast, resolve to text type. > no single quote like 11, no explicit cast, resolve to int type. > we actually can cast int to bit, also have pg_cast entry. > so the above these 2 examples should behave the same, given there is > no pg_cast entry for int to text. > > select castsource::regtype ,casttarget::regtype ,castfunc,castcontext, > castmethod > from pg_cast where 'int'::regtype in (castsource::regtype > ,casttarget::regtype); > > but i won't insist on it, since bit/varbit don't use that much. The cast from int to bit that exists in pg_cast is only good for explicit casts, so would truncate user's value instead of flagging it as invalid input, and this whole discussion is about not doing that. With the DEFAULT expression specified or interpreted as a text string, we don't have that problem because we can then use CoerceViaIO as an assignment-level cast, whereby the invalid input *is* flagged as it should, like this: SELECT JSON_VALUE(jsonb '1234', '$' RETURNING bit(3) DEFAULT '1' ON ERROR); ERROR: bit string length 5 does not match type bit(3) So it seems fair to me to flag it when the user specifies an integer in DEFAULT we can't create a cast expression that does not truncate a value to fit the RETURNING type. > > I'm planning to push the attached 2 patches. 0001 is to fix > > transformJsonBehavior() for these cases and 0002 to adjust the > > behavior of casting the result of JSON_EXISTS() and EXISTS columns to > > integer type. I've included the tests in your patch in 0001. I > > noticed using cast expression to coerce the boolean constants to > > fixed-length types would produce unexpected errors when the planner's > > const-simplification calls the cast functions. So in 0001, I've made > > that case also use runtime coercion using json_populate_type(). > > > > + > + > +If an ON ERROR or ON EMPTY > +expression can't be coerced to the RETURNING type > +successfully, an SQL NULL value will be returned. > + > + > > I think this change will have some controversy. On second thought, I agree. I've made some changes to *throw* the error when the JsonBehavior values fail being coerced to the RETURNING type. Please check the attached. In the attached patch, I've also taken care of the problem mentioned in your latest email -- the solution I've chosen is not to produce the error when ERROR ON ERROR is specified but to use runtime coercion also for the jsonb type or any type that is not integer. Also fixed the typos. Thanks for your attention! -- Thanks, Amit Langote 0002-SQL-JSON-Fix-casting-for-integer-EXISTS-columns-in-J.patch Description: Binary data 0001-SQL-JSON-Some-fixes-to-JsonBehavior-expression-casti.patch Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Thu, Jul 18, 2024 at 3:04 PM jian he wrote: > we still have problem in transformJsonBehavior > > currently transformJsonBehavior: > SELECT JSON_VALUE(jsonb '1234', '$' RETURNING bit(3) DEFAULT 010111 ON > ERROR); > ERROR: cannot cast behavior expression of type text to bit > LINE 1: ...VALUE(jsonb '1234', '$' RETURNING bit(3) DEFAULT 010111 ON ... > > here, 010111 will default to int4, so "cannot cast behavior expression > of type text to bit" > is wrong? > also int4/int8 can be explicitly cast to bit(3), in this case, it > should return 111. I think we shouldn't try too hard in the code to "automatically" cast the DEFAULT expression, especially if that means having to add special case code for all sorts of source-target-type combinations. I'm inclined to just give a HINT to the user to cast the DEFAULT expression by hand, because they *can* do that with the syntax that exists. On the other hand, transformJsonBehavior() should handle other "internal" expressions for which the cast cannot be specified by hand. > Also, do we want to deal with bit data type's typmod like we did for > string type in transformJsonBehavior? > like: > SELECT JSON_VALUE(jsonb '"111"', '$' RETURNING bit(3) default '' on > error); > should return error: > ERROR: bit string length 2 does not match type bit(3) > or success > > The attached patch makes it return an error, similar to what we did > for the fixed length string type. Yeah, that makes sense. I'm planning to push the attached 2 patches. 0001 is to fix transformJsonBehavior() for these cases and 0002 to adjust the behavior of casting the result of JSON_EXISTS() and EXISTS columns to integer type. I've included the tests in your patch in 0001. I noticed using cast expression to coerce the boolean constants to fixed-length types would produce unexpected errors when the planner's const-simplification calls the cast functions. So in 0001, I've made that case also use runtime coercion using json_populate_type(). -- Thanks, Amit Langote 0002-SQL-JSON-Fix-casting-for-integer-EXISTS-columns-in-J.patch Description: Binary data 0001-SQL-JSON-Some-fixes-to-JsonBehavior-expression-casti.patch Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Tue, Jul 2, 2024 at 5:03 PM Amit Langote wrote:
> On Tue, Jul 2, 2024 at 3:19 PM jian he wrote:
> > On Mon, Jul 1, 2024 at 6:45 PM Amit Langote wrote:
> > >
> > > On Sun, Jun 30, 2024 at 3:56 AM Tom Lane wrote:
> > > > Alvaro Herrera writes:
> > > > >> +/*
> > > > >> + * For domains, consider the base type's typmod to decide
> > > > >> whether to setup
> > > > >> + * an implicit or explicit cast.
> > > > >> + */
> > > > >> +if (get_typtype(returning->typid) == TYPTYPE_DOMAIN)
> > > > >> +(void) getBaseTypeAndTypmod(returning->typid,
> > > > >> &baseTypmod);
> > > >
> > > > > TBH I'm not super clear on why we decide on explicit or implicit cast
> > > > > based on presence of a typmod. Why isn't it better to always use an
> > > > > implicit one?
> > > >
> > > > Hmm ... there are a bunch of existing places that seem to have similar
> > > > logic, but they are all in new-ish SQL/JSON functionality, and I would
> > > > not be surprised if they are all wrong. parse_coerce.c is quite
> > > > opinionated about what a domain's typtypmod means (see comments in
> > > > coerce_type() for instance); see also the logic in coerce_to_domain:
> > > >
> > > > * If the domain applies a typmod to its base type, build the
> > > > appropriate
> > > > * coercion step. Mark it implicit for display purposes, because
> > > > we don't
> > > > * want it shown separately by ruleutils.c; but the isExplicit flag
> > > > passed
> > > > * to the conversion function depends on the manner in which the
> > > > domain
> > > > * coercion is invoked, so that the semantics of implicit and
> > > > explicit
> > > > * coercion differ. (Is that really the behavior we want?)
> > > >
> > > > I don't think that this SQL/JSON behavior quite matches that.
> > >
> > > The reason I decided to go for the implicit cast only when there is a
> > > typmod is that the behavior with COERCION_EXPLICIT is only problematic
> > > when there's a typmod because of this code in
> > > build_coercion_expression:
> > >
> > > if (nargs == 3)
> > > {
> > > /* Pass it a boolean isExplicit parameter, too */
> > > cons = makeConst(BOOLOID,
> > > -1,
> > > InvalidOid,
> > > sizeof(bool),
> > > BoolGetDatum(ccontext == COERCION_EXPLICIT),
> > > false,
> > > true);
> > >
> > > args = lappend(args, cons);
> > > }
> > >
> > > Yeah, we could have fixed that by always using COERCION_IMPLICIT for
> > > SQL/JSON but, as Jian said, we don't have a bunch of casts that these
> > > SQL/JSON functions need, which is why I guess we ended up with
> > > COERCION_EXPLICIT here in the first place.
> > >
> > > One option I hadn't tried was using COERCION_ASSIGNMENT instead, which
> > > seems to give coerceJsonFuncExpr() the casts it needs with the
> > > behavior it wants, so how about applying the attached?
> >
> > you patched works.
> > i think it's because of you mentioned build_coercion_expression ` if
> > (nargs == 3)` related code
> > and
> >
> > find_coercion_pathway:
> > if (result == COERCION_PATH_NONE)
> > {
> > if (ccontext >= COERCION_ASSIGNMENT &&
> > TypeCategory(targetTypeId) == TYPCATEGORY_STRING)
> > result = COERCION_PATH_COERCEVIAIO;
> > else if (ccontext >= COERCION_EXPLICIT &&
> > TypeCategory(sourceTypeId) == TYPCATEGORY_STRING)
> > result = COERCION_PATH_COERCEVIAIO;
> > }
> >
> > functions: JSON_OBJECT,JSON_ARRAY, JSON_ARRAYAGG,JSON_OBJECTAGG,
> > JSON_SERIALIZE
> > the returning type can only be string type or json. json type already
> > being handled in other code.
> > so the targetTypeId category will be only TYPCATEGORY_STRING.
>
> Yes, thanks for confirming that.
>
> I checked other sites that use COERCION_ASSIGNMENT and I don't see a
> reason why it can't be used in this context.
>
> I'll push the patch tomorrow unless there are objections.
Sorry, I dropped the ball on this one.
I've pushed the patch now.
--
Thanks, Amit Langote
Re: in parentesis is not usual on DOCs
On Wed, May 22, 2024 at 8:22 PM jian he wrote: > On Wed, May 22, 2024 at 7:14 PM Peter Eisentraut wrote: > > > > On 20.05.24 02:00, jian he wrote: > > >> removing parentheses means we need to rephrase this sentence? > > >> So I come up with the following rephrase: > > >> > > >> The context_item specifies the input data to query, the > > >> path_expression is a JSON path expression defining the query, > > >> json_path_name is an optional name for the path_expression. The > > >> optional PASSING clause can provide data values to the > > >> path_expression. > > > > > > Based on this, write a simple patch. > > > > Your patch kind of messes up the indentation of the text you are > > changing. Please check that. > > please check attached. Sorry about not noticing this earlier. Thanks for the patch and the reviews. I've pushed it now after minor changes. -- Thanks, Amit Langote
Re: Address the -Wuse-after-free warning in ATExecAttachPartition()
On Tue, Jul 9, 2024 at 19:58 Junwang Zhao wrote: > On Tue, Jul 9, 2024 at 6:18 PM Amit Langote > wrote: > > > > Hi Junwang, > > > > On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao wrote: > > > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > > > wrote: > > > > > > > > In [1], Andres reported a -Wuse-after-free bug in the > > > > ATExecAttachPartition() function. I've created a patch to address it > > > > with pointers from Amit offlist. > > > > > > > > The issue was that the partBoundConstraint variable was utilized > after > > > > the list_concat() function. This could potentially lead to accessing > > > > the partBoundConstraint variable after its memory has been freed. > > > > > > > > The issue was resolved by using the return value of the list_concat() > > > > function, instead of using the list1 argument of list_concat(). I > > > > copied the partBoundConstraint variable to a new variable named > > > > partConstraint and used it for the previous references before > invoking > > > > get_proposed_default_constraint(). I confirmed that the > > > > eval_const_expressions(), make_ands_explicit(), > > > > map_partition_varattnos(), QueuePartitionConstraintValidation() > > > > functions do not modify the memory location pointed to by the > > > > partBoundConstraint variable. Therefore, it is safe to use it for the > > > > next reference in get_proposed_default_constraint() > > > > > > > > Attaching the patch. Please review and share the comments if any. > > > > Thanks to Andres for spotting the bug and some off-list advice on how > > > > to reproduce it. > > > > > > The patch LGTM. > > > > > > Curious how to reproduce the bug ;) > > > > Download and apply (`git am`) Andres' patch to "add allocator > > attributes" here (note it's not merged into the tree yet!): > > > https://github.com/anarazel/postgres/commit/99067d5c944e7bd29a8702689f470f623723f4e7.patch > > > > Then configure using meson with -Dc_args="-Wuse-after-free=3" > > --buildtype=release > > > > Compiling with gcc-12 or gcc-13 should give you the warning that looks > > as follows: > > > > [713/2349] Compiling C object > > src/backend/postgres_lib.a.p/commands_tablecmds.c.o > > ../src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’: > > ../src/backend/commands/tablecmds.c:18593:25: warning: pointer > > ‘partBoundConstraint’ may be used after ‘list_concat’ > > [-Wuse-after-free] > > 18593 | > > get_proposed_default_constraint(partBoundConstraint); > > | > > ^~~~ > > ../src/backend/commands/tablecmds.c:18546:26: note: call to > ‘list_concat’ here > > 18546 | partConstraint = list_concat(partBoundConstraint, > > | ^~~~ > > 18547 | > > RelationGetPartitionQual(rel)); > > | > > ~~ > > > > -- > > Thanks, Amit Langote > > Thanks Amit, > > Good to know. > > When Nitin says: > > ```Thanks to Andres for spotting the bug and some off-list advice on how > to reproduce it.``` > > I thought maybe there is some test case that can really trigger the > use-after-free bug, I might get it wrong though ;) I doubt one could write a regression test to demonstrate the use-after-free bug, though I may of course be wrong. By “reproduce it”, I think Nitin meant the warning that suggests that use-after-free bug may occur. >
Re: Address the -Wuse-after-free warning in ATExecAttachPartition()
Hi Junwang, On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao wrote: > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav > wrote: > > > > In [1], Andres reported a -Wuse-after-free bug in the > > ATExecAttachPartition() function. I've created a patch to address it > > with pointers from Amit offlist. > > > > The issue was that the partBoundConstraint variable was utilized after > > the list_concat() function. This could potentially lead to accessing > > the partBoundConstraint variable after its memory has been freed. > > > > The issue was resolved by using the return value of the list_concat() > > function, instead of using the list1 argument of list_concat(). I > > copied the partBoundConstraint variable to a new variable named > > partConstraint and used it for the previous references before invoking > > get_proposed_default_constraint(). I confirmed that the > > eval_const_expressions(), make_ands_explicit(), > > map_partition_varattnos(), QueuePartitionConstraintValidation() > > functions do not modify the memory location pointed to by the > > partBoundConstraint variable. Therefore, it is safe to use it for the > > next reference in get_proposed_default_constraint() > > > > Attaching the patch. Please review and share the comments if any. > > Thanks to Andres for spotting the bug and some off-list advice on how > > to reproduce it. > > The patch LGTM. > > Curious how to reproduce the bug ;) Download and apply (`git am`) Andres' patch to "add allocator attributes" here (note it's not merged into the tree yet!): https://github.com/anarazel/postgres/commit/99067d5c944e7bd29a8702689f470f623723f4e7.patch Then configure using meson with -Dc_args="-Wuse-after-free=3" --buildtype=release Compiling with gcc-12 or gcc-13 should give you the warning that looks as follows: [713/2349] Compiling C object src/backend/postgres_lib.a.p/commands_tablecmds.c.o ../src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’: ../src/backend/commands/tablecmds.c:18593:25: warning: pointer ‘partBoundConstraint’ may be used after ‘list_concat’ [-Wuse-after-free] 18593 | get_proposed_default_constraint(partBoundConstraint); | ^~~~ ../src/backend/commands/tablecmds.c:18546:26: note: call to ‘list_concat’ here 18546 | partConstraint = list_concat(partBoundConstraint, | ^~~~ 18547 | RelationGetPartitionQual(rel)); | ~~ -- Thanks, Amit Langote
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
On Tue, Jul 9, 2024 at 12:30 PM Amit Langote wrote: > On Tue, Jul 9, 2024 at 10:39 AM jian he wrote: > > On Mon, Jul 8, 2024 at 8:57 PM Amit Langote wrote: > > > > > > Updated patch attached. > > > > > > > Returns true if the SQL/JSON > > path_expression > > -applied to the context_item using the > > -PASSING values > > yields any > > -items. > > +applied to the context_item doesn't > > yield > > +any items. > > should "doesn't" be removed? > > should it be "yields"? > > Oops, fixed. > > > +set. The ON ERROR clause specifies the behavior > > +if an error occurs when evaluating > > path_expression, > > +when coercing the result value to the > > RETURNING type, > > +or when evaluating the ON EMPTY expression if > > the > > +path_expression evaluation results in an > > +empty set. > > last sentence, "in an empty set." should be "is an empty set" > > "results in an empty set" here means "the result of the evaluation is > an empty set", similar to: > > $ git grep "results in an" doc > doc/src/sgml/charset.sgml:results in an error, because even though > the || operator > doc/src/sgml/plpgsql.sgml: an omitted ELSE > clause results in an error rather > doc/src/sgml/plpython.sgml:If the second UPDATE > statement results in an > doc/src/sgml/pltcl.sgml: If the second UPDATE > statement results in an > > Maybe I could just replace that by "returns an empty set". > > Will push shortly after making those changes. And...pushed. Thanks for the reviews. -- Thanks, Amit Langote
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
On Tue, Jul 9, 2024 at 10:39 AM jian he wrote: > On Mon, Jul 8, 2024 at 8:57 PM Amit Langote wrote: > > > > Updated patch attached. > > > > Returns true if the SQL/JSON > path_expression > -applied to the context_item using the > -PASSING values yields > any > -items. > +applied to the context_item doesn't yield > +any items. > should "doesn't" be removed? > should it be "yields"? Oops, fixed. > +set. The ON ERROR clause specifies the behavior > +if an error occurs when evaluating > path_expression, > +when coercing the result value to the > RETURNING type, > +or when evaluating the ON EMPTY expression if the > +path_expression evaluation results in an > +empty set. > last sentence, "in an empty set." should be "is an empty set" "results in an empty set" here means "the result of the evaluation is an empty set", similar to: $ git grep "results in an" doc doc/src/sgml/charset.sgml:results in an error, because even though the || operator doc/src/sgml/plpgsql.sgml: an omitted ELSE clause results in an error rather doc/src/sgml/plpython.sgml:If the second UPDATE statement results in an doc/src/sgml/pltcl.sgml: If the second UPDATE statement results in an Maybe I could just replace that by "returns an empty set". Will push shortly after making those changes. -- Thanks, Amit Langote
Re: a potential typo in comments of pg_parse_json
On Mon, Jul 8, 2024 at 5:25 PM Junwang Zhao wrote: > Not 100% sure, sorry if this doesn't make sense. > > --- a/src/common/jsonapi.c > +++ b/src/common/jsonapi.c > @@ -514,7 +514,7 @@ freeJsonLexContext(JsonLexContext *lex) > * > * If FORCE_JSON_PSTACK is defined then the routine will call the > non-recursive > * JSON parser. This is a useful way to validate that it's doing the right > - * think at least for non-incremental cases. If this is on we expect to see > + * thing at least for non-incremental cases. If this is on we expect to see > * regression diffs relating to error messages about stack depth, but no > * other differences. > */ Good catch. Fixed. -- Thanks, Amit Langote
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
2]"}', 'lax $.a'
> RETURNING int[] OMIT QUOTES ERROR ON ERROR);
> +JSON_QUERY(jsonb '{"a": "[1, 2]"}', 'lax $.a'
> RETURNING int[] OMIT QUOTES ERROR ON ERROR);
>
> These two example queries don't need semicolons at the end?
Fixed.
Also, I've removed the following sentence in the description of
JSON_EXISTS, because a) it seems out of place
-
-Note that if the path_expression is
-strict and ON ERROR behavior is
-ERROR, an error is generated if it yields no items.
-
and b) does not seem correct:
SELECT JSON_EXISTS(jsonb '{"key1": [1,2,3]}', 'strict $.key1[*] ? (@ >
3)' ERROR ON ERROR);
json_exists
-
f
(1 row)
path_expression being strict or lax only matters inside
jsonpath_exec.c, not in ExecEvalJsonPathExpr().
Updated patch attached.
--
Thanks, Amit Langote
v4-0001-SQL-JSON-Various-improvements-to-SQL-JSON-query-f.patch
Description: Binary data
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
On Fri, Jul 5, 2024 at 10:16 PM Erik Rijkers wrote:
> Op 7/5/24 om 14:35 schreef Amit Langote:
> > Hi Jian,
> >
> > Thanks for the reviews.
> >
> > [v3-0001-SQL-JSON-Various-improvements-to-SQL-JSON-query-f.patch]
>i.e., from the patch for doc/src/sgml/func.sgml
>
>
> Small changes:
>
> 4x:
> 'a SQL' should be
> 'an SQL'
> ('a SQL' does never occur in the docs; it's always 'an SQL'; apperently
> the 'sequel' pronunciation is out)
>
> 'some other type to which can be successfully coerced'
> 'some other type to which it can be successfully coerced'
>
>
> 'specifies the behavior behavior'
> 'specifies the behavior'
>
>
> In the following sentence:
>
> "By default, the result is returned as a value of type jsonb,
> though the RETURNING clause can be used to return
> the original jsonb value as some other type to which it
> can be successfully coerced."
>
> it seems to me that this phrase is better removed:
> "the original jsonb value as"
Thanks, I've addressed all these in the next patch I'll send.
--
Thanks, Amit Langote
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
``
Ditto.
> still based on v2-0001, v2-0002.
> picture attached.
>
> as you can see from the curly braces,
> ```
> { KEEP | OMIT } QUOTES [ ON SCALAR STRING ]
> ```
> we must choose one, "KEEP" or "OMIT".
>
> but the wrapping_clause:
> ```
> WITHOUT [ARRAY] WRAPPER
> WITH [UNCONDITIONAL] [ARRAY] WRAPPER
> WITH CONDITIONAL [ARRAY] WRAPPER
> ```
> this way, didn't say we must choose between one in these three.
Ditto.
> ---
> on_error_boolean
> on_error_set
> on_error_value
> on_empty_set
> on_empty_value
>
> alternative ON { ERROR | EMPTY }
>
>
> didn't explain on_error_value, on_empty_value.
> why not just on_error_clause, on_empty_clause?
Ditto.
> ---
> << When JSON_QUERY function produces multiple JSON values, they are
> returned as a JSON array. By default, the result values are
> unconditionally wrapped even if the array contains only one element.
> You can specify the WITH CONDITIONAL variant to say that the wrapper
> be added only when there are multiple values in the resulting array.
> Or specify the WITHOUT variant to say that the wrapper be removed when
> there is only one element, but it is ignored if there are multiple
> values.
> <<
> The above paragraph didn't explicitly mention that UNCONDITIONAL is the
> default.
> BTW, by comparing patch with master, I found out:
>
> """
> If the wrapper is UNCONDITIONAL, an array wrapper will always be
> applied, even if the returned value is already a single JSON object or
> an array. If it is CONDITIONAL, it will not be applied to a single
> JSON object or an array. UNCONDITIONAL is the default.
> """
> this description seems not right.
> if "UNCONDITIONAL is the default", then
> select json_query(jsonb '{"a": [1]}', 'lax $.a' with unconditional
> array wrapper);
> should be same as
> select json_query(jsonb '{"a": [1]}', 'lax $.a' );
>
> another two examples with SQL/JSON scalar item:
>
> select json_query(jsonb '{"a": 1}', 'lax $.a' );
> select json_query(jsonb '{"a": 1}', 'lax $.a' with unconditional wrapper);
>
> Am I interpreting "UNCONDITIONAL is the default" the wrong way?
Current text is confusing, so I've rewritten the paragraph as:
+If the path expression may return multiple values, it might
be necessary
+to wrap those values using the WITH
WRAPPER clause to
+make it a valid JSON string, because the default behavior is
to not wrap
+them, as if WITHOUT WRAPPER were specified. The
+WITH WRAPPER clause is by default taken to mean
+WITH UNCONDITIONAL WRAPPER, which means that even a
+single result value will be wrapped. To apply the wrapper only when
+multiple values are present, specify WITH
CONDITIONAL WRAPPER.
+Note that an error will be thrown if multiple values are returned and
+WITHOUT WRAPPER is specified.
So, UNCONDITIONAL is the default as in WITH [UNCONDITIONAL] WRAPPER.
(The default when no wrapping clause is present is WITHOUT WRAPPER as
seen in your example).
Please check the attached. I've also added lists as I
remember you had proposed before to make the functions' descriptions a
bit more readable -- I'm persuaded. :-)
--
Thanks, Amit Langote
v3-0001-SQL-JSON-Various-improvements-to-SQL-JSON-query-f.patch
Description: Binary data
Re: sql/json miscellaneous issue
Hi,
On Tue, Jun 25, 2024 at 1:53 PM jian he wrote:
> On Tue, Jun 25, 2024 at 11:23 AM Amit Langote wrote:
> > > My thoughts for the above cases are:
> > > * json_value, json_query main description is the same:
> > > {{Returns the result of applying the SQL/JSON path_expression to the
> > > context_item using the PASSING values.}}
> > > same context_item, same path_expression, for the above cases, the
> > > result should be the same?
> > >
> > > * in json_value, description
> > > {{The extracted value must be a single SQL/JSON scalar item; an error
> > > is thrown if that's not the case. If you expect that extracted value
> > > might be an object or an array, use the json_query function instead.}}
> > > query: `SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb);`
> > > the returned jsonb 'null' (applying the path expression) is a single
> > > SQL/JSON scalar item.
> > > json_value return jsonb null should be fine
> > >
> > >
> > > However, other database implementations return SQL null,
> > > so I guess returning SQL null is fine)
> > > (based on the doc explanation, returning jsonb null more make sense, imho)
> >
> > If the difference in behavior is not clear from the docs, I guess that
> > means that we need to improve the docs. Would you like to give a shot
> > at writing the patch?
> >
>
> other databases did mention how json_value deals with json null. eg.
> [0] mysql description:
> When the data at the specified path consists of or resolves to a JSON
> null literal, the function returns SQL NULL.
> [1] oracle description:
> SQL/JSON function json_value applied to JSON value null returns SQL
> NULL, not the SQL string 'null'. This means, in particular, that you
> cannot use json_value to distinguish the JSON value null from the
> absence of a value; SQL NULL indicates both cases.
>
>
> imitate above, i come up with following:
> "The extracted value must be a single SQL/JSON scalar item; an error
> is thrown if that's not the case. ..."
> to
> "The extracted value must be a single SQL/JSON scalar item; an error
> is thrown if that's not the case.
> If the extracted value is a JSON null, an SQL NULL value will return.
> This means that you cannot use json_value to distinguish the JSON
> value null from evaluating path_expression yields no value at all; SQL
> NULL indicates both cases, to distinguish these two cases, use
> json_query instead.
> "
>
> I also changed from
> ON EMPTY is not specified is to return a null value.
> ON ERROR is not specified is to return a null value.
> to
> The default when ON EMPTY is not specified is to return an SQL NULL value.
> The default when ON ERROR is not specified is to return an SQL NULL value.
>
> [0]
> https://dev.mysql.com/doc/refman/8.4/en/json-search-functions.html#function_json-value
> [1]https://docs.oracle.com/en/database/oracle/oracle-database/19/adjsn/function-JSON_VALUE.html#GUID-622170D8-7BAD-4F5F-86BF-C328451FC3BE
Thanks, though the patch at [1], which is a much larger attempt to
rewrite SQL/JSON query function docs, takes care of mentioning this.
Could you please give that one a read?
--
Thanks, Amit Langote
[1]
https://www.postgresql.org/message-id/CA%2BHiwqH_vwkNqL3Y0tpnugEaR5-7vU43XSxAC06oZJ6U%3D3LVdw%40mail.gmail.com
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
On Fri, Jun 28, 2024 at 2:56 PM Amit Langote wrote: > On Tue, Jun 25, 2024 at 3:47 PM David G. Johnston > wrote: > > > > Hey! > > > > Lots of SQL/JSON threads going about. This one is less about technical > > correctness and more about usability of the documentation. Though in > > writing this I am finding some things that aren't quite clear. I'm going > > to come back with those on a follow-on post once I get a chance to make my > > second pass on this. But for the moment just opening it up to a content > > and structure review. > > > > Please focus on the text changes. It passes "check-docs" but I still need > > to work on layout and stuff in html (markup, some more links). > > > > Thanks! > > > > David J. > > > > p.s. v1 exists here (is just the idea of using basically variable names in > > the function signature and minimizing direct syntax in the table); > > > > https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com > > Thanks for writing the patch. I'll take a look at this next Monday. I've attached a delta (0002) against your patch, wherein I've kept most of the structuring changes you've proposed, but made changes such as: * use tags consistently * use language matching the rest of func.sgml, IMO * avoid repetition (eg. context_item described both above and below the table) * correcting some factual discrepancies (eg. json_value never returns json null) * avoid forward references * capitalize function names, SQL keywords in examples as requested in a previous review [1] Maybe we could still polish this some more. -- Thanks, Amit Langote [1] https://www.postgresql.org/message-id/CAA-aLv7Dfy9BMrhUZ1skcg%3DOdqysWKzObS7XiDXdotJNF0E44Q%40mail.gmail.com v2-0001-SQL-JSON-Improve-documentation-structure.patch Description: Binary data v2-0002-Delta-against-David-J-s-patch.patch Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Tue, Jul 2, 2024 at 3:19 PM jian he wrote:
> On Mon, Jul 1, 2024 at 6:45 PM Amit Langote wrote:
> >
> > On Sun, Jun 30, 2024 at 3:56 AM Tom Lane wrote:
> > > Alvaro Herrera writes:
> > > >> +/*
> > > >> + * For domains, consider the base type's typmod to decide whether
> > > >> to setup
> > > >> + * an implicit or explicit cast.
> > > >> + */
> > > >> +if (get_typtype(returning->typid) == TYPTYPE_DOMAIN)
> > > >> +(void) getBaseTypeAndTypmod(returning->typid,
> > > >> &baseTypmod);
> > >
> > > > TBH I'm not super clear on why we decide on explicit or implicit cast
> > > > based on presence of a typmod. Why isn't it better to always use an
> > > > implicit one?
> > >
> > > Hmm ... there are a bunch of existing places that seem to have similar
> > > logic, but they are all in new-ish SQL/JSON functionality, and I would
> > > not be surprised if they are all wrong. parse_coerce.c is quite
> > > opinionated about what a domain's typtypmod means (see comments in
> > > coerce_type() for instance); see also the logic in coerce_to_domain:
> > >
> > > * If the domain applies a typmod to its base type, build the
> > > appropriate
> > > * coercion step. Mark it implicit for display purposes, because we
> > > don't
> > > * want it shown separately by ruleutils.c; but the isExplicit flag
> > > passed
> > > * to the conversion function depends on the manner in which the
> > > domain
> > > * coercion is invoked, so that the semantics of implicit and explicit
> > > * coercion differ. (Is that really the behavior we want?)
> > >
> > > I don't think that this SQL/JSON behavior quite matches that.
> >
> > The reason I decided to go for the implicit cast only when there is a
> > typmod is that the behavior with COERCION_EXPLICIT is only problematic
> > when there's a typmod because of this code in
> > build_coercion_expression:
> >
> > if (nargs == 3)
> > {
> > /* Pass it a boolean isExplicit parameter, too */
> > cons = makeConst(BOOLOID,
> > -1,
> > InvalidOid,
> > sizeof(bool),
> > BoolGetDatum(ccontext == COERCION_EXPLICIT),
> > false,
> > true);
> >
> > args = lappend(args, cons);
> > }
> >
> > Yeah, we could have fixed that by always using COERCION_IMPLICIT for
> > SQL/JSON but, as Jian said, we don't have a bunch of casts that these
> > SQL/JSON functions need, which is why I guess we ended up with
> > COERCION_EXPLICIT here in the first place.
> >
> > One option I hadn't tried was using COERCION_ASSIGNMENT instead, which
> > seems to give coerceJsonFuncExpr() the casts it needs with the
> > behavior it wants, so how about applying the attached?
>
> you patched works.
> i think it's because of you mentioned build_coercion_expression ` if
> (nargs == 3)` related code
> and
>
> find_coercion_pathway:
> if (result == COERCION_PATH_NONE)
> {
> if (ccontext >= COERCION_ASSIGNMENT &&
> TypeCategory(targetTypeId) == TYPCATEGORY_STRING)
> result = COERCION_PATH_COERCEVIAIO;
> else if (ccontext >= COERCION_EXPLICIT &&
> TypeCategory(sourceTypeId) == TYPCATEGORY_STRING)
> result = COERCION_PATH_COERCEVIAIO;
> }
>
> functions: JSON_OBJECT,JSON_ARRAY, JSON_ARRAYAGG,JSON_OBJECTAGG,
> JSON_SERIALIZE
> the returning type can only be string type or json. json type already
> being handled in other code.
> so the targetTypeId category will be only TYPCATEGORY_STRING.
Yes, thanks for confirming that.
I checked other sites that use COERCION_ASSIGNMENT and I don't see a
reason why it can't be used in this context.
I'll push the patch tomorrow unless there are objections.
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
On Sun, Jun 30, 2024 at 3:56 AM Tom Lane wrote:
> Alvaro Herrera writes:
> >> +/*
> >> + * For domains, consider the base type's typmod to decide whether to
> >> setup
> >> + * an implicit or explicit cast.
> >> + */
> >> +if (get_typtype(returning->typid) == TYPTYPE_DOMAIN)
> >> +(void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);
>
> > TBH I'm not super clear on why we decide on explicit or implicit cast
> > based on presence of a typmod. Why isn't it better to always use an
> > implicit one?
>
> Hmm ... there are a bunch of existing places that seem to have similar
> logic, but they are all in new-ish SQL/JSON functionality, and I would
> not be surprised if they are all wrong. parse_coerce.c is quite
> opinionated about what a domain's typtypmod means (see comments in
> coerce_type() for instance); see also the logic in coerce_to_domain:
>
> * If the domain applies a typmod to its base type, build the appropriate
> * coercion step. Mark it implicit for display purposes, because we don't
> * want it shown separately by ruleutils.c; but the isExplicit flag passed
> * to the conversion function depends on the manner in which the domain
> * coercion is invoked, so that the semantics of implicit and explicit
> * coercion differ. (Is that really the behavior we want?)
>
> I don't think that this SQL/JSON behavior quite matches that.
The reason I decided to go for the implicit cast only when there is a
typmod is that the behavior with COERCION_EXPLICIT is only problematic
when there's a typmod because of this code in
build_coercion_expression:
if (nargs == 3)
{
/* Pass it a boolean isExplicit parameter, too */
cons = makeConst(BOOLOID,
-1,
InvalidOid,
sizeof(bool),
BoolGetDatum(ccontext == COERCION_EXPLICIT),
false,
true);
args = lappend(args, cons);
}
Yeah, we could have fixed that by always using COERCION_IMPLICIT for
SQL/JSON but, as Jian said, we don't have a bunch of casts that these
SQL/JSON functions need, which is why I guess we ended up with
COERCION_EXPLICIT here in the first place.
One option I hadn't tried was using COERCION_ASSIGNMENT instead, which
seems to give coerceJsonFuncExpr() the casts it needs with the
behavior it wants, so how about applying the attached?
--
Thanks, Amit Langote
v6-0001-SQL-JSON-Rethink-c2d93c3802b.patch
Description: Binary data
Re: remaining sql/json patches
Hi Alexander,
On Fri, Jun 28, 2024 at 5:00 PM Alexander Lakhin wrote:
>
> Hi Amit,
>
> 28.06.2024 09:15, Amit Langote wrote:
> > Hi Alexander,
> >
> >
> > Thanks for the report. Yeah, those comments that got added in
> > 7081ac46ace are obsolete.
> >
>
> Thanks for paying attention to that!
>
> Could you also look at comments for transformJsonObjectAgg() and
> transformJsonArrayAgg(), aren't they obsolete too?
You're right. I didn't think they needed to be similarly fixed,
because I noticed the code like the following in in
transformJsonObjectAgg() which sets the OID of the function to call
from, again, JsonConstructorExpr:
{
if (agg->absent_on_null)
if (agg->unique)
aggfnoid = F_JSONB_OBJECT_AGG_UNIQUE_STRICT;
else
aggfnoid = F_JSONB_OBJECT_AGG_STRICT;
else if (agg->unique)
aggfnoid = F_JSONB_OBJECT_AGG_UNIQUE;
else
aggfnoid = F_JSONB_OBJECT_AGG;
aggtype = JSONBOID;
}
So, yes, the comments for them should be fixed too like the other two
to also mention JsonConstructorExpr.
Updated patch attached.
Wonder if Alvaro has any thoughts on this.
--
Thanks, Amit Langote
v2-0001-SQL-JSON-Fix-some-obsolete-comments.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Fri, Jun 28, 2024 at 3:14 PM jian he wrote:
> On Thu, Jun 27, 2024 at 7:48 PM Amit Langote wrote:
> > >
> > > I've attempted that in the attached 0001, which removes
> > > JsonExpr.coercion_expr and a bunch of code around it.
> > >
> > > 0002 is now the original patch minus the changes to make
> > > JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
> > > because the changes in 0001 covers them. The changes for JsonBehavior
> > > expression coercion as they were in the last version of the patch are
> > > still needed, but I decided to move those into 0001 so that the
> > > changes for query functions are all in 0001 and those for constructors
> > > in 0002. It would be nice to get rid of that coerce_to_target_type()
> > > call to coerce the "behavior expression" to RETURNING type, but I'm
> > > leaving that as a task for another day.
> >
> > Updated 0001 to remove outdated references, remove some more unnecessary
> > code.
> >
>
> i found some remaining references of "coercion_expr" should be removed.
>
> src/include/nodes/primnodes.h
> /* JsonExpr's collation, if coercion_expr is NULL. */
>
>
> src/include/nodes/execnodes.h
> /*
> * Address of the step to coerce the result value of jsonpath evaluation
> * to the RETURNING type using JsonExpr.coercion_expr. -1 if no coercion
> * is necessary or if either JsonExpr.use_io_coercion or
> * JsonExpr.use_json_coercion is true.
> */
> int jump_eval_coercion;
>
> src/backend/jit/llvm/llvmjit_expr.c
> /* coercion_expr code */
> LLVMPositionBuilderAtEnd(b, b_coercion);
> if (jsestate->jump_eval_coercion >= 0)
> LLVMBuildBr(b, opblocks[jsestate->jump_eval_coercion]);
> else
> LLVMBuildUnreachable(b);
>
>
> src/backend/executor/execExprInterp.c
> /*
> * Checks if an error occurred either when evaluating JsonExpr.coercion_expr
> or
> * in ExecEvalJsonCoercion(). If so, this sets JsonExprState.error to trigger
> * the ON ERROR handling steps.
> */
> void
> ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op)
> {
> }
>
> if (jbv == NULL)
> {
> /* Will be coerced with coercion_expr, if any. */
> *op->resvalue = (Datum) 0;
> *op->resnull = true;
> }
>
>
> src/backend/executor/execExpr.c
> /*
> * Jump to coerce the NULL using coercion_expr if present. Coercing NULL
> * is only interesting when the RETURNING type is a domain whose
> * constraints must be checked. jsexpr->coercion_expr containing a
> * CoerceToDomain node must have been set in that case.
> */
>
> /*
> * Jump to coerce the NULL using coercion_expr if present. Coercing NULL
> * is only interesting when the RETURNING type is a domain whose
> * constraints must be checked. jsexpr->coercion_expr containing a
> * CoerceToDomain node must have been set in that case.
> */
Thanks for checking.
Will push the attached 0001 and 0002 shortly.
--
Thanks, Amit Langote
v5-0001-SQL-JSON-Fix-coercion-of-constructor-outputs-to-t.patch
Description: Binary data
v5-0002-SQL-JSON-Always-coerce-JsonExpr-result-at-runtime.patch
Description: Binary data
Re: remaining sql/json patches
Hi Alexander, On Wed, Jun 26, 2024 at 8:00 PM Alexander Lakhin wrote: > > Hello, > > I'm not sure I've chosen the most appropriate thread for reporting the > issue, but maybe you would like to look at code comments related to > SQL/JSON constructors: > > * Transform JSON_ARRAY() constructor. > * > * JSON_ARRAY() is transformed into json[b]_build_array[_ext]() call > * depending on the output JSON format. The first argument of > * json[b]_build_array_ext() is absent_on_null. > > > * Transform JSON_OBJECT() constructor. > * > * JSON_OBJECT() is transformed into json[b]_build_object[_ext]() call > * depending on the output JSON format. The first two arguments of > * json[b]_build_object_ext() are absent_on_null and check_unique. > > But the referenced functions were removed at [1]; Nikita Glukhov wrote: > > I have removed json[b]_build_object_ext() and json[b]_build_array_ext(). > > (That thread seems too old for the current discussion.) > > Also, a comment above transformJsonObjectAgg() references > json[b]_objectagg[_unique][_strict](key, value), but I could find > json_objectagg() only. > > [1] > https://www.postgresql.org/message-id/be40362b-7821-7422-d33f-fbf1c61bb3e3%40postgrespro.ru Thanks for the report. Yeah, those comments that got added in 7081ac46ace are obsolete. Attached is a patch to fix that. Should be back-patched to v16. -- Thanks, Amit Langote v1-0001-SQL-JSON-Fix-some-obsolete-comments.patch Description: Binary data
Re: Doc Rework: Section 9.16.13 SQL/JSON Query Functions
Hi David, On Tue, Jun 25, 2024 at 3:47 PM David G. Johnston wrote: > > Hey! > > Lots of SQL/JSON threads going about. This one is less about technical > correctness and more about usability of the documentation. Though in writing > this I am finding some things that aren't quite clear. I'm going to come > back with those on a follow-on post once I get a chance to make my second > pass on this. But for the moment just opening it up to a content and > structure review. > > Please focus on the text changes. It passes "check-docs" but I still need to > work on layout and stuff in html (markup, some more links). > > Thanks! > > David J. > > p.s. v1 exists here (is just the idea of using basically variable names in > the function signature and minimizing direct syntax in the table); > > https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com Thanks for writing the patch. I'll take a look at this next Monday. -- Thanks, Amit Langote
Re: SQL/JSON query functions context_item doc entry and type requirement
On Thu, Jun 27, 2024 at 9:01 PM Amit Langote wrote:
> On Sat, Jun 22, 2024 at 6:39 PM jian he wrote:
> > On Fri, Jun 21, 2024 at 8:18 PM Amit Langote
> > wrote:
> > >
> > > >> JSON_VALUE on error, on empty semantics should be the same as
> > > >> json_query.
> > > >> like:
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON EMPTY ]
> > > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > > >> ON ERROR ])
> > > >>
> > > >> examples:
> > > >> select JSON_value(jsonb '[]' , '$' empty array on error);
> > > >> select JSON_value(jsonb '[]' , '$' empty object on error);
> > > >
> > > > Again the documented behavior seems to make sense though and the
> > > > ability to specify empty in the value function seems like a bug. If
> > > > you really want an empty array or object you do have access to default.
> > > > The reason json_query provides for an empty array/object is that it is
> > > > already expecting to produce an array (object seems a little odd).
> > > >
> > > > I agree our docs and code do not match which needs to be fixed, ideally
> > > > in the direction of the standard which I'm guessing our documentation
> > > > is based off of. But let's not go off of my guess.
> > >
> > > Oops, that is indeed not great and, yes, the problem is code not
> > > matching the documentation, the latter of which is "correct".
> > >
> > > Basically, the grammar allows specifying any of the all possible ON
> > > ERROR/EMPTY behavior values irrespective of the function, so parse
> > > analysis should be catching and flagging an attempt to specify
> > > incompatible value for a given function, which it does not.
> > >
> > > I've attached a patch to fix that, which I'd like to push before
> > > anything else we've been discussing.
> > >
> >
> > + errcode(ERRCODE_SYNTAX_ERROR),
> > + errmsg("invalid ON ERROR behavior"),
> > + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
> > is allowed in ON ERROR for JSON_QUERY()."),
> > + parser_errposition(pstate, func->on_error->location));
> >
> > `EMPTY [ ARRAY | OBJECT }` seems not correct,
> > maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> > (apply to other places)
>
> Or EMPTY [ ARRAY ], EMPTY OBJECT
>
> > `DEFAULT `
> > `DEFAULT ` or just `DEFAULT expression` would be more correct?
> > (apply to other places)
>
> "DEFAULT expression" sounds good.
>
> > I think we should make json_query, json_value on empty, on error
> > behave the same way.
> > otherwise, it will have consistency issues for scalar jsonb.
> > for example, we should expect the following two queries to return the
> > same result?
> > SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> > SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
> >
> > Also the json_table function will call json_value or json_query,
> > make these two functions on error, on empty behavior the same can
> > reduce unintended complexity.
> >
> > So based on your
> > patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> > and the above points, I have made some changes, attached.
> > it will make json_value, json_query not allow {true | false | unknown
> > } on error, {true | false | unknown } on empty.
> > json_table error message deal the same way as
> > b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af
>
> Here is an updated patch that I think takes care of these points.
>
> > BTW,
> > i found one JSON_TABLE document deficiency
> > [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> > [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> > expression } ON ERROR ]
> >
> > it should be
> >
> > [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON EMPTY ]
> > [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> > expression } ON ERROR ]
>
> You're right. Fixed.
>
> Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
> columns which is meaningless because JSON_EXISTS() doesn't have a
> corresponding ON EMPTY clause. Fixed grammar to prevent that in the
> attached 0002.
I've pushed this for now to close out the open item.
I know there's some documentation improvement work left to do [1],
which I'll try to find some time for next week.
--
Thanks, Amit Langote
[1]
https://www.postgresql.org/message-id/CAKFQuwbYBvUZasGj_ZnfXhC2kk4AT%3DepwGkNd2%3DRMMVXkfTNMQ%40mail.gmail.com
Re: ON ERROR in json_query and the like
On Wed, Jun 26, 2024 at 9:10 PM Amit Langote wrote: > On Sat, Jun 22, 2024 at 5:43 PM Amit Langote wrote: > > On Fri, Jun 21, 2024 at 8:00 PM Markus Winand > > wrote: > > > So updating the three options: > > > > 1. Disallow anything but jsonb for context_item (the patch I posted > > > > yesterday) > > > > > > * Non-conforming > > > * patch available > > > > > > > 2. Continue allowing context_item to be non-json character or utf-8 > > > > encoded bytea strings, but document that any parsing errors do not > > > > respect the ON ERROR clause. > > > > > > * Conforming by choosing IA050 to implement GR4: raise errors independent > > > of the ON ERROR clause. > > > * currently committed. > > > > > > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors > > > > respect ON ERROR (no patch written yet). > > > > > > * Conforming by choosing IA050 not to implement GR4: Parsing happens > > > later, considering the ON ERROR clause. > > > * no patch available, not trivial > > > > > > I guess I’m the only one in favour of 3 ;) My remaining arguments are > > > that Oracle and Db2 (LUW) do it that way and also that it is IMHO what > > > users would expect. However, as 2 is also conforming (how could I miss > > > that?), proper documentation is a very tempting option. > > > > So, we should go with 2 for v17, because while 3 may be very > > appealing, there's a risk that it might not get done in the time > > remaining for v17. > > > > I'll post the documentation patch on Monday. > > Here's that patch, which adds this note after table 9.16.3. SQL/JSON > Query Functions: > > + > + > +The context_item expression is converted to > +jsonb by an implicit cast if the expression is not already > of > +type jsonb. Note, however, that any parsing errors that > occur > +during that conversion are thrown unconditionally, that is, are not > +handled according to the (specified or implicit) ON > ERROR > +clause. > + > + I have pushed this. -- Thanks, Amit Langote
Re: SQL/JSON query functions context_item doc entry and type requirement
Hi,
On Sat, Jun 22, 2024 at 6:39 PM jian he wrote:
> On Fri, Jun 21, 2024 at 8:18 PM Amit Langote wrote:
> >
> > >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> > >> like:
> > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > >> ON EMPTY ]
> > >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> > >> ON ERROR ])
> > >>
> > >> examples:
> > >> select JSON_value(jsonb '[]' , '$' empty array on error);
> > >> select JSON_value(jsonb '[]' , '$' empty object on error);
> > >
> > > Again the documented behavior seems to make sense though and the ability
> > > to specify empty in the value function seems like a bug. If you really
> > > want an empty array or object you do have access to default. The reason
> > > json_query provides for an empty array/object is that it is already
> > > expecting to produce an array (object seems a little odd).
> > >
> > > I agree our docs and code do not match which needs to be fixed, ideally
> > > in the direction of the standard which I'm guessing our documentation is
> > > based off of. But let's not go off of my guess.
> >
> > Oops, that is indeed not great and, yes, the problem is code not
> > matching the documentation, the latter of which is "correct".
> >
> > Basically, the grammar allows specifying any of the all possible ON
> > ERROR/EMPTY behavior values irrespective of the function, so parse
> > analysis should be catching and flagging an attempt to specify
> > incompatible value for a given function, which it does not.
> >
> > I've attached a patch to fix that, which I'd like to push before
> > anything else we've been discussing.
> >
>
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("invalid ON ERROR behavior"),
> + errdetail("Only ERROR, NULL, EMPTY [ ARRAY | OBJECT }, or DEFAULT
> is allowed in ON ERROR for JSON_QUERY()."),
> + parser_errposition(pstate, func->on_error->location));
>
> `EMPTY [ ARRAY | OBJECT }` seems not correct,
> maybe just EMPTY, EMPTY ARRAY, EMPTY OBJECT.
> (apply to other places)
Or EMPTY [ ARRAY ], EMPTY OBJECT
> `DEFAULT `
> `DEFAULT ` or just `DEFAULT expression` would be more correct?
> (apply to other places)
"DEFAULT expression" sounds good.
> I think we should make json_query, json_value on empty, on error
> behave the same way.
> otherwise, it will have consistency issues for scalar jsonb.
> for example, we should expect the following two queries to return the
> same result?
> SELECT * FROM JSON_query(jsonb '1', '$.a' returning jsonb empty on empty);
> SELECT * FROM JSON_value(jsonb '1', '$.a' returning jsonb empty on empty);
>
> Also the json_table function will call json_value or json_query,
> make these two functions on error, on empty behavior the same can
> reduce unintended complexity.
>
> So based on your
> patch(v1-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch)
> and the above points, I have made some changes, attached.
> it will make json_value, json_query not allow {true | false | unknown
> } on error, {true | false | unknown } on empty.
> json_table error message deal the same way as
> b4fad46b6bc8a9bf46ff689bcb1bd4edf8f267af
Here is an updated patch that I think takes care of these points.
> BTW,
> i found one JSON_TABLE document deficiency
> [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> expression } ON EMPTY ]
> [ { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT
> expression } ON ERROR ]
>
> it should be
>
> [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> expression } ON EMPTY ]
> [ { ERROR | NULL | EMPTY { [ARRAY] | OBJECT } | DEFAULT
> expression } ON ERROR ]
You're right. Fixed.
Also, I noticed that the grammar allows ON EMPTY in JSON_TABLE EXISTS
columns which is meaningless because JSON_EXISTS() doesn't have a
corresponding ON EMPTY clause. Fixed grammar to prevent that in the
attached 0002.
--
Thanks, Amit Langote
v2-0002-SQL-JSON-Prevent-ON-EMPTY-for-EXISTS-columns-in-J.patch
Description: Binary data
v2-0001-SQL-JSON-Disallow-incompatible-values-in-ON-ERROR.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Thu, Jun 27, 2024 at 6:57 PM Amit Langote wrote:
> On Wed, Jun 26, 2024 at 11:46 PM jian he wrote:
> > On Wed, Jun 26, 2024 at 8:39 PM Amit Langote
> > wrote:
> > >
> > > >
> > > > The RETURNING variant giving an error is what the standard asks us to
> > > > do apparently. I read Tom's last message on this thread as agreeing
> > > > to that, even though hesitantly. He can correct me if I got that
> > > > wrong.
> > > >
> > > > > your patch will make domain and char(n) behavior inconsistent.
> > > > > create domain char2 as char(2);
> > > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON
> > > > > ERROR);
> > > > >
> > > > >
> > > > > another example:
> > > > > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > > > > default '"aaa"'::jsonb ON ERROR);
> > > > > same value (jsonb "aaa") error on error will yield error,
> > > > > but `default expression on error` can coerce the value to char(2),
> > > > > which looks a little bit inconsistent, I think.
> > > >
> > > > Interesting examples, thanks for sharing.
> > > >
> > > > Attached updated version should take into account that typmod may be
> > > > hiding under domains. Please test.
> > >
> >
> > SELECT JSON_VALUE(jsonb '111', '$' RETURNING queryfuncs_char2 default
> > '13' on error);
> > return
> > ERROR: value too long for type character(2)
> > should return 13
> >
> > I found out the source of the problem is in coerceJsonExprOutput
> > /*
> > * Use cast expression for domain types; we need CoerceToDomain here.
> > */
> > if (get_typtype(returning->typid) != TYPTYPE_DOMAIN)
> > {
> > jsexpr->use_io_coercion = true;
> > return;
> > }
>
> Thanks for this test case and the analysis. Yes, using a cast
> expression for coercion to the RETURNING type generally seems to be a
> source of many problems that could've been solved by fixing things so
> that only use_io_coercion and use_json_coercion are enough to handle
> all the cases.
>
> I've attempted that in the attached 0001, which removes
> JsonExpr.coercion_expr and a bunch of code around it.
>
> 0002 is now the original patch minus the changes to make
> JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
> because the changes in 0001 covers them. The changes for JsonBehavior
> expression coercion as they were in the last version of the patch are
> still needed, but I decided to move those into 0001 so that the
> changes for query functions are all in 0001 and those for constructors
> in 0002. It would be nice to get rid of that coerce_to_target_type()
> call to coerce the "behavior expression" to RETURNING type, but I'm
> leaving that as a task for another day.
Updated 0001 to remove outdated references, remove some more unnecessary code.
--
Thanks, Amit Langote
v4-0002-SQL-JSON-Use-implicit-casts-for-RETURNING-type-wi.patch
Description: Binary data
v4-0001-SQL-JSON-Always-coerce-JsonExpr-result-at-runtime.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
Hi,
On Wed, Jun 26, 2024 at 11:46 PM jian he wrote:
> On Wed, Jun 26, 2024 at 8:39 PM Amit Langote wrote:
> >
> > >
> > > The RETURNING variant giving an error is what the standard asks us to
> > > do apparently. I read Tom's last message on this thread as agreeing
> > > to that, even though hesitantly. He can correct me if I got that
> > > wrong.
> > >
> > > > your patch will make domain and char(n) behavior inconsistent.
> > > > create domain char2 as char(2);
> > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > > > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);
> > > >
> > > >
> > > > another example:
> > > > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > > > default '"aaa"'::jsonb ON ERROR);
> > > > same value (jsonb "aaa") error on error will yield error,
> > > > but `default expression on error` can coerce the value to char(2),
> > > > which looks a little bit inconsistent, I think.
> > >
> > > Interesting examples, thanks for sharing.
> > >
> > > Attached updated version should take into account that typmod may be
> > > hiding under domains. Please test.
> >
>
> SELECT JSON_VALUE(jsonb '111', '$' RETURNING queryfuncs_char2 default
> '13' on error);
> return
> ERROR: value too long for type character(2)
> should return 13
>
> I found out the source of the problem is in coerceJsonExprOutput
> /*
> * Use cast expression for domain types; we need CoerceToDomain here.
> */
> if (get_typtype(returning->typid) != TYPTYPE_DOMAIN)
> {
> jsexpr->use_io_coercion = true;
> return;
> }
Thanks for this test case and the analysis. Yes, using a cast
expression for coercion to the RETURNING type generally seems to be a
source of many problems that could've been solved by fixing things so
that only use_io_coercion and use_json_coercion are enough to handle
all the cases.
I've attempted that in the attached 0001, which removes
JsonExpr.coercion_expr and a bunch of code around it.
0002 is now the original patch minus the changes to make
JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
because the changes in 0001 covers them. The changes for JsonBehavior
expression coercion as they were in the last version of the patch are
still needed, but I decided to move those into 0001 so that the
changes for query functions are all in 0001 and those for constructors
in 0002. It would be nice to get rid of that coerce_to_target_type()
call to coerce the "behavior expression" to RETURNING type, but I'm
leaving that as a task for another day.
--
Thanks, Amit Langote
v3-0001-SQL-JSON-Always-coerce-JsonExpr-result-at-runtime.patch
Description: Binary data
v3-0002-SQL-JSON-Use-implicit-casts-for-RETURNING-type-wi.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Fri, Jun 21, 2024 at 10:48 PM Amit Langote wrote:
> On Fri, Jun 21, 2024 at 4:05 PM jian he wrote:
> > hi.
> > i am a little confused.
> >
> > here[1] tom says:
> > > Yeah, I too think this is a cast, and truncation is the spec-defined
> > > behavior for casting to varchar with a specific length limit. I see
> > > little reason that this should work differently from
> > >
> > > select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
> > > json_serialize
> > >
> > > {"a":
> > > (1 row)
> >
> > if i understand it correctly, and my english interpretation is fine.
> > i think tom means something like:
> >
> > select json_serialize('{"a":1, "a":2}' returning text)::varchar(5) =
> > json_serialize('{"a":1, "a":2}' returning varchar(5));
> >
> > should return true.
> > the master will return true, but apply your patch, the above query
> > will yield an error.
>
> The RETURNING variant giving an error is what the standard asks us to
> do apparently. I read Tom's last message on this thread as agreeing
> to that, even though hesitantly. He can correct me if I got that
> wrong.
>
> > your patch will make domain and char(n) behavior inconsistent.
> > create domain char2 as char(2);
> > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> > SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);
> >
> >
> > another example:
> > SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> > default '"aaa"'::jsonb ON ERROR);
> > same value (jsonb "aaa") error on error will yield error,
> > but `default expression on error` can coerce the value to char(2),
> > which looks a little bit inconsistent, I think.
>
> Interesting examples, thanks for sharing.
>
> Attached updated version should take into account that typmod may be
> hiding under domains. Please test.
I'd like to push this one tomorrow, barring objections.
I could use some advice on backpatching. As I mentioned upthread,
this changes the behavior for JSON_OBJECT(), JSON_ARRAY(),
JSON_ARRAYAGG(), JSON_OBJECTAGG() too, which were added in v16.
Should this change be backpatched? In general, what's our stance on
changes that cater to improving standard compliance, but are not
necessarily bugs.
--
Thanks, Amit Langote
Re: ON ERROR in json_query and the like
Hi, On Sat, Jun 22, 2024 at 5:43 PM Amit Langote wrote: > On Fri, Jun 21, 2024 at 8:00 PM Markus Winand wrote: > > So updating the three options: > > > 1. Disallow anything but jsonb for context_item (the patch I posted > > > yesterday) > > > > * Non-conforming > > * patch available > > > > > 2. Continue allowing context_item to be non-json character or utf-8 > > > encoded bytea strings, but document that any parsing errors do not > > > respect the ON ERROR clause. > > > > * Conforming by choosing IA050 to implement GR4: raise errors independent > > of the ON ERROR clause. > > * currently committed. > > > > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors > > > respect ON ERROR (no patch written yet). > > > > * Conforming by choosing IA050 not to implement GR4: Parsing happens later, > > considering the ON ERROR clause. > > * no patch available, not trivial > > > > I guess I’m the only one in favour of 3 ;) My remaining arguments are that > > Oracle and Db2 (LUW) do it that way and also that it is IMHO what users > > would expect. However, as 2 is also conforming (how could I miss that?), > > proper documentation is a very tempting option. > > So, we should go with 2 for v17, because while 3 may be very > appealing, there's a risk that it might not get done in the time > remaining for v17. > > I'll post the documentation patch on Monday. Here's that patch, which adds this note after table 9.16.3. SQL/JSON Query Functions: + + +The context_item expression is converted to +jsonb by an implicit cast if the expression is not already of +type jsonb. Note, however, that any parsing errors that occur +during that conversion are thrown unconditionally, that is, are not +handled according to the (specified or implicit) ON ERROR +clause. + + Peter, sorry about the last-minute ask, but do you have any thoughts/advice on conformance as discussed above? -- Thanks, Amit Langote v1-0001-SQL-JSON-Document-behavior-when-context_item-is-n.patch Description: Binary data
Re: sql/json miscellaneous issue
On Tue, Jun 25, 2024 at 12:18 PM jian he wrote:
> On Mon, Jun 24, 2024 at 7:46 PM Amit Langote wrote:
> > On Mon, Jun 24, 2024 at 7:04 PM jian he wrote:
> > >
> > > hi.
> > > the following two queries should return the same result?
> > >
> > > SELECT * FROM JSON_query (jsonb 'null', '$' returning jsonb);
> > > SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb);
> >
> > I get this with HEAD:
> >
> > SELECT * FROM JSON_query (jsonb 'null', '$' returning jsonb);
> > json_query
> >
> > null
> > (1 row)
> >
> > Time: 734.587 ms
> > SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb);
> > json_value
> >
> >
> > (1 row)
> >
> > Much like:
> >
> > SELECT JSON_QUERY('{"key": null}', '$.key');
> > json_query
> >
> > null
> > (1 row)
> >
> > Time: 2.975 ms
> > SELECT JSON_VALUE('{"key": null}', '$.key');
> > json_value
> >
> >
> > (1 row)
> >
> > Which makes sense to me, because JSON_QUERY() is supposed to return a
> > JSON null in both cases and JSON_VALUE() is supposed to return a SQL
> > NULL for a JSON null.
> >
> > --
> > Thanks, Amit Langote
>
> hi amit, sorry to bother you again.
No worries.
> My thoughts for the above cases are:
> * json_value, json_query main description is the same:
> {{Returns the result of applying the SQL/JSON path_expression to the
> context_item using the PASSING values.}}
> same context_item, same path_expression, for the above cases, the
> result should be the same?
>
> * in json_value, description
> {{The extracted value must be a single SQL/JSON scalar item; an error
> is thrown if that's not the case. If you expect that extracted value
> might be an object or an array, use the json_query function instead.}}
> query: `SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb);`
> the returned jsonb 'null' (applying the path expression) is a single
> SQL/JSON scalar item.
> json_value return jsonb null should be fine
>
>
> However, other database implementations return SQL null,
> so I guess returning SQL null is fine)
> (based on the doc explanation, returning jsonb null more make sense, imho)
If the difference in behavior is not clear from the docs, I guess that
means that we need to improve the docs. Would you like to give a shot
at writing the patch?
--
Thanks, Amit Langote
Re: sql/json miscellaneous issue
Hi,
On Mon, Jun 24, 2024 at 7:04 PM jian he wrote:
>
> hi.
> the following two queries should return the same result?
>
> SELECT * FROM JSON_query (jsonb 'null', '$' returning jsonb);
> SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb);
I get this with HEAD:
SELECT * FROM JSON_query (jsonb 'null', '$' returning jsonb);
json_query
null
(1 row)
Time: 734.587 ms
SELECT * FROM JSON_value (jsonb 'null', '$' returning jsonb);
json_value
(1 row)
Much like:
SELECT JSON_QUERY('{"key": null}', '$.key');
json_query
null
(1 row)
Time: 2.975 ms
SELECT JSON_VALUE('{"key": null}', '$.key');
json_value
(1 row)
Which makes sense to me, because JSON_QUERY() is supposed to return a
JSON null in both cases and JSON_VALUE() is supposed to return a SQL
NULL for a JSON null.
--
Thanks, Amit Langote
Re: sql/json miscellaneous issue
Hi, On Mon, Jun 24, 2024 at 8:02 PM Stepan Neretin wrote: > Hi! > > I also noticed a very strange difference in behavior in these two queries, it > seems to me that although it returns a string by default, for the boolean > operator it is necessary to return true or false > SELECT * FROM JSON_value (jsonb '1', '$ == "1"' returning jsonb); > json_value > > > (1 row) > > SELECT * FROM JSON_value (jsonb 'null', '$ == "1"' returning jsonb); > json_value > > false > (1 row) Hmm, that looks sane to me when comparing the above two queries with their jsonb_path_query() equivalents: select jsonb_path_query(jsonb '1', '$ == "1"'); jsonb_path_query -- null (1 row) select jsonb_path_query(jsonb 'null', '$ == "1"'); jsonb_path_query -- false (1 row) -- Thanks, Amit Langote
Re: ON ERROR in json_query and the like
Hi, Thanks all for chiming in. On Fri, Jun 21, 2024 at 8:00 PM Markus Winand wrote: > So updating the three options: > > 1. Disallow anything but jsonb for context_item (the patch I posted > > yesterday) > > * Non-conforming > * patch available > > > 2. Continue allowing context_item to be non-json character or utf-8 > > encoded bytea strings, but document that any parsing errors do not > > respect the ON ERROR clause. > > * Conforming by choosing IA050 to implement GR4: raise errors independent of > the ON ERROR clause. > * currently committed. > > > 3. Go ahead and fix implicit casts to jsonb so that any parsing errors > > respect ON ERROR (no patch written yet). > > * Conforming by choosing IA050 not to implement GR4: Parsing happens later, > considering the ON ERROR clause. > * no patch available, not trivial > > I guess I’m the only one in favour of 3 ;) My remaining arguments are that > Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would > expect. However, as 2 is also conforming (how could I miss that?), proper > documentation is a very tempting option. So, we should go with 2 for v17, because while 3 may be very appealing, there's a risk that it might not get done in the time remaining for v17. I'll post the documentation patch on Monday. -- Thanks, Amit Langote
Re: SQL/JSON query functions context_item doc entry and type requirement
On Fri, Jun 21, 2024 at 9:18 PM Amit Langote wrote:
> On Fri, Jun 21, 2024 at 9:47 AM David G. Johnston
> wrote:
> > On Thu, Jun 20, 2024 at 9:01 AM jian he wrote:
> >> playing around with it.
> >> found some minor issues:
> >>
> >> json_exists allow: DEFAULT expression ON ERROR, which is not
> >> mentioned in the doc.
> >> for example:
> >> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default
> >> true ON ERROR);
> >> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 0 ON
> >> ERROR);
> >> select JSON_EXISTS(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' default 11 ON
> >> ERROR);
> >
> >
> > Yeah, surprised it works, the documented behavior seems logical. Being
> > able to return a non-boolean here seems odd. Especially since it is cast
> > to boolean on output.
> >
> >>
> >> JSON_VALUE on error, on empty semantics should be the same as json_query.
> >> like:
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON EMPTY ]
> >> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> >> ON ERROR ])
> >>
> >> examples:
> >> select JSON_value(jsonb '[]' , '$' empty array on error);
> >> select JSON_value(jsonb '[]' , '$' empty object on error);
> >
> >
> > Again the documented behavior seems to make sense though and the ability to
> > specify empty in the value function seems like a bug. If you really want
> > an empty array or object you do have access to default. The reason
> > json_query provides for an empty array/object is that it is already
> > expecting to produce an array (object seems a little odd).
> >
> > I agree our docs and code do not match which needs to be fixed, ideally in
> > the direction of the standard which I'm guessing our documentation is based
> > off of. But let's not go off of my guess.
>
> Oops, that is indeed not great and, yes, the problem is code not
> matching the documentation, the latter of which is "correct".
>
> Basically, the grammar allows specifying any of the all possible ON
> ERROR/EMPTY behavior values irrespective of the function, so parse
> analysis should be catching and flagging an attempt to specify
> incompatible value for a given function, which it does not.
>
> I've attached a patch to fix that, which I'd like to push before
> anything else we've been discussing.
While there are still a few hours to go before Saturday noon UTC when
beta2 freeze goes into effect, I'm thinking to just push this after
beta2 is stamped. Adding an open item for now.
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
Hi,
Thanks for taking a look.
On Fri, Jun 21, 2024 at 4:05 PM jian he wrote:
> On Tue, Jun 18, 2024 at 5:02 PM Amit Langote wrote:
> > On Tue, Jun 4, 2024 at 7:03 PM Amit Langote wrote:
> > > On Tue, Jun 4, 2024 at 2:20 AM Tom Lane wrote:
> > > > Peter Eisentraut writes:
> > > > > On 02.06.24 21:46, Tom Lane wrote:
> > > > >> If you don't
> > > > >> like our current behavior, then either you have to say that RETURNING
> > > > >> with a length-limited target type is illegal (which is problematic
> > > > >> for the spec, since they have no such type) or that the cast behaves
> > > > >> like an implicit cast, with errors for overlength input (which I find
> > > > >> to be an unintuitive definition for a construct that names the target
> > > > >> type explicitly).
> > > >
> > > > > It asks for the latter behavior, essentially (but it's not defined in
> > > > > terms of casts). It says:
> > > >
> > > > Meh. Who needs consistency? But I guess the answer is to do what was
> > > > suggested earlier and change the code to use COERCE_IMPLICIT_CAST.
> > >
> > > OK, will post a patch to do so in a new thread on -hackers.
> >
> > Oops, didn't realize that this is already on -hackers.
> >
> > Attached is a patch to use COERCE_IMPLICIT_CAST when the RETURNING
> > type specifies a length limit.
>
> hi.
> i am a little confused.
>
> here[1] tom says:
> > Yeah, I too think this is a cast, and truncation is the spec-defined
> > behavior for casting to varchar with a specific length limit. I see
> > little reason that this should work differently from
> >
> > select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
> > json_serialize
> >
> > {"a":
> > (1 row)
>
> if i understand it correctly, and my english interpretation is fine.
> i think tom means something like:
>
> select json_serialize('{"a":1, "a":2}' returning text)::varchar(5) =
> json_serialize('{"a":1, "a":2}' returning varchar(5));
>
> should return true.
> the master will return true, but apply your patch, the above query
> will yield an error.
The RETURNING variant giving an error is what the standard asks us to
do apparently. I read Tom's last message on this thread as agreeing
to that, even though hesitantly. He can correct me if I got that
wrong.
> your patch will make domain and char(n) behavior inconsistent.
> create domain char2 as char(2);
> SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
> SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);
>
>
> another example:
> SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
> default '"aaa"'::jsonb ON ERROR);
> same value (jsonb "aaa") error on error will yield error,
> but `default expression on error` can coerce the value to char(2),
> which looks a little bit inconsistent, I think.
Interesting examples, thanks for sharing.
Attached updated version should take into account that typmod may be
hiding under domains. Please test.
> --
> current in ExecInitJsonExpr we have
>
> if (jsexpr->coercion_expr)
> ...
> else if (jsexpr->use_json_coercion)
> ...
> else if (jsexpr->use_io_coercion)
> ...
>
> do you think it is necessary to add following asserts:
> Assert (!(jsexpr->coercion_expr && jsexpr->use_json_coercion))
> Assert (!(jsexpr->coercion_expr && jsexpr->use_io_coercion))
Yeah, perhaps, but let's consider this independently please.
--
Thanks, Amit Langote
v2-0001-SQL-JSON-Use-implicit-casts-for-RETURNING-type-wi.patch
Description: Binary data
Re: SQL/JSON query functions context_item doc entry and type requirement
On Fri, Jun 21, 2024 at 9:47 AM David G. Johnston
wrote:
> On Thu, Jun 20, 2024 at 9:01 AM jian he wrote:
>>
>> On Thu, Jun 20, 2024 at 5:46 PM Amit Langote wrote:
>> >
>> > On Thu, Jun 20, 2024 at 1:03 AM David G. Johnston
>> > wrote:
>> > > On Wed, Jun 19, 2024 at 8:29 AM jian he
>> > > wrote:
>> > >>
>> > >> On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack wrote:
>> > >> >
>> > >> > Hi,
>> > >> >
>> > >> > On 06/17/24 02:43, Amit Langote wrote:
>> > >> > > context_item expression can be a value of
>> > >> > > any type that can be cast to jsonb. This includes types
>> > >> > > such as char, text, bpchar,
>> > >> > > character varying, and bytea (with
>> > >> > > ENCODING UTF8), as well as any domains over these
>> > >> > > types.
>> > >> >
>> > >> > Reading this message in conjunction with [0] makes me think that we
>> > >> > are
>> > >> > really talking about a function that takes a first parameter of type
>> > >> > jsonb,
>> > >> > and behaves exactly that way (so any cast required is applied by the
>> > >> > system
>> > >> > ahead of the call). Under those conditions, this seems like an unusual
>> > >> > sentence to add in the docs, at least until we have also documented
>> > >> > that
>> > >> > tan's argument can be of any type that can be cast to double
>> > >> > precision.
>> > >> >
>> > >>
>> > >> I guess it would be fine to add an unusual sentence to the docs.
>> > >>
>> > >> imagine a function: array_avg(anyarray) returns anyelement.
>> > >> array_avg calculate an array's elements's avg. like
>> > >> array('{1,2,3}'::int[]) returns 2.
>> > >> but array_avg won't make sense if the input argument is a date array.
>> > >> so mentioning in the doc: array_avg can accept anyarray, but anyarray
>> > >> cannot date array.
>> > >> seems ok.
>> > >
>> > >
>> > > There is existing wording for this:
>> > >
>> > > "The expression can be of any JSON type, any character string type, or
>> > > bytea in UTF8 encoding."
>> > >
>> > > If you add this sentence to the paragraph the link that already exists,
>> > > which simply points the reader to this sentence, becomes redundant and
>> > > should be removed.
>> >
>> > I've just posted a patch in the other thread [1] to restrict
>> > context_item to be of jsonb type, which users would need to ensure by
>> > adding an explicit cast if needed. I think that makes this
>> > clarification unnecessary.
>> >
>> > > As for table 9.16.3 - it is unwieldy already. Lets try and make the
>> > > core syntax shorter, not longer. We already have precedence in the
>> > > subsequent json_table section - give each major clause item a name then
>> > > below the table define the syntax and meaning for those names. Unlike
>> > > in that section - which probably should be modified too - context_item
>> > > should have its own description line.
>> >
>> > I had posted a patch a little while ago at [1] to render the syntax a
>> > bit differently with each function getting its own syntax synopsis.
>> > Resending it here; have addressed Jian He's comments.
>> >
>> > --
>
>
> I was thinking more like:
>
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index c324906b22..b9d157663a 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -18692,8 +18692,10 @@ $.* ? (@ like_regex "^\\d+$")
>
> json_exists
> json_exists (
> -context_item,
> path_expression
> PASSING { value
> AS varname } ,
> ...
> - { TRUE | FALSE
> | UNKNOWN | ERROR } ON
> ERROR )
> +context_item,
> +path_expression
> +variable_definitions
> +on_error_boolean)
>
>
> Returns true if the SQL/JSON
> path_expression
> @@ -18732,12 +18734,14 @@ ERROR: jsonpath array subscript is out of bounds
>
> json_query
> json_q
Re: ON ERROR in json_query and the like
On Fri, Jun 21, 2024 at 10:01 AM David G. Johnston wrote: > On Thu, Jun 20, 2024 at 5:22 PM Amit Langote wrote: >> >> >> Soft error handling *was* used for catching cast errors in the very >> early versions of this patch (long before I got involved and the >> infrastructure you mention got added). It was taken out after Pavel >> said [1] that he didn't like producing NULL instead of throwing an >> error. Not sure if Pavel's around but it would be good to know why he >> didn't like it at the time. >> > > I'm personally in the "make it error" camp but "make it conform to the > standard" is a stronger membership (in general). > > I see this note in your linked thread: > > > By the standard, it is implementation-defined whether JSON parsing errors > > should be caught by ON ERROR clause. > > Absent someone contradicting that claim I retract my position here and am > fine with failing if these "functions" are supplied with something that > cannot be cast to json. I'd document them like functions that accept json > with the implications that any casting to json happens before the function is > called and thus its arguments do not apply to that step. Thanks for that clarification. So, there are the following options: 1. Disallow anything but jsonb for context_item (the patch I posted yesterday) 2. Continue allowing context_item to be non-json character or utf-8 encoded bytea strings, but document that any parsing errors do not respect the ON ERROR clause. 3. Go ahead and fix implicit casts to jsonb so that any parsing errors respect ON ERROR (no patch written yet). David's vote seems to be 2, which is my inclination too. Markus' vote seems to be either 1 or 3. Anyone else? -- Thanks, Amit Langote
Re: ON ERROR in json_query and the like
On Fri, Jun 21, 2024 at 1:11 AM David G. Johnston wrote: > On Thu, Jun 20, 2024 at 2:19 AM Amit Langote wrote: >> On Mon, Jun 17, 2024 at 10:07 PM Markus Winand >> wrote: >> > > On 17.06.2024, at 08:20, Amit Langote wrote: >> > > Agree that the documentation needs to be clear about this. I'll update >> > > my patch at [1] to add a note next to table 9.16.3. SQL/JSON Query >> > > Functions. >> > >> > Considering another branch of this thread [1] I think the >> > "Supported Features” appendix of the docs should mention that as well. >> > >> > The way I see it is that the standards defines two overloaded >> > JSON_QUERY functions, of which PostgreSQL will support only one. >> > In case of valid JSON, the implied CAST makes it look as though >> > the second variant of these function was supported as well but that >> > illusion totally falls apart once the JSON is not valid anymore. >> > >> > I think it affects the following feature IDs: >> > >> > - T821, Basic SQL/JSON query operators >> > For JSON_VALUE, JSON_TABLE and JSON_EXISTS >> > - T828, JSON_QUERY >> > >> > Also, how hard would it be to add the functions that accept >> > character strings? Is there, besides the effort, any thing else >> > against it? I’m asking because I believe once released it might >> > never be changed — for backward compatibility. >> >> Hmm, I'm starting to think that adding the implied cast to json wasn't >> such a great idea after all, because it might mislead the users to >> think that JSON parsing is transparent (respects ON ERROR), which is >> what you are saying, IIUC. >> > > Actually, the implied cast is exactly the correct thing to do here - the > issue is that we aren't using the newly added soft errors infrastructure [1] > to catch the result of that cast and instead produce whatever output on error > tells us to produce. This seems to be in the realm of doability so we should > try in the interest of being standard conforming. Soft error handling *was* used for catching cast errors in the very early versions of this patch (long before I got involved and the infrastructure you mention got added). It was taken out after Pavel said [1] that he didn't like producing NULL instead of throwing an error. Not sure if Pavel's around but it would be good to know why he didn't like it at the time. I can look into making that work again, but that is not going to make beta2. > I'd even argue to make this an open item unless and until the attempt is > agreed upon to have failed (or it succeeds of course). OK, adding an open item. -- Thanks, Amit Langote [1] https://www.postgresql.org/message-id/CAFj8pRCnzO2cnHi5ebXciV%3DtuGVvAQOW9uPU%2BDQV1GkL31R%3D-g%40mail.gmail.com
Re: SQL/JSON query functions context_item doc entry and type requirement
On Thu, Jun 20, 2024 at 1:03 AM David G. Johnston
wrote:
> On Wed, Jun 19, 2024 at 8:29 AM jian he wrote:
>>
>> On Mon, Jun 17, 2024 at 9:05 PM Chapman Flack wrote:
>> >
>> > Hi,
>> >
>> > On 06/17/24 02:43, Amit Langote wrote:
>> > > context_item expression can be a value of
>> > > any type that can be cast to jsonb. This includes types
>> > > such as char, text, bpchar,
>> > > character varying, and bytea (with
>> > > ENCODING UTF8), as well as any domains over these types.
>> >
>> > Reading this message in conjunction with [0] makes me think that we are
>> > really talking about a function that takes a first parameter of type jsonb,
>> > and behaves exactly that way (so any cast required is applied by the system
>> > ahead of the call). Under those conditions, this seems like an unusual
>> > sentence to add in the docs, at least until we have also documented that
>> > tan's argument can be of any type that can be cast to double precision.
>> >
>>
>> I guess it would be fine to add an unusual sentence to the docs.
>>
>> imagine a function: array_avg(anyarray) returns anyelement.
>> array_avg calculate an array's elements's avg. like
>> array('{1,2,3}'::int[]) returns 2.
>> but array_avg won't make sense if the input argument is a date array.
>> so mentioning in the doc: array_avg can accept anyarray, but anyarray
>> cannot date array.
>> seems ok.
>
>
> There is existing wording for this:
>
> "The expression can be of any JSON type, any character string type, or bytea
> in UTF8 encoding."
>
> If you add this sentence to the paragraph the link that already exists, which
> simply points the reader to this sentence, becomes redundant and should be
> removed.
I've just posted a patch in the other thread [1] to restrict
context_item to be of jsonb type, which users would need to ensure by
adding an explicit cast if needed. I think that makes this
clarification unnecessary.
> As for table 9.16.3 - it is unwieldy already. Lets try and make the core
> syntax shorter, not longer. We already have precedence in the subsequent
> json_table section - give each major clause item a name then below the table
> define the syntax and meaning for those names. Unlike in that section -
> which probably should be modified too - context_item should have its own
> description line.
I had posted a patch a little while ago at [1] to render the syntax a
bit differently with each function getting its own syntax synopsis.
Resending it here; have addressed Jian He's comments.
--
Thanks, Amit Langote
[1]
https://www.postgresql.org/message-id/CA%2BHiwqF2Z6FATWQV6bG9NeKYf%3D%2B%2BfOgmdbYc9gWSNJ81jfqCuA%40mail.gmail.com
v1-0001-SQL-JSON-Various-fixes-to-SQL-JSON-query-function.patch
Description: Binary data
Re: ON ERROR in json_query and the like
Hi, On Mon, Jun 17, 2024 at 10:07 PM Markus Winand wrote: > > On 17.06.2024, at 08:20, Amit Langote wrote: > > Agree that the documentation needs to be clear about this. I'll update > > my patch at [1] to add a note next to table 9.16.3. SQL/JSON Query > > Functions. > > Considering another branch of this thread [1] I think the > "Supported Features” appendix of the docs should mention that as well. > > The way I see it is that the standards defines two overloaded > JSON_QUERY functions, of which PostgreSQL will support only one. > In case of valid JSON, the implied CAST makes it look as though > the second variant of these function was supported as well but that > illusion totally falls apart once the JSON is not valid anymore. > > I think it affects the following feature IDs: > > - T821, Basic SQL/JSON query operators > For JSON_VALUE, JSON_TABLE and JSON_EXISTS > - T828, JSON_QUERY > > Also, how hard would it be to add the functions that accept > character strings? Is there, besides the effort, any thing else > against it? I’m asking because I believe once released it might > never be changed — for backward compatibility. Hmm, I'm starting to think that adding the implied cast to json wasn't such a great idea after all, because it might mislead the users to think that JSON parsing is transparent (respects ON ERROR), which is what you are saying, IIUC. I'm inclined to push the attached patch which puts back the restriction to allow only jsonb arguments, asking users to add an explicit cast if necessary. -- Thanks, Amit Langote v1-0001-SQL-JSON-Only-allow-arguments-of-jsonb-type.patch Description: Binary data
Re: ON ERROR in json_query and the like
Hi,
On Mon, Jun 17, 2024 at 9:47 PM Chapman Flack wrote:
> On 06/17/24 02:20, Amit Langote wrote:
> >>>Apparently, the functions expect JSONB so that a cast is implied
> >>>when providing TEXT. However, the errors during that cast are
> >>>not subject to the ON ERROR clause.
> >>>
> >>>17beta1=# SELECT JSON_QUERY('invalid', '$' NULL ON ERROR);
> >>>ERROR: invalid input syntax for type json
> >>>DETAIL: Token "invalid" is invalid.
> >>>CONTEXT: JSON data, line 1: invalid
> >>>
> >>>Oracle DB and Db2 (LUW) both return NULL in that case.
>
> I wonder, could prosupport rewriting be used to detect that the first
> argument is supplied by a cast, and rewrite the expression to apply the
> cast 'softly'? Or would that behavior be too magical?
I don't think prosupport rewriting can be used, because JSON_QUERY().
We could possibly use "runtime coercion" for context_item so that the
coercion errors can be "caught", which is how we coerce the jsonpath
result to the RETURNING type.
For now, I'm inclined to simply document the limitation that errors
when coercing string arguments to json are always thrown.
--
Thanks, Amit Langote
Re: ON ERROR in json_query and the like
On Mon, Jun 17, 2024 at 10:07 PM Markus Winand wrote:
> > On 17.06.2024, at 08:20, Amit Langote wrote:
> >>> 2. EMPTY [ARRAY|OBJECT] ON ERROR implies ERROR ON EMPTY
> >>>
> >>> 17beta1=# SELECT JSON_QUERY('[]', '$[*]' EMPTY ARRAY ON ERROR) a;
> >>>a
> >>>
> >>>[]
> >>> (1 row)
> >>>
> >>> As NULL ON EMPTY is implied, it should give the same result as
> >>> explicitly adding NULL ON EMPTY:
> >>>
> >>> 17beta1=# SELECT JSON_QUERY('[]', '$[*]' NULL ON EMPTY EMPTY ARRAY ON
> >>> ERROR) a;
> >>>a
> >>> ---
> >>>
> >>> (1 row)
> >>>
> >>> Interestingly, Oracle DB gives the same (wrong) results. Db2 (LUW)
> >>> on the other hand returns NULL for both queries.
> >>>
> >>> I don’t think that PostgreSQL should follow Oracle DB's suit here
> >>> but again, in case this is intentional it should be made explicit
> >>> in the docs.
> >
> > This behavior is a bug and result of an unintentional change that I
> > made at some point after getting involved with this patch set. So I'm
> > going to fix this so that the empty results of jsonpath evaluation use
> > NULL ON EMPTY by default, ie, when the ON EMPTY clause is not present.
> > Attached a patch to do so.
> >
>
> Tested: works.
Pushed, thanks for testing.
I'll work on the documentation updates that may be needed based on
this and nearby discussion(s).
--
Thanks, Amit Langote
Re: Incorrect matching of sql/json PASSING variable names
On Thu, Jun 13, 2024 at 5:04 PM Amit Langote wrote:
> On Thu, Jun 6, 2024 at 6:20 PM Amit Langote wrote:
> >
> > Hi,
> >
> > Alvaro reported off-list that the following should really fail,
> > because the jsonpath expression refers to a PASSING variable that
> > doesn't exist:
> >
> > select json_query('"1"', jsonpath '$xy' passing 2 AS xyz);
> > json_query
> >
> > 2
> > (1 row)
> >
> > This works because of a bug in GetJsonPathVar() whereby it allows a
> > jsonpath expression to reference any prefix of the PASSING variable
> > names.
> >
> > Attached is a patch to fix that.
>
> Here's an updated version that I'll push tomorrow.
Pushed.
(Seems like pgsql-committers notification has stalled.)
--
Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
On Tue, Jun 4, 2024 at 7:03 PM Amit Langote wrote: > On Tue, Jun 4, 2024 at 2:20 AM Tom Lane wrote: > > Peter Eisentraut writes: > > > On 02.06.24 21:46, Tom Lane wrote: > > >> If you don't > > >> like our current behavior, then either you have to say that RETURNING > > >> with a length-limited target type is illegal (which is problematic > > >> for the spec, since they have no such type) or that the cast behaves > > >> like an implicit cast, with errors for overlength input (which I find > > >> to be an unintuitive definition for a construct that names the target > > >> type explicitly). > > > > > It asks for the latter behavior, essentially (but it's not defined in > > > terms of casts). It says: > > > > Meh. Who needs consistency? But I guess the answer is to do what was > > suggested earlier and change the code to use COERCE_IMPLICIT_CAST. > > OK, will post a patch to do so in a new thread on -hackers. Oops, didn't realize that this is already on -hackers. Attached is a patch to use COERCE_IMPLICIT_CAST when the RETURNING type specifies a length limit. Given that this also affects JSON_OBJECT() et al that got added in v16, maybe back-patching is in order but I'd like to hear opinions on that. -- Thanks, Amit Langote v1-0001-SQL-JSON-Use-implicit-casts-for-RETURNING-type-wi.patch Description: Binary data
Re: SQL/JSON query functions context_item doc entry and type requirement
Hi,
On Tue, Jun 4, 2024 at 12:11 AM jian he wrote:
>
> hi
> based on gram.y and function transformJsonValueExpr.
>
> gram.y:
> | JSON_QUERY '('
> json_value_expr ',' a_expr json_passing_clause_opt
> json_returning_clause_opt
> json_wrapper_behavior
> json_quotes_clause_opt
> json_behavior_clause_opt
> ')'
>
> | JSON_EXISTS '('
> json_value_expr ',' a_expr json_passing_clause_opt
> json_on_error_clause_opt
> ')'
>
> | JSON_VALUE '('
> json_value_expr ',' a_expr json_passing_clause_opt
> json_returning_clause_opt
> json_behavior_clause_opt
> ')'
>
> json_format_clause_opt contains:
> | FORMAT_LA JSON
> {
> $$ = (Node *) makeJsonFormat(JS_FORMAT_JSON, JS_ENC_DEFAULT, @1);
> }
>
>
> That means, all the context_item can specify "FORMAT JSON" options,
> in the meantime, do we need to update these functions
> synopsis/signature in the doc?
>
> some examples:
> create table a(b jsonb);
> create table a1(b int4range);
> select json_value(b format json, 'strict $[*]' DEFAULT 9 ON ERROR) from a;
> select json_value(b format json, 'strict $[*]' DEFAULT 9 ON ERROR) from a1;
> select json_value(text '"1"' format json, 'strict $[*]' DEFAULT 9 ON ERROR);
>
>
> transformJsonValueExpr
>
> /* Try to coerce to the target type. */
> coerced = coerce_to_target_type(pstate, expr, exprtype,
> targettype, -1,
> COERCION_EXPLICIT,
> COERCE_EXPLICIT_CAST,
> location);
>
> based on the function transformJsonValueExpr and subfunction
> coerce_to_target_type,
> for SQL/JSON query functions (JSON_EXISTS, JSON_QUERY, and JSON_VALUE)
> the context_item requirement is any data type that not error out while
> explicitly casting to jsonb in coerce_to_target_type.
>
> I played around with it, I think these types can be used in context_item.
> {char,text,bpchar,character varying } and these types of associated domains.
> bytea data type too, but need specify "ENCODING UTF8".
> e.g.
> select json_value(bytea '"1"' format json ENCODING UTF8, 'strict $[*]'
> DEFAULT 9 ON ERROR);
>
>
> Maybe we can add some brief explanation in this para to explain more
> about "context_item"
> {
> SQL/JSON functions JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE()
> described in Table 9.52 can be used to query JSON documents. Each of
> these functions apply a path_expression (the query) to a context_item
> (the document); see Section 9.16.2 for more details on what
> path_expression can contain.
> }
If I understand correctly, you're suggesting that we add a line to the
above paragraph to mention which types are appropriate for
context_item. How about we add the following:
context_item expression can be a value of
any type that can be cast to jsonb. This includes types
such as char, text, bpchar,
character varying, and bytea (with
ENCODING UTF8), as well as any domains over these types.
--
Thanks, Amit Langote
Re: ON ERROR in json_query and the like
Hi,
(apologies for not replying to this thread sooner)
On Tue, May 28, 2024 at 6:57 PM Pavel Stehule wrote:
> út 28. 5. 2024 v 11:29 odesílatel Markus Winand
> napsal:
>>
>> Hi!
>>
>> I’ve noticed two “surprising” (to me) behaviors related to
>> the “ON ERROR” clause of the new JSON query functions in 17beta1.
>>
>> 1. JSON parsing errors are not subject to ON ERROR
>>Apparently, the functions expect JSONB so that a cast is implied
>>when providing TEXT. However, the errors during that cast are
>>not subject to the ON ERROR clause.
>>
>>17beta1=# SELECT JSON_QUERY('invalid', '$' NULL ON ERROR);
>>ERROR: invalid input syntax for type json
>>DETAIL: Token "invalid" is invalid.
>>CONTEXT: JSON data, line 1: invalid
>>
>>Oracle DB and Db2 (LUW) both return NULL in that case.
>>
>>I had a look on the list archive to see if that is intentional but
>>frankly speaking these functions came a long way. In case it is
>>intentional it might be worth adding a note to the docs.
>
>
> I remember a talk about this subject years ago. Originally the JSON_QUERY was
> designed in similar like Oracle, and casting to jsonb was done inside. If I
> remember this behave depends on the fact, so old SQL/JSON has not json type
> and it was based just on processing of plain text. But Postgres has JSON, and
> JSONB and then was more logical to use these types. And because the
> JSON_QUERY uses these types, and the casting is done before the execution of
> the function, then the clause ON ERROR cannot be handled. Moreover, until
> soft errors Postgres didn't allow handling input errors in common functions.
>
> I think so this difference should be mentioned in documentation.
Agree that the documentation needs to be clear about this. I'll update
my patch at [1] to add a note next to table 9.16.3. SQL/JSON Query
Functions.
>> 2. EMPTY [ARRAY|OBJECT] ON ERROR implies ERROR ON EMPTY
>>
>>17beta1=# SELECT JSON_QUERY('[]', '$[*]' EMPTY ARRAY ON ERROR) a;
>> a
>>
>> []
>>(1 row)
>>
>>As NULL ON EMPTY is implied, it should give the same result as
>>explicitly adding NULL ON EMPTY:
>>
>>17beta1=# SELECT JSON_QUERY('[]', '$[*]' NULL ON EMPTY EMPTY ARRAY ON
>> ERROR) a;
>> a
>>---
>>
>>(1 row)
>>
>>Interestingly, Oracle DB gives the same (wrong) results. Db2 (LUW)
>>on the other hand returns NULL for both queries.
>>
>>I don’t think that PostgreSQL should follow Oracle DB's suit here
>>but again, in case this is intentional it should be made explicit
>>in the docs.
This behavior is a bug and result of an unintentional change that I
made at some point after getting involved with this patch set. So I'm
going to fix this so that the empty results of jsonpath evaluation use
NULL ON EMPTY by default, ie, when the ON EMPTY clause is not present.
Attached a patch to do so.
--
Thanks, Amit Langote
[1]
https://www.postgresql.org/message-id/CA%2BHiwqGdineyHfcTEe0%3D8jjXonH3qXi4vFB%2BgRxf1L%2BxR2v_Pw%40mail.gmail.com
v1-0001-SQL-JSON-Correctly-enforce-the-default-ON-EMPTY-b.patch
Description: Binary data
Re: Incorrect matching of sql/json PASSING variable names
On Thu, Jun 6, 2024 at 6:20 PM Amit Langote wrote:
>
> Hi,
>
> Alvaro reported off-list that the following should really fail,
> because the jsonpath expression refers to a PASSING variable that
> doesn't exist:
>
> select json_query('"1"', jsonpath '$xy' passing 2 AS xyz);
> json_query
>
> 2
> (1 row)
>
> This works because of a bug in GetJsonPathVar() whereby it allows a
> jsonpath expression to reference any prefix of the PASSING variable
> names.
>
> Attached is a patch to fix that.
Here's an updated version that I'll push tomorrow.
--
Thanks, Amit Langote
v2-0001-SQL-JSON-Correct-jsonpath-variable-name-matching.patch
Description: Binary data
Incorrect matching of sql/json PASSING variable names
Hi,
Alvaro reported off-list that the following should really fail,
because the jsonpath expression refers to a PASSING variable that
doesn't exist:
select json_query('"1"', jsonpath '$xy' passing 2 AS xyz);
json_query
2
(1 row)
This works because of a bug in GetJsonPathVar() whereby it allows a
jsonpath expression to reference any prefix of the PASSING variable
names.
Attached is a patch to fix that.
Thanks Alvaro for the report.
--
Thanks, Amit Langote
v1-0001-in-transformJsonBehavior-better-handle-default-ex.patch
Description: Binary data
Re: pgsql: Add more SQL/JSON constructor functions
On Tue, Jun 4, 2024 at 2:20 AM Tom Lane wrote: > Peter Eisentraut writes: > > On 02.06.24 21:46, Tom Lane wrote: > >> If you don't > >> like our current behavior, then either you have to say that RETURNING > >> with a length-limited target type is illegal (which is problematic > >> for the spec, since they have no such type) or that the cast behaves > >> like an implicit cast, with errors for overlength input (which I find > >> to be an unintuitive definition for a construct that names the target > >> type explicitly). > > > It asks for the latter behavior, essentially (but it's not defined in > > terms of casts). It says: > > Meh. Who needs consistency? But I guess the answer is to do what was > suggested earlier and change the code to use COERCE_IMPLICIT_CAST. OK, will post a patch to do so in a new thread on -hackers. -- Thanks, Amit Langote
Re: pgsql: Add more SQL/JSON constructor functions
Hi Alvaro,
On Mon, May 27, 2024 at 7:10 PM Alvaro Herrera wrote:
>
> On 2024-May-27, Alvaro Herrera wrote:
>
> > > JSON_SERIALIZE()
>
> I just noticed this behavior, which looks like a bug to me:
>
> select json_serialize('{"a":1, "a":2}' returning varchar(5));
> json_serialize
>
> {"a":
>
> I think this function should throw an error if the destination type
> doesn't have room for the output json. Otherwise, what good is the
> serialization function?
I remember using the reasoning mentioned by David G when testing
json_query() et al with varchar(n), so you get:
select json_query('{"a":1, "a":2}', '$' returning varchar(5));
json_query
{"a":
(1 row)
which is the same as:
select '{"a":1, "a":2}'::varchar(5);
varchar
-
{"a":
(1 row)
Also,
select json_value('{"a":"abcdef"}', '$.a' returning varchar(5) error on error);
json_value
abcde
(1 row)
This behavior comes from using COERCE_EXPLICIT_CAST when creating the
coercion expression to convert json_*() functions' argument to the
RETURNING type.
--
Thanks, Amit Langote
Re: First draft of PG 17 release notes
On Thu, May 9, 2024 at 1:04 PM Bruce Momjian wrote: > I have committed the first draft of the PG 17 release notes; you can > see the results here: > > https://momjian.us/pgsql_docs/release-17.html > > It will be improved until the final release. The item count is 188, > which is similar to recent releases: > > release-10: 189 > release-11: 170 > release-12: 180 > release-13: 178 > release-14: 220 > release-15: 184 > release-16: 206 > release-17: 188 > > I welcome feedback. For some reason it was an easier job than usual. Thanks Bruce for working on this as always. Failed to notice when I read the notes before: Add SQL/JSON constructor functions JSON(), JSON_SCALAR(), and JSON_SERIALIZE() (Amit Langote) Should be: Add SQL/JSON constructor functions JSON(), JSON_SCALAR(), and JSON_SERIALIZE() (Nikita Glukhov, Teodor Sigaev, Oleg Bartunov, Alexander Korotkov, Andrew Dunstan, Amit Langote) Patch attached. -- Thanks, Amit Langote sql-json-credits.patch Description: Binary data
Re: generic plans and "initial" pruning
On Sun, May 19, 2024 at 9:39 AM David Rowley wrote: > For #1, the locks taken for SELECT queries are less likely to conflict > with other locks obtained by PostgreSQL, but at least at the moment if > someone is getting deadlocks with a DDL type operation, they can > change their query or DDL script so that locks are taken in the same > order. If we allowed executor startup to do this then if someone > comes complaining that PG18 deadlocks when PG17 didn't we'd just have > to tell them to live with it. There's a comment at the bottom of > find_inheritance_children_extended() just above the qsort() which > explains about the deadlocking issue. Thought to chime in on this. A deadlock may occur with the execution-time locking proposed in the patch if the DDL script makes assumptions about how a cached plan's execution determines the locking order for children of multiple parent relations. Specifically, the deadlock can happen if the script tries to lock the child relations directly, instead of locking them through their respective parent relations. The patch doesn't change the order of locking of relations mentioned in the query, because that's defined in AcquirePlannerLocks(). -- Thanks, Amit Langote
Re: remaining sql/json patches
Hi Thom,
On Thu, May 16, 2024 at 8:50 AM Thom Brown wrote:
> On Mon, 8 Apr 2024 at 10:09, Amit Langote wrote:
>>
>> On Mon, Apr 8, 2024 at 2:02 PM jian he wrote:
>> > On Mon, Apr 8, 2024 at 11:21 AM jian he
>> > wrote:
>> > >
>> > > On Mon, Apr 8, 2024 at 12:34 AM jian he
>> > > wrote:
>> > > >
>> > > > On Sun, Apr 7, 2024 at 9:36 PM Amit Langote
>> > > > wrote:
>> > > > > 0002 needs an expanded commit message but I've run out of energy
>> > > > > today.
>> > > > >
>> > >
>> > > other than that, it looks good to me.
>> >
>> > one more tiny issue.
>> > +EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view1;
>> > +ERROR: relation "jsonb_table_view1" does not exist
>> > +LINE 1: EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view1...
>> > + ^
>> > maybe you want
>> > EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view7;
>> > at the end of the sqljson_jsontable.sql.
>> > I guess it will be fine, but the format json explain's out is quite big.
>> >
>> > you also need to `drop table s cascade;` at the end of the test?
>>
>> Pushed after fixing this and other issues. Thanks a lot for your
>> careful reviews.
>>
>> I've marked the CF entry for this as committed now:
>> https://commitfest.postgresql.org/47/4377/
>>
>> Let's work on the remaining PLAN clause with a new entry in the next
>> CF, possibly in a new email thread.
>
>
> I've just taken a look at the doc changes,
Thanks for taking a look.
> and I think we need to either remove the leading "select" keyword, or
> uppercase it in the examples.
>
> For example (on
> https://www.postgresql.org/docs/devel/functions-json.html#SQLJSON-QUERY-FUNCTIONS):
>
> json_exists ( context_item, path_expression [ PASSING { value AS varname } [,
> ...]] [ { TRUE | FALSE | UNKNOWN | ERROR } ON ERROR ])
>
> Returns true if the SQL/JSON path_expression applied to the context_item
> using the PASSING values yields any items.
> The ON ERROR clause specifies the behavior if an error occurs; the default is
> to return the boolean FALSE value. Note that if the path_expression is strict
> and ON ERROR behavior is ERROR, an error is generated if it yields no items.
> Examples:
> select json_exists(jsonb '{"key1": [1,2,3]}', 'strict $.key1[*] ? (@ > 2)') →
> t
> select json_exists(jsonb '{"a": [1,2,3]}', 'lax $.a[5]' ERROR ON ERROR) → f
> select json_exists(jsonb '{"a": [1,2,3]}', 'strict $.a[5]' ERROR ON ERROR) →
> ERROR: jsonpath array subscript is out of bounds
>
> Examples are more difficult to read when keywords appear to be at the same
> level as predicates. Plus other examples within tables on the same page
> don't start with "select", and further down, block examples uppercase
> keywords. Either way, I don't like it as it is.
I agree that the leading SELECT should be removed from these examples.
Also, the function names should be capitalized both in the syntax
description and in the examples, even though other functions appearing
on this page aren't.
> Separate from this, I think these tables don't scan well (see json_query for
> an example of what I'm referring to). There is no clear separation of the
> syntax definition, the description, and the example. This is more a matter
> for the website mailing list, but I'm expressing it here to check whether
> others agree.
Hmm, yes, I think I forgot to put around the syntax like
it's done for a few other functions listed on the page.
How about the attached? Other than the above points, it removes the
tags from the description text of each function to turn it into
a single paragraph, because the multi-paragraph style only seems to
appear in this table and it's looking a bit weird now. Though it's
also true that the functions in this table have the longest
descriptions.
--
Thanks, Amit Langote
v1-0001-Doc-some-stylistic-fixes-to-SQL-JSON-query-functi.patch
Description: Binary data
Re: New committers: Melanie Plageman, Richard Guo
On Fri, Apr 26, 2024 at 8:54 PM Jonathan S. Katz wrote: > The Core Team would like to extend our congratulations to Melanie > Plageman and Richard Guo, who have accepted invitations to become our > newest PostgreSQL committers. > > Please join us in wishing them much success and few reverts! Congratulations to both! -- Thanks, Amit Langote
Re: sql/json remaining issue
On Thu, Apr 18, 2024 at 9:33 AM Amit Langote wrote: > On Mon, Apr 15, 2024 at 9:46 PM Amit Langote wrote: > > On Sat, Apr 13, 2024 at 11:12 PM jian he > > wrote: > > > On Fri, Apr 12, 2024 at 5:44 PM Amit Langote > > > wrote: > > > > > > > > > elog(ERROR, "unrecognized json wrapper %d", wrapper); > > > > > should be > > > > > elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); > > > > > > > > Fixed in 0003. > > > > > > > the fix seems not in 0003? > > > other than that, everything looks fine. > > Oops, really fixed now in 0002. > > > I've combined these patches into one -- attached 0001. Will push tomorrow. > > Decided to break the error message improvement patch into its own > after all -- attached 0001. > > > Now studying the JsonBehavior DEFAULT expression issue and your patch. > > I found some more coercion-related expression nodes that must also be > checked along with CoerceViaIO and CoerceToDomain. Also, after fixing > the code to allow them, I found that we'd need to also check > recursively whether their argument expression is also one of the > supported expression nodes. Also, I decided that it's not necessary > to include "cast" in the error message as one of the supported > expressions. > > Will push all today. Totally forgot to drop a note here that I pushed those and marked the 2 open items as resolved. -- Thanks, Amit Langote
Re: sql/json remaining issue
On Mon, Apr 15, 2024 at 9:46 PM Amit Langote wrote: > On Sat, Apr 13, 2024 at 11:12 PM jian he wrote: > > On Fri, Apr 12, 2024 at 5:44 PM Amit Langote > > wrote: > > > > > > > elog(ERROR, "unrecognized json wrapper %d", wrapper); > > > > should be > > > > elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); > > > > > > Fixed in 0003. > > > > > the fix seems not in 0003? > > other than that, everything looks fine. Oops, really fixed now in 0002. > I've combined these patches into one -- attached 0001. Will push tomorrow. Decided to break the error message improvement patch into its own after all -- attached 0001. > Now studying the JsonBehavior DEFAULT expression issue and your patch. I found some more coercion-related expression nodes that must also be checked along with CoerceViaIO and CoerceToDomain. Also, after fixing the code to allow them, I found that we'd need to also check recursively whether their argument expression is also one of the supported expression nodes. Also, I decided that it's not necessary to include "cast" in the error message as one of the supported expressions. Will push all today. -- Thanks, Amit Langote v5-0002-SQL-JSON-Miscellaneous-fixes-and-improvements.patch Description: Binary data v5-0001-SQL-JSON-Improve-some-error-messages.patch Description: Binary data v5-0003-SQL-JSON-Fix-issues-with-DEFAULT-.-ON-ERROR-EMPTY.patch Description: Binary data
Re: sql/json remaining issue
Hi,
On Sat, Apr 13, 2024 at 11:12 PM jian he wrote:
> On Fri, Apr 12, 2024 at 5:44 PM Amit Langote wrote:
> >
> > > elog(ERROR, "unrecognized json wrapper %d", wrapper);
> > > should be
> > > elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);
> >
> > Fixed in 0003.
> >
> the fix seems not in 0003?
> other than that, everything looks fine.
>
>
>
> SELECT * FROM JSON_TABLE (
> '{"favorites":
> {"movies":
> [{"name": "One", "director": "John Doe"},
>{"name": "Two", "director": "Don Joe"}],
> "books":
> [{"name": "Mystery", "authors": [{"name": "Brown Dan"}]},
>{"name": "Wonder", "authors": [{"name": "Jun Murakami"},
> {"name":"Craig Doe"}]}]
> }}'::json, '$.favs[*]'
> COLUMNS (user_id FOR ORDINALITY,
> NESTED '$.movies[*]'
> COLUMNS (
> movie_id FOR ORDINALITY,
> mname text PATH '$.name',
> director text),
> NESTED '$.books[*]'
> COLUMNS (
> book_id FOR ORDINALITY,
> bname text PATH '$.name',
> NESTED '$.authors[*]'
> COLUMNS (
> author_id FOR ORDINALITY,
> author_name text PATH '$.name';
>
>
> I actually did run the query, it returns null.
> '$.favs[*]'
> should be
> '$.favorites[*]'
Oops, fixed.
I've combined these patches into one -- attached 0001. Will push tomorrow.
> one more minor thing, I previously mentioned in getJsonPathVariable
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_OBJECT),
> errmsg("could not find jsonpath variable \"%s\"",
> pnstrdup(varName, varNameLength;
>
> do we need to remove pnstrdup?
Looking at this again, it seems like that's necessary because varName,
being a string extracted from JsonPathItem, is not necessarily
null-terminated. There are many pndstrdup()s in jsonpath_exec.c
because of that aspect.
Now studying the JsonBehavior DEFAULT expression issue and your patch.
--
Thanks, Amit Langote
v4-0001-SQL-JSON-Miscellaneous-fixes-and-improvements.patch
Description: Binary data
Re: sql/json remaining issue
On Thu, Apr 11, 2024 at 12:02 PM jian he wrote:
> On Wed, Apr 10, 2024 at 4:39 PM Amit Langote wrote:
> > Attached is a bit more polished version of that, which also addresses
> > the error messages in JsonPathQuery() and JsonPathValue(). I noticed
> > that there was comment I had written at one point during JSON_TABLE()
> > hacking that said that we should be doing this.
> >
> > I've also added an open item for this.
>
> `
> | NESTED [ PATH ] json_path_specification [ AS json_path_name ]
> COLUMNS ( json_table_column [, ...] )
> NESTED [ PATH ] json_path_specification [ AS json_path_name ] COLUMNS
> ( json_table_column [, ...] )
> `
> "json_path_specification" should be "path_expression"?
Fixed in 0002.
> your explanation about memory usage is clear to me!
>
>
> The following are minor cosmetic issues while applying v2.
> +errmsg("JSON path expression in JSON_VALUE should return singleton
> scalar item")));
> "singleton" is not intuitive to me.
> Then I looked around.
> https://www.postgresql.org/search/?u=%2Fdocs%2F16%2F&q=singleton
> There is only one appearance of "singleton" in the manual.
Yes, singleton is a term used a lot in the source code but let's keep
it out of error messages and docs. So fixed.
> errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequence into
> array.")));
> maybe
> errhint("Use WITH WRAPPER clause to wrap SQL/JSON items into array.")));
> or
> errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequences into
> array.")));
Changed to use "SQL/JSON items into array.".
> then I wonder what's the difference between
> 22038 ERRCODE_SINGLETON_SQL_JSON_ITEM_REQUIRED
> 2203F ERRCODE_SQL_JSON_SCALAR_REQUIRED
>
> i assume '{"hello":"world"}' is a singleton, but not a scalar item?
> if so, then I think the error message within the "if (count > 1)"
> branch in JsonPathValue
> should use ERRCODE_SINGLETON_SQL_JSON_ITEM_REQUIRED
> within the "if (!IsAJsonbScalar(res))" branch should use
> ERRCODE_SQL_JSON_SCALAR_REQUIRED
> ?
> + if (column_name)
> + ereport(ERROR,
> + (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM),
> + errmsg("JSON path expression for column \"%s\" should return
> singleton scalar item",
> + column_name)));
> + else
> + ereport(ERROR,
> + (errcode(ERRCODE_SQL_JSON_SCALAR_REQUIRED),
> + errmsg("JSON path expression in JSON_VALUE should return singleton
> scalar item")));
> the error message seems similar, but the error code is different?
> both within "if (count > 1)" and "if (!IsAJsonbScalar(res))" branch.
Using different error codes for the same error is a copy-paste mistake
on my part. Fixed.
> in src/include/utils/jsonpath.h, comments
> /* SQL/JSON item */
> should be
> /* SQL/JSON query functions */
>
>
> elog(ERROR, "unrecognized json wrapper %d", wrapper);
> should be
> elog(ERROR, "unrecognized json wrapper %d", (int) wrapper);
Fixed in 0003.
--
Thanks, Amit Langote
v3-0001-JSON_TABLE-mention-column-name-in-some-error-mess.patch
Description: Binary data
v3-0002-JSON_TABLE-Use-term-path_expression-more-consiste.patch
Description: Binary data
v3-0003-SQL-JSON-Fix-some-comments-in-jsonpath.h.patch
Description: Binary data
Re: sql/json remaining issue
On Tue, Apr 9, 2024 at 8:37 PM Amit Langote wrote: > On Tue, Apr 9, 2024 at 4:47 PM jian he wrote: > > the last query error message is: > > ` > > ERROR: no SQL/JSON item > > ` > > > > we are in ExecEvalJsonExprPath, can we output it to be: > > ` > > ERROR: after applying json_path "5s", no SQL/JSON item found > > ` > > in a json_table query, we can have multiple path_expressions, like the > > above query. > > it's not easy to know applying which path_expression failed. > > Hmm, I'm not so sure about mentioning the details of the path because > path names are optional and printing path expression itself is not a > good idea. Perhaps, we could mention the column name which would > always be there, but we'd then need to add a new field column_name > that's optionally set to JsonFuncExpr and JsonExpr, that is, when they > are being set up for JSON_TABLE() columns. As shown in the attached. > With the patch you'll get: > > ERROR: no SQL/JSON item found for column "b" Attached is a bit more polished version of that, which also addresses the error messages in JsonPathQuery() and JsonPathValue(). I noticed that there was comment I had written at one point during JSON_TABLE() hacking that said that we should be doing this. I've also added an open item for this. -- Thanks, Amit Langote v2-0001-JSON_TABLE-mention-column-name-in-some-error-mess.patch Description: Binary data
Re: sql/json remaining issue
Hi,
On Tue, Apr 9, 2024 at 4:47 PM jian he wrote:
>
> hi.
> `
> | NESTED [ PATH ] json_path_specification [ AS json_path_name ]
> COLUMNS ( json_table_column [, ...] )
> NESTED [ PATH ] json_path_specification [ AS json_path_name ] COLUMNS
> ( json_table_column [, ...] )
> `
> "json_path_specification" should be "path_expression"?
> --
> drop table s1;
> create or replace function random_text_1000() returns text
> as $$select string_agg(md5(random()::text),'') from
> generate_Series(1,1000) s $$ LANGUAGE SQL;
>
> create unlogged table s1(a int GENERATED BY DEFAULT AS IDENTITY, js jsonb);
> insert into s1(js)
> select jsonb ('{"a":{"za":[{"z1": [11,]},{"z21": [22, 234,' || g
> || ']},{"z22": [32, 204,145]}]},"c": ' || g
> || ',"id": "' || random_text_1000() || '"}')
> from generate_series(1_000_000, 1_000_000) g;
> insert into s1(js)
> select jsonb ('{"a":{"za":[{"z1": [11,]},{"z21": [22, 234,' || g
> || ']},{"z22": [32, 204,145]}]},"c": ' || g
> || ',"id": "' || random_text_1000() || '"}')
> from generate_series(235, 235 + 20,1) g;
>
> select count(*), pg_size_pretty(pg_total_relation_size('s1')) from s1;
> count | pg_size_pretty
> +
> 22 | 6398 MB
> --
>
> explain(analyze, costs off,buffers, timing)
> SELECT sub.*, s.a as s_a FROM s,
> (values(23)) x(x),
> generate_series(13, 13) y,
> JSON_TABLE(js, '$' AS c1 PASSING x AS x, y AS y
> COLUMNS (
> xx1 int PATH '$.c',
> NESTED PATH '$.a.za[2]' COLUMNS (NESTED PATH '$.z22[*]' as z22 COLUMNS
> (c int PATH '$')),
> NESTED PATH '$.a.za[1]' COLUMNS (d json PATH '$ ? (@.z21[*] == ($"x" -1))'),
> NESTED PATH '$.a.za[0]' COLUMNS (NESTED PATH '$.z1[*] ? (@ >= ($"x"
> -2))' as z1 COLUMNS (a int PATH '$')),
> NESTED PATH '$.a.za[1]' COLUMNS
> (NESTED PATH '$.z21[*] ? (@ >= ($"y" +121))' as z21 COLUMNS (b int
> PATH '$ ? (@ <= ($"x" + 76))' default -1000 ON EMPTY))
> )) sub;
>
> for one jsonb, it can expand to 7 rows, the above query will return
> around 1.4 million rows.
> i use the above query, and pg_log_backend_memory_contexts in another
> session to check the memory usage.
> didn't find memory over consumed issue.
>
> but I am not sure this is the right way to check the memory consumption.
> --
> begin;
> SELECT sub.*, s.a as s_a FROM s,
> (values(23)) x(x),
> generate_series(13, 13) y,
> JSON_TABLE(js, '$' AS c1 PASSING x AS x, y AS y
> COLUMNS (
> xx1 int PATH '$.c',
> NESTED PATH '$.a.za[2]' COLUMNS (NESTED PATH '$.z22[*]' as z22 COLUMNS
> (c int PATH '$')),
> NESTED PATH '$.a.za[1]' COLUMNS (d json PATH '$ ? (@.z21[*] == ($"x" -1))'),
> NESTED PATH '$.a.za[0]' COLUMNS (NESTED PATH '$.z1[*] ? (@ >= ($"x"
> -2))' as z1 COLUMNS (a int PATH '$')),
> NESTED PATH '$.a.za[1]' COLUMNS
> (NESTED PATH '$.z21[*] ? (@ >= ($"y" +121))' as z21 COLUMNS (b int
> PATH '$ ? (@ <= ($"x" + 76))' error ON EMPTY))
> )) sub;
> rollback;
>
> only the last row will fail, because of "error ON EMPTY", (1_000_000
> <= 23 + 76) is false.
> I remember the very previous patch, because of error cleanup, it took
> a lot of resources.
> does our current implementation, only the very last row fail, will it
> be easy to clean up the transaction?
I am not sure I understand your concern. Could you please rephrase
it? Which previous patch are you referring to and what problem did it
cause with respect to error cleanup?
Per-row memory allocated for each successful output row JSON_TABLE()
doesn't pile up, because it's allocated in a context that is reset
after evaluating each row; see tfuncLoadRows(). But again I may be
misunderstanding your concern.
> the last query error message is:
> `
> ERROR: no SQL/JSON item
> `
>
> we are in ExecEvalJsonExprPath, can we output it to be:
> `
> ERROR: after applying json_path "5s", no SQL/JSON item found
> `
> in a json_table query, we can have multiple path_expressions, like the
> above query.
> it's not easy to know applying which path_expression failed.
Hmm, I'm not so sure about mentioning the details of the path because
path names are optional and printing path expression itself is not a
good idea. Perhaps, we could mention the column name which would
always be there, but we'd then need to add a new field column_name
that's optionally set to JsonFuncExpr and JsonExpr, that is, when they
are being set up for JSON_TABLE() columns. As shown in the attached.
With the patch you'll get:
ERROR: no SQL/JSON item found for column "b"
--
Thanks, Amit Langote
v1-0001-JSON_TABLE-mention-column-name-in-the-ON-EMPTY-er.patch
Description: Binary data
Re: remaining sql/json patches
On Mon, Apr 8, 2024 at 2:02 PM jian he wrote: > On Mon, Apr 8, 2024 at 11:21 AM jian he wrote: > > > > On Mon, Apr 8, 2024 at 12:34 AM jian he wrote: > > > > > > On Sun, Apr 7, 2024 at 9:36 PM Amit Langote > > > wrote: > > > > 0002 needs an expanded commit message but I've run out of energy today. > > > > > > > > other than that, it looks good to me. > > one more tiny issue. > +EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view1; > +ERROR: relation "jsonb_table_view1" does not exist > +LINE 1: EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view1... > + ^ > maybe you want > EXPLAIN (COSTS OFF, VERBOSE) SELECT * FROM jsonb_table_view7; > at the end of the sqljson_jsontable.sql. > I guess it will be fine, but the format json explain's out is quite big. > > you also need to `drop table s cascade;` at the end of the test? Pushed after fixing this and other issues. Thanks a lot for your careful reviews. I've marked the CF entry for this as committed now: https://commitfest.postgresql.org/47/4377/ Let's work on the remaining PLAN clause with a new entry in the next CF, possibly in a new email thread. -- Thanks, Amit Langote
Re: generic plans and "initial" pruning
Hi Andrey, On Sun, Mar 31, 2024 at 2:03 PM Andrey M. Borodin wrote: > > On 6 Dec 2023, at 23:52, Robert Haas wrote: > > > > I hope that it's at least somewhat useful. > > > On 5 Jan 2024, at 15:46, vignesh C wrote: > > > > There is a leak reported > > Hi Amit, > > this is a kind reminder that some feedback on your patch[0] is waiting for > your reply. > Thank you for your work! Thanks for moving this to the next CF. My apologies (especially to Robert) for not replying on this thread for a long time. I plan to start working on this soon. -- Thanks, Amit Langote
Re: remaining sql/json patches
On Sun, Apr 7, 2024 at 10:21 PM jian he wrote:
> On Sun, Apr 7, 2024 at 12:30 PM jian he wrote:
> >
> > other than that, it looks good to me.
> while looking at it again.
>
> + | NESTED path_opt Sconst
> + COLUMNS '(' json_table_column_definition_list ')'
> + {
> + JsonTableColumn *n = makeNode(JsonTableColumn);
> +
> + n->coltype = JTC_NESTED;
> + n->pathspec = (JsonTablePathSpec *)
> + makeJsonTablePathSpec($3, NULL, @3, -1);
> + n->columns = $6;
> + n->location = @1;
> + $$ = (Node *) n;
> + }
> + | NESTED path_opt Sconst AS name
> + COLUMNS '(' json_table_column_definition_list ')'
> + {
> + JsonTableColumn *n = makeNode(JsonTableColumn);
> +
> + n->coltype = JTC_NESTED;
> + n->pathspec = (JsonTablePathSpec *)
> + makeJsonTablePathSpec($3, $5, @3, @5);
> + n->columns = $8;
> + n->location = @1;
> + $$ = (Node *) n;
> + }
> + ;
> +
> +path_opt:
> + PATH
> + | /* EMPTY */
> ;
>
> for `NESTED PATH`, `PATH` is optional.
> So for the doc, many places we need to replace `NESTED PATH` to `NESTED
> [PATH]`?
Thanks for checking.
I've addressed most of your comments in the recent days including
today's. Thanks for the patches for adding new test cases. That was
very helpful.
I've changed the recursive structure of JsonTablePlanNextRow(). While
it still may not be perfect, I think it's starting to look good now.
0001 is a patch to fix up get_json_expr_options() so that it now emits
WRAPPER and QUOTES such that they work correctly.
0002 needs an expanded commit message but I've run out of energy today.
--
Thanks, Amit Langote
v51-0001-Fix-JsonExpr-deparsing-to-emit-QUOTES-and-WRAPPE.patch
Description: Binary data
v51-0002-JSON_TABLE-Add-support-for-NESTED-columns.patch
Description: Binary data
Re: remaining sql/json patches
Hi,
On Sat, Apr 6, 2024 at 3:55 PM jian he wrote:
> On Sat, Apr 6, 2024 at 2:03 PM Amit Langote wrote:
> >
> > >
> > > * problem with type "char". the view def output is not the same as
> > > the select * from v1.
> > >
> > > create or replace view v1 as
> > > SELECT col FROM s,
> > > JSON_TABLE(jsonb '{"d": ["hello", "hello1"]}', '$' as c1
> > > COLUMNS(col "char" path '$.d' without wrapper keep quotes))sub;
> > >
> > > \sv v1
> > > CREATE OR REPLACE VIEW public.v1 AS
> > > SELECT sub.col
> > >FROM s,
> > > JSON_TABLE(
> > > '{"d": ["hello", "hello1"]}'::jsonb, '$' AS c1
> > > COLUMNS (
> > > col "char" PATH '$."d"'
> > > )
> > > ) sub
> > > one under the hood called JSON_QUERY_OP, another called JSON_VALUE_OP.
> >
> > Hmm, I don't see a problem as long as both are equivalent or produce
> > the same result. Though, perhaps we could make
> > get_json_expr_options() also deparse JSW_NONE explicitly into "WITHOUT
> > WRAPPER" instead of a blank. But that's existing code, so will take
> > care of it as part of the above open item.
> >
> > > I will do extensive checking for other types later, so far, other than
> > > these two issues,
> > > get_json_table_columns is pretty solid, I've tried nested columns with
> > > nested columns, it just works.
> >
> > Thanks for checking.
> >
> After applying v50, this type also has some issues.
> CREATE OR REPLACE VIEW t1 as
> SELECT sub.* FROM JSON_TABLE(jsonb '{"d": ["hello", "hello1"]}',
> '$' AS c1 COLUMNS (
> "tsvector0" tsvector path '$.d' without wrapper omit quotes,
> "tsvector1" tsvector path '$.d' without wrapper keep quotes))sub;
> table t1;
>
> return
> tsvector0|tsvector1
> -+-
> '"hello1"]' '["hello",' | '"hello1"]' '["hello",'
> (1 row)
>
> src5=# \sv t1
> CREATE OR REPLACE VIEW public.t1 AS
> SELECT tsvector0,
> tsvector1
>FROM JSON_TABLE(
> '{"d": ["hello", "hello1"]}'::jsonb, '$' AS c1
> COLUMNS (
> tsvector0 tsvector PATH '$."d"' OMIT QUOTES,
> tsvector1 tsvector PATH '$."d"'
> )
> ) sub
>
> but
>
> SELECT tsvector0,
> tsvector1
>FROM JSON_TABLE(
> '{"d": ["hello", "hello1"]}'::jsonb, '$' AS c1
> COLUMNS (
> tsvector0 tsvector PATH '$."d"' OMIT QUOTES,
> tsvector1 tsvector PATH '$."d"'
> )
> ) sub
>
> only return
> tsvector0| tsvector1
> -+---
> '"hello1"]' '["hello",' |
Yep, we *should* fix get_json_expr_options() to emit KEEP QUOTES and
WITHOUT WRAPPER options so that transformJsonTableColumns() does the
correct thing when you execute the \sv output. Like this:
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 283ca53cb5..5a6aabe100 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -8853,9 +8853,13 @@ get_json_expr_options(JsonExpr *jsexpr,
deparse_context *context,
appendStringInfo(context->buf, " WITH CONDITIONAL WRAPPER");
else if (jsexpr->wrapper == JSW_UNCONDITIONAL)
appendStringInfo(context->buf, " WITH UNCONDITIONAL WRAPPER");
+else if (jsexpr->wrapper == JSW_NONE)
+appendStringInfo(context->buf, " WITHOUT WRAPPER");
if (jsexpr->omit_quotes)
appendStringInfo(context->buf, " OMIT QUOTES");
+else
+appendStringInfo(context->buf, " KEEP QUOTES");
}
Will get that pushed tomorrow. Thanks for the test case.
--
Thanks, Amit Langote
Re: remaining sql/json patches
On Sat, Apr 6, 2024 at 12:31 PM jian he wrote:
> On Fri, Apr 5, 2024 at 8:35 PM Amit Langote wrote:
> > Here's one. Main changes:
> >
> > * Fixed a bug in get_table_json_columns() which caused nested columns
> > to be deparsed incorrectly, something Jian reported upthread.
> > * Simplified the algorithm in JsonTablePlanNextRow()
> >
> > I'll post another revision or two maybe tomorrow, but posting what I
> > have now in case Jian wants to do more testing.
>
> i am using the upthread view validation function.
> by comparing `execute the view definition` and `select * from the_view`,
> I did find 2 issues.
>
> * problem in transformJsonBehavior, JSON_BEHAVIOR_DEFAULT branch.
> I think we can fix this problem later, since sql/json query function
> already committed?
>
> CREATE DOMAIN jsonb_test_domain AS text CHECK (value <> 'foo');
> normally, we do:
> SELECT JSON_VALUE(jsonb '{"d1": "H"}', '$.a2' returning
> jsonb_test_domain DEFAULT 'foo' ON ERROR);
>
> but parsing back view def, we do:
> SELECT JSON_VALUE(jsonb '{"d1": "H"}', '$.a2' returning
> jsonb_test_domain DEFAULT 'foo'::text::jsonb_test_domain ON ERROR);
>
> then I found the following two queries should not be error out.
> SELECT JSON_VALUE(jsonb '{"d1": "H"}', '$.a2' returning
> jsonb_test_domain DEFAULT 'foo1'::text::jsonb_test_domain ON ERROR);
> SELECT JSON_VALUE(jsonb '{"d1": "H"}', '$.a2' returning
> jsonb_test_domain DEFAULT 'foo1'::jsonb_test_domain ON ERROR);
Yeah, added an open item for this:
https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items#Open_Issues
>
>
> * problem with type "char". the view def output is not the same as
> the select * from v1.
>
> create or replace view v1 as
> SELECT col FROM s,
> JSON_TABLE(jsonb '{"d": ["hello", "hello1"]}', '$' as c1
> COLUMNS(col "char" path '$.d' without wrapper keep quotes))sub;
>
> \sv v1
> CREATE OR REPLACE VIEW public.v1 AS
> SELECT sub.col
>FROM s,
> JSON_TABLE(
> '{"d": ["hello", "hello1"]}'::jsonb, '$' AS c1
> COLUMNS (
> col "char" PATH '$."d"'
> )
> ) sub
> one under the hood called JSON_QUERY_OP, another called JSON_VALUE_OP.
Hmm, I don't see a problem as long as both are equivalent or produce
the same result. Though, perhaps we could make
get_json_expr_options() also deparse JSW_NONE explicitly into "WITHOUT
WRAPPER" instead of a blank. But that's existing code, so will take
care of it as part of the above open item.
> I will do extensive checking for other types later, so far, other than
> these two issues,
> get_json_table_columns is pretty solid, I've tried nested columns with
> nested columns, it just works.
Thanks for checking.
--
Thanks, Amit Langote
Re: remaining sql/json patches
Hi Michael,
On Fri, Apr 5, 2024 at 3:07 PM Michael Paquier wrote:
> On Fri, Apr 05, 2024 at 09:00:00AM +0300, Alexander Lakhin wrote:
> > Please look at an assertion failure:
> > TRAP: failed Assert("count <= tupdesc->natts"), File: "parse_relation.c",
> > Line: 3048, PID: 1325146
> >
> > triggered by the following query:
> > SELECT * FROM JSON_TABLE('0', '$' COLUMNS (js int PATH '$')),
> > COALESCE(row(1)) AS (a int, b int);
> >
> > Without JSON_TABLE() I get:
> > ERROR: function return row and query-specified return row do not match
> > DETAIL: Returned row contains 1 attribute, but query expects 2.
>
> I've added an open item on this one. We need to keep track of all
> that.
We figured out that this is an existing bug unrelated to JSON_TABLE(),
which Alexander reported to -bugs:
https://postgr.es/m/18422-89ca86c8eac52...@postgresql.org
I have moved the item to Older Bugs:
https://wiki.postgresql.org/wiki/PostgreSQL_17_Open_Items#Live_issues
--
Thanks, Amit Langote
Re: remaining sql/json patches
On Thu, Apr 4, 2024 at 9:02 PM Amit Langote wrote: > I'll post the rebased 0002 tomorrow after addressing your comments. Here's one. Main changes: * Fixed a bug in get_table_json_columns() which caused nested columns to be deparsed incorrectly, something Jian reported upthread. * Simplified the algorithm in JsonTablePlanNextRow() I'll post another revision or two maybe tomorrow, but posting what I have now in case Jian wants to do more testing. -- Thanks, Amit Langote v50-0001-JSON_TABLE-Add-support-for-NESTED-columns.patch Description: Binary data
Re: remaining sql/json patches
On Fri, Apr 5, 2024 at 5:00 PM Alexander Lakhin wrote: > 05.04.2024 10:09, Amit Langote wrote: > > Seems like it might be a pre-existing issue, because I can also > > reproduce the crash with: > > That's strange, because I get the error (on master, 6f132ed69). > With backtrace_functions = 'tupledesc_match', I see > 2024-04-05 10:48:27.827 MSK client backend[2898632] regress ERROR: function > return row and query-specified return row do > not match > 2024-04-05 10:48:27.827 MSK client backend[2898632] regress DETAIL: Returned > row contains 1 attribute, but query expects 2. > 2024-04-05 10:48:27.827 MSK client backend[2898632] regress BACKTRACE: > tupledesc_match at execSRF.c:948:3 > ExecMakeTableFunctionResult at execSRF.c:427:13 > FunctionNext at nodeFunctionscan.c:94:5 > ExecScanFetch at execScan.c:131:10 > ExecScan at execScan.c:180:10 > ExecFunctionScan at nodeFunctionscan.c:272:1 > ExecProcNodeFirst at execProcnode.c:465:1 > ExecProcNode at executor.h:274:9 > (inlined by) ExecutePlan at execMain.c:1646:10 > standard_ExecutorRun at execMain.c:363:3 > ExecutorRun at execMain.c:305:1 > PortalRunSelect at pquery.c:926:26 > PortalRun at pquery.c:775:8 > exec_simple_query at postgres.c:1282:3 > PostgresMain at postgres.c:4684:27 > BackendMain at backend_startup.c:57:2 > pgarch_die at pgarch.c:847:1 > BackendStartup at postmaster.c:3593:8 > ServerLoop at postmaster.c:1674:6 > main at main.c:184:3 > /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80) > [0x7f37127f0e40] > 2024-04-05 10:48:27.827 MSK client backend[2898632] regress STATEMENT: > SELECT * FROM COALESCE(row(1)) AS (a int, b int); > > That's why I had attributed the failure to JSON_TABLE(). > > Though SELECT * FROM generate_series(1, 1), COALESCE(row(1)) AS (a int, b > int); > really triggers the assert too. > Sorry for the noise... No worries. Let's start another thread so that this gets more attention. -- Thanks, Amit Langote
Re: remaining sql/json patches
Hi Alexander,
On Fri, Apr 5, 2024 at 3:00 PM Alexander Lakhin wrote:
>
> Hello Amit,
>
> 04.04.2024 15:02, Amit Langote wrote:
> > Pushed after fixing these and a few other issues. I didn't include
> > the testing function you proposed in your other email. It sounds
> > useful for testing locally but will need some work before we can
> > include it in the tree.
> >
> > I'll post the rebased 0002 tomorrow after addressing your comments.
>
> Please look at an assertion failure:
> TRAP: failed Assert("count <= tupdesc->natts"), File: "parse_relation.c",
> Line: 3048, PID: 1325146
>
> triggered by the following query:
> SELECT * FROM JSON_TABLE('0', '$' COLUMNS (js int PATH '$')),
>COALESCE(row(1)) AS (a int, b int);
>
> Without JSON_TABLE() I get:
> ERROR: function return row and query-specified return row do not match
> DETAIL: Returned row contains 1 attribute, but query expects 2.
Thanks for the report.
Seems like it might be a pre-existing issue, because I can also
reproduce the crash with:
SELECT * FROM COALESCE(row(1)) AS (a int, b int);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>
Backtrace:
#0 __pthread_kill_implementation (threadid=281472845250592,
signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1 0x806c4334 in __pthread_kill_internal (signo=6,
threadid=) at pthread_kill.c:78
#2 0x8067c73c in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#3 0x80669034 in __GI_abort () at abort.c:79
#4 0x00ad9d4c in ExceptionalCondition (conditionName=0xcbb368
"!(tupdesc->natts >= colcount)", errorType=0xcbb278 "FailedAssertion",
fileName=0xcbb2c8 "nodeFunctionscan.c",
lineNumber=379) at assert.c:54
#5 0x0073edec in ExecInitFunctionScan (node=0x293d4ed0,
estate=0x293d51b8, eflags=16) at nodeFunctionscan.c:379
#6 0x00724bc4 in ExecInitNode (node=0x293d4ed0,
estate=0x293d51b8, eflags=16) at execProcnode.c:248
#7 0x0071b1cc in InitPlan (queryDesc=0x292f5d78, eflags=16)
at execMain.c:1006
#8 0x00719f6c in standard_ExecutorStart
(queryDesc=0x292f5d78, eflags=16) at execMain.c:252
#9 0x00719cac in ExecutorStart (queryDesc=0x292f5d78,
eflags=0) at execMain.c:134
#10 0x00945520 in PortalStart (portal=0x29399458, params=0x0,
eflags=0, snapshot=0x0) at pquery.c:527
#11 0x0093ee50 in exec_simple_query (query_string=0x29332d38
"SELECT * FROM COALESCE(row(1)) AS (a int, b int);") at
postgres.c:1175
#12 0x00943cb8 in PostgresMain (argc=1, argv=0x2935d610,
dbname=0x2935d450 "postgres", username=0x2935d430 "amit") at
postgres.c:4297
#13 0x0087e978 in BackendRun (port=0x29356c00) at postmaster.c:4517
#14 0x0087e0bc in BackendStartup (port=0x29356c00) at postmaster.c:4200
#15 0x00879638 in ServerLoop () at postmaster.c:1725
#16 0x00878eb4 in PostmasterMain (argc=3, argv=0x292eeac0) at
postmaster.c:1398
#17 0x00791db8 in main (argc=3, argv=0x292eeac0) at main.c:228
Backtrace looks a bit different with a query similar to yours:
SELECT * FROM generate_series(1, 1), COALESCE(row(1)) AS (a int, b int);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>
#0 __pthread_kill_implementation (threadid=281472845250592,
signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
#1 0x806c4334 in __pthread_kill_internal (signo=6,
threadid=) at pthread_kill.c:78
#2 0x8067c73c in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#3 0x80669034 in __GI_abort () at abort.c:79
#4 0x00ad9d4c in ExceptionalCondition (conditionName=0xc903b0
"!(count <= tupdesc->natts)", errorType=0xc8f8c8 "FailedAssertion",
fileName=0xc8f918 "parse_relation.c",
lineNumber=2649) at assert.c:54
#5 0x00649664 in expandTupleDesc (tupdesc=0x293da188,
eref=0x293d7318, count=2, offset=0, rtindex=2, sublevels_up=0,
location=-1, include_dropped=true, colnames=0x0,
colvars=0xc39253c8) at parse_relation.c:2649
#6 0x00648d08 in expandRTE (rte=0x293d7390, rtindex=2,
sublevels_up=0, location=-1, include_dropped=true, colnames=0x0,
colvars=0xc39253c8) at parse_relation.c:2361
#7 0x00849bd0 in build_physical_tlist (root=0x293d5318,
rel=0x293d88e8) at plancat.c:1681
#8 0x00806ad0 in create_scan_plan (root=0x293d5318,
best_path=0x293cd888, flags=0) at createplan.c:605
#9 0x0080666c in create_plan_recurse (root=0x293d5318,
best
Re: remaining sql/json patches
On Wed, Apr 3, 2024 at 11:48 PM jian he wrote:
> hi.
> +
> + json_table is an SQL/JSON function which
> + queries JSON data
> + and presents the results as a relational view, which can be accessed as a
> + regular SQL table. You can only use
> json_table inside the
> + FROM clause of a SELECT,
> + UPDATE, DELETE, or
> MERGE
> + statement.
> +
>
> the only issue is that MERGE Synopsis don't have
> FROM clause.
> other than that, it's quite correct.
> see following tests demo:
>
> drop table ss;
> create table ss(a int);
> insert into ss select 1;
> delete from ss using JSON_TABLE(jsonb '1', '$' COLUMNS (a int PATH '$'
> ) ERROR ON ERROR) jt where jt.a = 1;
> insert into ss select 2;
> update ss set a = 1 from JSON_TABLE(jsonb '2', '$' COLUMNS (a int PATH
> '$')) jt where jt.a = 2;
> DROP TABLE IF EXISTS target;
> CREATE TABLE target (tid integer, balance integer) WITH
> (autovacuum_enabled=off);
> INSERT INTO target VALUES (1, 10),(2, 20),(3, 30);
> MERGE INTO target USING JSON_TABLE(jsonb '2', '$' COLUMNS (a int PATH
> '$' ) ERROR ON ERROR) source(sid)
> ON target.tid = source.sid
> WHEN MATCHED THEN UPDATE SET balance = 0
> returning *;
> --
>
> +
> + To split the row pattern into columns, json_table
> + provides the COLUMNS clause that defines the
> + schema of the created view. For each column, a separate path expression
> + can be specified to be evaluated against the row pattern to get a
> + SQL/JSON value that will become the value for the specified column in
> + a given output row.
> +
> should be "an SQL/JSON".
>
> +
> + Inserts a SQL/JSON value obtained by applying
> + path_expression against the row pattern into
> + the view's output row after coercing it to specified
> + type.
> +
> should be "an SQL/JSON".
>
> "coercing it to specified type"
> should be
> "coercing it to the specified type"?
> ---
> +
> + The value corresponds to whether evaluating the PATH
> + expression yields any SQL/JSON values.
> +
> maybe we can change to
> +
> + The value corresponds to whether applying the
> path_expression
> + expression yields any SQL/JSON values.
> +
> so it looks more consistent with the preceding paragraph.
>
> +
> + Optionally, ON ERROR can be used to specify whether
> + to throw an error or return the specified value to handle structural
> + errors, respectively. The default is to return a boolean value
> + FALSE.
> +
> we don't need "respectively" here?
>
> + if (jt->on_error &&
> + jt->on_error->btype != JSON_BEHAVIOR_ERROR &&
> + jt->on_error->btype != JSON_BEHAVIOR_EMPTY &&
> + jt->on_error->btype != JSON_BEHAVIOR_EMPTY_ARRAY)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("invalid ON ERROR behavior"),
> + errdetail("Only EMPTY or ERROR is allowed for ON ERROR in JSON_TABLE()."),
> + parser_errposition(pstate, jt->on_error->location));
>
> errdetail("Only EMPTY or ERROR is allowed for ON ERROR in JSON_TABLE()."),
> maybe change to something like:
> `
> errdetail("Only EMPTY or ERROR is allowed for ON ERROR in the
> top-level JSON_TABLE() ").
> `
> i guess mentioning "top-level" is fine.
> since "top-level", we have 19 appearances in functions-json.html.
Thanks for checking.
Pushed after fixing these and a few other issues. I didn't include
the testing function you proposed in your other email. It sounds
useful for testing locally but will need some work before we can
include it in the tree.
I'll post the rebased 0002 tomorrow after addressing your comments.
--
Thanks, Amit Langote
Re: remaining sql/json patches
Hi Alvaro, On Fri, Mar 29, 2024 at 2:04 AM Alvaro Herrera wrote: > On 2024-Mar-28, Amit Langote wrote: > > > Here's patch 1 for the time being that implements barebones > > JSON_TABLE(), that is, without NESTED paths/columns and PLAN clause. > > I've tried to shape the interfaces so that those features can be added > > in future commits without significant rewrite of the code that > > implements barebones JSON_TABLE() functionality. I'll know whether > > that's really the case when I rebase the full patch over it. > > I think this barebones patch looks much closer to something that can be > committed for pg17, given the current commitfest timeline. Maybe we > should just slip NESTED and PLAN to pg18 to focus current efforts into > getting the basic functionality in 17. When I looked at the JSON_TABLE > patch last month, it appeared far too large to be reviewable in > reasonable time. The fact that this split now exists gives me hope that > we can get at least the first part of it. Thanks for chiming in. I agree that 0001 looks more manageable. > (A note that PLAN seems to correspond to separate features T824+T838, so > leaving that one out would still let us claim T821 "Basic SQL/JSON query > operators" ... however, the NESTED clause does not appear to be a > separate SQL feature; in particular it does not appear to correspond to > T827, though I may be reading the standard wrong. So if we don't have > NESTED, apparently we could not claim to support T821.) I've posted 0002 just now, which shows that adding just NESTED but not PLAN might be feasible. -- Thanks, Amit Langote
Re: remaining sql/json patches
On Wed, Mar 27, 2024 at 12:42 PM jian he wrote: > hi. > I don't fully understand all the code in json_table patch. > maybe we can split it into several patches, I'm working on exactly that atm. > like: > * no nested json_table_column. > * nested json_table_column, with PLAN DEFAULT > * nested json_table_column, with PLAN ( json_table_plan ) Yes, I think it will end up something like this. I'll try to post the breakdown tomorrow. -- Thanks, Amit Langote
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
On Mon, Mar 25, 2024 at 7:25 PM Richard Guo wrote: > On Mon, Mar 25, 2024 at 5:17 PM Amit Langote wrote: >> I've pushed this now. >> >> When updating the commit message, I realized that you had made the >> right call to divide the the changes around not translating the dummy >> SpecialJoinInfos into a separate patch. So, I pushed it like that. > > > Sorry I'm late for the party. I noticed that in commit 6190d828cd the > comment of init_dummy_sjinfo() mentions the non-existing 'rel1' and > 'rel2', which should be addressed. Thanks for catching that. Oops. > Also, I think we can make function > consider_new_or_clause() call init_dummy_sjinfo() to simplify the code a > bit. Indeed. > Attached is a trivial patch for the fixes. Will apply shortly. -- Thanks, Amit Langote
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
On Fri, Mar 22, 2024 at 7:48 PM Ashutosh Bapat wrote: > With the later code, semi_rhs_expr is indeed free'able. It wasn't so when I > wrote the patches. Thanks for updating the comment. I wish we would have > freeObject(). Nothing that can be done for this patch. Yes. > Attached patches > Squashed your 0001 and 0002 into 0001. I have also reworded the commit > message to reflect the latest code changes. > 0002 has some minor edits. Please feel free to include or reject when > committing the patch. I've pushed this now. When updating the commit message, I realized that you had made the right call to divide the the changes around not translating the dummy SpecialJoinInfos into a separate patch. So, I pushed it like that. Thanks for working on this. -- Thanks, Amit Langote
Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning
Hi Ashutosh, On Tue, Mar 19, 2024 at 12:47 AM Ashutosh Bapat wrote: > On Mon, Mar 18, 2024 at 5:40 PM Amit Langote wrote: >> >> >> >> Sorry, I should’ve mentioned that I was interested in seeing cpu times to >> >> compare the two approaches. Specifically, to see if the palloc / frees >> >> add noticeable overhead. >> > >> > No problem. Here you go >> > >> > tables | master | patched | perc_change >> > +--+--+- >> > 2 | 477.87 | 492.32 | -3.02 >> > 3 | 1970.83 | 1989.52 | -0.95 >> > 4 | 6305.01 | 6268.81 |0.57 >> > 5 | 19261.56 | 18684.86 |2.99 >> > >> > +ve change indicates reduced planning time. It seems that the planning >> > time improves as the number of tables increases. But all the numbers are >> > well within noise levels and thus may not show any improvement or >> > deterioration. If you want, I can repeat the experiment. >> >> Thanks. I also wanted to see cpu time difference between the two >> approaches of SpecialJoinInfo allocation -- on stack and on heap. So >> comparing times with the previous version of the patch using stack >> allocation and the new one with palloc. I'm hoping that the new >> approach is only worse in the noise range. > > > Ah, sorry, I didn't read it carefully. Alvaro made me realise that I have > been gathering numbers using assert enabled builds, so they are not that > reliable. Here they are with non-assert enabled builds. > > planning time (in ms) reported by EXPLAIN. > tables | master | stack_alloc | perc_change_1 | palloc | perc_change_2 | > total_perc_change > +--+-+---+--+---+--- > 2 |338.1 | 333.92 | 1.24 | 332.16 | 0.53 | > 1.76 > 3 | 1507.93 | 1475.76 | 2.13 | 1472.79 | 0.20 | > 2.33 > 4 | 5099.45 | 4980.35 | 2.34 | 4947.3 | 0.66 | > 2.98 > 5 | 15442.64 |15531.94 | -0.58 | 15393.41 | 0.89 | > 0.32 > > stack_alloc = timings with SpecialJoinInfo on stack > perc_change_1 = percentage change of above wrt master > palloc = timings with palloc'ed SpecialJoinInfo > perc_change_2 = percentage change of above wrt timings with stack_alloc > total_perc_change = percentage change between master and all patches > > total change is within noise. Timing with palloc is better than that with > SpecialJoinInfo on stack but only marginally. I don't think that means palloc > based allocation is better or worse than stack based. > > You requested CPU time difference between stack based SpecialJoinInfo and > palloc'ed SpecialJoinInfo. Here it is > tables | stack_alloc | palloc | perc_change > +-+--+- > 2 |0.438204 | 0.438986 | -0.18 > 3 |1.224672 | 1.238781 | -1.15 > 4 |3.511317 | 3.663334 | -4.33 > 5 |9.283856 | 9.340516 | -0.61 > > Yes, there's a consistent degradation albeit within noise levels. > > The memory measurements > tables | master | with_patch > ++ > 2 | 26 MB | 26 MB > 3 | 93 MB | 84 MB > 4 | 277 MB | 235 MB > 5 | 954 MB | 724 MB > > The first row shows the same value because of rounding. The actual values > there are 27740432 and 26854704 resp. > > Please let me know if you need anything. Thanks for repeating the benchmark. So I don't see a problem with keeping the existing palloc() way of allocating the SpecialJoinInfos(). We're adding a few cycles by adding free_child_join_sjinfo() (see my delta patch about the rename), but the reduction in memory consumption due to it, which is our goal here, far outweighs what looks like a minor CPU slowdown. I have squashed 0002, 0003 into 0001, done some changes of my own, and attached the delta patch as 0002. I've listed the changes in the commit message. Let me know if anything seems amiss. I'd like to commit 0001 + 0002 + any other changes based on your comments (if any) sometime early next week. -- Thanks, Amit Langote From 9d8c408b3379faab6269421f67d75530f1c4cde2 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Fri, 22 Mar 2024 16:51:48 +0900 Subject: [PATCH v1 2/2] Some fixes * Revert (IMO) unnecessary modifications of build_child_join_sjinfo(). For example, hunks to rename sjinfo to child_sjinfo seem unnecessary, because it's clear from the context that they are &q
