Re: SQL/JSON features for v15 - Mailing list pgsql-hackers

 - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
 - Added immutability checks that were missed with elimination   of coercion expressions.   Coercions text::datetime, datetime1::datetime2 and even   datetime::text for some datetime types are mutable.   datetime::text can be made immutable by passing ISO date   style into output functions (like in jsonpath).
 - Disabled non-Const expressions in DEFAULT ON EMPTY in non   ERROR ON ERROR case.  Non-constant expressions are tried to   evaluate into Const directly inside transformExpr().

I am not sure if it's OK to eval_const_expressions() on a Query
sub-expression during parse-analysis.  IIUC, it is only correct to
apply it to after the rewriting phase.

I also was not sure. Maybe it can be moved to rewriting phase or
even to execution phase.

I suppose we wouldn't need to bother with doing this when we
eventually move to supporting the DEFAULT expressions.

   Maybe it would be better to simply remove DEFAULT ON EMPTY.

So +1 to this for now.

See last patch #9.


It is possible to easily split this patch into several subpatches,
I will do it if needed.

That would be nice indeed.

I have extracted patches #1-6 with numerous safe input and type conversion
functions.


I'm wondering if you're going to change the PASSING values
initialization to add the steps into the parent JsonExpr's ExprState,
like the previous patch was doing?

I forget to incorporate your changes for subsidary ExprStates elimination.
See patch #8.

Thanks.  Here are some comments.

First of all, regarding 0009, my understanding was that we should
disallow DEFAULT expression ON ERROR too for now, so something like
the following does not occur:

SELECT JSON_VALUE(jsonb '"err"', '$' RETURNING numeric DEFAULT ('{"'
|| -1+a || '"}')::text ON ERROR) from foo;
ERROR:  invalid input syntax for type numeric: "{"0"}"

Personally, I don't like complete removal of DEFAULT behaviors, but 
I've done it in patch #10 (JsonBehavior node removed, grammar fixed).

Patches 0001-0006:

On Tue, Aug 30, 2022 at 6:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Aug-30, Amit Langote wrote:

Patches 0001-0006:

Yeah, these add the overhead of an extra function call (typin() ->
typin_opt_error()) in possibly very common paths.  Other than
refactoring *all* places that call typin() to use the new API, the
only other option seems to be to leave the typin() functions alone and
duplicate their code in typin_opt_error() versions for all the types
that this patch cares about.  Though maybe, that's not necessarily a
better compromise than accepting the extra function call overhead.

I think another possibility is to create a static inline function in the
corresponding .c module (say boolin_impl() in bool.c), which is called
by both the opt_error variant as well as the regular one.  This would
avoid the duplicate code as well as the added function-call overhead.

+1

I always thought about such internal inline functions, I 've added them in v10.

Patch 0007:

+
+           /* Override default coercion in OMIT QUOTES case */
+           if (ExecJsonQueryNeedsIOCoercion(jexpr, res, *resnull))
+           {
+               char       *str = JsonbUnquote(DatumGetJsonbP(res));
...
+           else if (ret_typid == VARCHAROID || ret_typid == BPCHAROID ||
+                    ret_typid == BYTEAOID)
+           {
+               Jsonb      *jb = DatumGetJsonbP(res);
+               char       *str = JsonbToCString(NULL, &jb->root, VARSIZE(jb));
+
+               return ExecJsonStringCoercion(str, strlen(str),
ret_typid, ret_typmod);
+           }

I think it might be better to create ExecJsonQueryCoercion() similar
to ExecJsonValueCoercion() and put the above block in that function
rather than inlining it in ExecEvalJsonExprInternal().

Extracted ExecJsonQueryCoercion().

+ ExecJsonStringCoercion(const char *str, int32 len, Oid typid, int32 typmod)

I'd suggest renaming this one to ExecJsonConvertCStringToText().

+ ExecJsonCoercionToText(PGFunction outfunc, Datum value, Oid typid,
int32 typmod)
+ ExecJsonDatetimeCoercion(Datum val, Oid val_typid, Oid typid, int32 typmod,
+ ExecJsonBoolCoercion(bool val, Oid typid, int32 typmod, Datum *res)

And also rename these to sound like verbs:

ExecJsonCoerceToText
ExecJsonCoerceDatetime[ToType]
ExecJsonCoerceBool[ToType]

Fixed.

+   /*
+    * XXX coercion to text is done using output functions, and they
+    * are mutable for non-time[tz] types due to using of DateStyle.
+    * We can pass USE_ISO_DATES, which is used inside jsonpath, to
+    * make these coercions and JSON_VALUE(RETURNING text) immutable.
+    *
+    * XXX Also timestamp[tz] output functions can throw "out of range"
+    * error, but this error seem to be not possible.
+    */

Are we planning to fix these before committing?

I don't know,  but the first issue is critical for building functional indexes 
on JSON_VALUE().

+static Datum
+JsonbPGetTextDatum(Jsonb *jb)

Maybe static inline?

Fixed.

-           coercion = &coercions->composite;
-           res = JsonbPGetDatum(JsonbValueToJsonb(item));
+           Assert(0);  /* non-scalars must be rejected by JsonPathValue() */

I didn't notice any changes to JsonPathValue().  Is the new comment
referring to an existing behavior of JsonPathValue() or something that
must be done by the patch?

JsonPathValue() has a check for non-scalars items, this is simply a new comment.

@@ -411,6 +411,26 @@ contain_mutable_functions_walker(Node *node, void *context)    {        JsonExpr   *jexpr = castNode(JsonExpr, node);        Const      *cnst;
+       bool        returns_datetime;
+
+       /*
+        * Input fuctions for datetime types are stable.  They can be
+        * called in JSON_VALUE(), when the resulting SQL/JSON is a
+        * string.
+        */
...


Sorry if you've mentioned it before, but are these hunks changing
contain_mutable_functions_walker() fixing a bug?  That is, did the
original SQL/JSON patch miss doing this?

In the original patch there were checks for mutability of expressions contained 
in JsonCoercion nodes.  After their removal, we need to use hardcoded checks.

+   Oid         collation;      /* OID of collation, or InvalidOid if none */

I think the comment should rather say: /* Collation of <what>, ... */

Fixed.

+
+bool
+expr_can_throw_errors(Node *expr)
+{
+   if (!expr)
+       return false;
+
+   if (IsA(expr, Const))
+       return false;
+
+   /* TODO consider more cases */
+   return true;
+}

+extern bool expr_can_throw_errors(Node *expr);
+

Not used anymore.

expr_can_throw_errors() removed.

Patch 0008:

Thanks for re-introducing this.

+bool
+ExecEvalJsonExprSkip(ExprState *state, ExprEvalStep *op)
+{
+   JsonExprState *jsestate = op->d.jsonexpr_skip.jsestate;
+
+   /*
+    * Skip if either of the input expressions has turned out to be
+    * NULL, though do execute domain checks for NULLs, which are
+    * handled by the coercion step.
+    */

I think the part starting with ", though" is no longer necessary.

Fixed.

+ * Return value:
+ *   1 - Ok, jump to the end of JsonExpr
+ *   0 - empty result, need to execute DEFAULT ON EMPTY expression
+ *  -1 - error occured, need to execute DEFAULT ON ERROR expression

...need to execute ON EMPTY/ERROR behavior

+           return 0;   /* jump to ON EMPTY expression */
...
+           return -1;  /* jump to ON ERROR expression */

Likewise:

/* jump to handle ON EMPTY/ERROR behavior */

+                    * Jump to coercion step if true was returned,
+                    * which signifies skipping of JSON path evaluation,
...

Jump to "end" if true was returned.

Fixed, but I leaved "expression" instead of "behavior" because
these jumps are needed only for execution of DEFAULT expressions.



-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company