Re: SQL/JSON features for v15

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

> 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