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

On Wed, Aug 31, 2022 at 6:25 AM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> On 30.08.2022 11:09, Amit Langote wrote:
> 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).

To clarify, I had meant to ask if the standard specifies how to handle
the errors of evaluating the DEFAULT ON ERROR expressions themselves?
My understanding is that the sub-transaction that is being removed
would have caught and suppressed the above error too, so along with
removing the sub-transactions, we should also remove anything that
might cause such errors.

> On 30.08.2022 13:29, Amit Langote wrote:
> 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.

Thanks.

In 0003:

-Datum
-float4in(PG_FUNCTION_ARGS)
+static float
+float4in_internal(char *num, bool *have_error)

Looks like you forgot the inline marker?

In 0006:

-static inline Datum jsonb_from_cstring(char *json, int len, bool unique_keys);
...
+extern Datum jsonb_from_cstring(char *json, int len, bool unique_keys,
+                               bool *error);

Did you intentionally remove the inline marker from
jsonb_from_cstring() as opposed to the other cases?

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

Thanks.

+/* Coerce JSONB datum to the output typid(typmod) */
 static Datum
+ExecJsonQueryCoercion(JsonExpr *jexpr, Oid typid, int32 typmod,
+                     Datum jb, bool *error)

Might make sense to expand to comment to mention JSON_QUERY, say as:

/* Coerce JSONB datum returned by JSON_QUERY() to the output typid(typmod) */

+/* Coerce SQL/JSON item to the output typid */
+static Datum
+ExecJsonValueCoercion(JsonbValue *item, Oid typid, int32 typmod,
+                     bool *isnull, bool *error)

While at it, also update the comment of ExecJsonValueCoercion() as:

/* Coerce SQL/JSON item returned by JSON_VALUE() to the output typid */

> +   /*
> +    * 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().

Ok.

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

Ok.

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

Ah, okay, makes sense.   Though I do wonder why list the individual
type OIDs here rather than checking the mutability markings on their
input/output functions?  For example, we could do what the following
blob in check_funcs_in_node() that is called by
contain_mutable_functions_walker() does:

        case T_CoerceViaIO:
            {
                CoerceViaIO *expr = (CoerceViaIO *) node;
                Oid         iofunc;
                Oid         typioparam;
                bool        typisvarlena;

                /* check the result type's input function */
                getTypeInputInfo(expr->resulttype,
                                 &iofunc, &typioparam);
                if (checker(iofunc, context))
                    return true;
                /* check the input type's output function */
                getTypeOutputInfo(exprType((Node *) expr->arg),
                                  &iofunc, &typisvarlena);
                if (checker(iofunc, context))
                    return true;
            }

I guess that's what would get used when the JsonCoercion nodes were present.

On 0010:

@@ -5402,7 +5401,7 @@ ExecEvalJsonExprSkip(ExprState *state, ExprEvalStep *op)
  *   true  - Ok, jump to the end of JsonExpr
  *   false - error occured, need to execute DEFAULT ON ERROR expression
  */
-bool
+void

Looks like you forgot to update the comment.

 SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 111 ON ERROR);
- json_value
-------------
-        111
-(1 row)
-
+ERROR:  syntax error at or near "DEFAULT"
+LINE 1: ...ELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 11...

Is it intentional that you left many instances of the regression test
output changes like the above?

Finally, I get this warning:

execExprInterp.c: In function ‘ExecJsonCoerceCStringToText’:
execExprInterp.c:4765:3: warning: missing braces around initializer
[-Wmissing-braces]
   NameData encoding = {0};
   ^
execExprInterp.c:4765:3: warning: (near initialization for
‘encoding.data’) [-Wmissing-braces]


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