Re: SQL/JSON features for v15

On Tue, Aug 30, 2022 at 6:49 AM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> On 29.08.2022 15:56, Amit Langote wrote:
> On Sat, Aug 27, 2022 at 5:11 AM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> I have completed in v9 all the things I previously planned:
>
> BTW, maybe the following hunk in boolin_opt_error() is unnecessary?
>
> -   len = strlen(str);
> +   len -= str - in_str;
>
> This is really not necessary, but helps to avoid extra strlen() call.
> I have  replaced it with more intuitive
>
> +   {
>
>         str++;
> +       len--;
> +   }
>
> -   len = strlen(str);

+1

>  - 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"}"

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.

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().

+ 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]

+   /*
+    * 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?

+static Datum
+JsonbPGetTextDatum(Jsonb *jb)

Maybe static inline?

-           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?

@@ -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?

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

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

+
+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.

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.

+ * 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.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com