Re: SQL/JSON features for v15

On Wed, Aug 31, 2022 at 6:25 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)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(at)alvh(dot)no-ip(dot)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