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