Re: remaining sql/json patches

On Wed, Oct 11, 2023 at 2:08 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Sat, Oct 7, 2023 at 6:49 AM Andres Freund <andres@anarazel.de> wrote:
> > On 2023-09-29 13:57:46 +0900, Amit Langote wrote:
> > > Thanks.  I will push the attached 0001 shortly.
> >
> > Sorry for not looking at this earlier.
>
> Thanks for the review.  Replying here only to your comments on 0001.
>
> > Have you done benchmarking to verify that 0001 does not cause performance
> > regressions? I'd not be suprised if it did.
>
> I found that it indeed did once I benchmarked with something that
> would stress EEOP_IOCOERCE:
>
> do $$
> begin
> for i in 1..20000000 loop
> i := i::text;
> end loop; end; $$ language plpgsql;
> DO
>
> Times and perf report:
>
> HEAD:
>
> Time: 1815.824 ms (00:01.816)
> Time: 1818.980 ms (00:01.819)
> Time: 1695.555 ms (00:01.696)
> Time: 1762.022 ms (00:01.762)
>
>                 --97.49%--exec_stmts
>                           |
>                           --85.97%--exec_assign_expr
>                                     |
>                                     |--65.56%--exec_eval_expr
>                                     |          |
>                                     |          |--53.71%--ExecInterpExpr
>                                     |          |          |
>                                     |          |          |--14.14%--textin
>
>
> Patched:
>
> Time: 1872.469 ms (00:01.872)
> Time: 1927.371 ms (00:01.927)
> Time: 1910.126 ms (00:01.910)
> Time: 1948.322 ms (00:01.948)
>
>                 --97.70%--exec_stmts
>                           |
>                           --88.13%--exec_assign_expr
>                                     |
>                                     |--73.27%--exec_eval_expr
>                                     |          |
>                                     |          |--58.29%--ExecInterpExpr
>                                     |          |          |
>                                     |          |
> |--25.69%--InputFunctionCallSafe
>                                     |          |          |          |
>                                     |          |          |
> |--14.75%--textin
>
> So, yes, putting InputFunctionCallSafe() in the common path may not
> have been such a good idea.
>
> > I'd split the soft-error path into
> > a separate opcode. For JIT it can largely be implemented using the same code,
> > eliding the check if it's the non-soft path. Or you can just put it into an
> > out-of-line function.
>
> Do you mean putting the execExprInterp.c code for the soft-error path
> (with a new opcode) into an out-of-line function?  That definitely
> makes the JIT version a tad simpler than if the error-checking is done
> in-line.
>
> So, the existing code for EEOP_IOCOERCE in both execExprInterp.c and
> llvmjit_expr.c will remain unchanged.  Also, I can write the code for
> the new opcode such that it doesn't use InputFunctionCallSafe() at
> runtime, but rather passes the ErrorSaveContext directly by putting
> that in the input function's FunctionCallInfo.context and checking
> SOFT_ERROR_OCCURRED() directly.  That will have less overhead.
>
> > I don't like adding more stuff to ExprState. This one seems particularly
> > awkward, because it might be used by more than one level in an expression
> > subtree, which means you really need to save/restore old values when
> > recursing.
>
> Hmm, I'd think that all levels will follow either soft or non-soft
> error mode, so sharing the ErrorSaveContext passed via ExprState
> doesn't look wrong to me.  IOW, there's only one value, not one for
> every level, so there doesn't appear to be any need to have the
> save/restore convention as we have for innermost_domainval et al.
>
> I can see your point that adding another 8 bytes at the end of
> ExprState might be undesirable.  Note though that ExprState.escontext
> is only accessed in the ExecInitExpr phase, but during evaluation.
>
> The alternative to not passing the ErrorSaveContext via ExprState is
> to add a new parameter to ExecInitExprRec() and to functions that call
> it.  The footprint would be much larger though.  Would you rather
> prefer that?
>
> > > @@ -1579,25 +1582,13 @@ ExecInitExprRec(Expr *node, ExprState *state,
> > >
> > >                               /* lookup the result type's input function */
> > >                               scratch.d.iocoerce.finfo_in = palloc0(sizeof(FmgrInfo));
> > > -                             scratch.d.iocoerce.fcinfo_data_in = palloc0(SizeForFunctionCallInfo(3));
> > > -
> > >                               getTypeInputInfo(iocoerce->resulttype,
> > > -                                                              &iofunc, &typioparam);
> > > +                                                              &iofunc, &scratch.d.iocoerce.typioparam);
> > >                               fmgr_info(iofunc, scratch.d.iocoerce.finfo_in);
> > >                               fmgr_info_set_expr((Node *) node, scratch.d.iocoerce.finfo_in);
> > > -                             InitFunctionCallInfoData(*scratch.d.iocoerce.fcinfo_data_in,
> > > -                                                                              scratch.d.iocoerce.finfo_in,
> > > -                                                                              3, InvalidOid, NULL, NULL);
> > >
> > > -                             /*
> > > -                              * We can preload the second and third arguments for the input
> > > -                              * function, since they're constants.
> > > -                              */
> > > -                             fcinfo_in = scratch.d.iocoerce.fcinfo_data_in;
> > > -                             fcinfo_in->args[1].value = ObjectIdGetDatum(typioparam);
> > > -                             fcinfo_in->args[1].isnull = false;
> > > -                             fcinfo_in->args[2].value = Int32GetDatum(-1);
> > > -                             fcinfo_in->args[2].isnull = false;
> > > +                             /* Use the ErrorSaveContext passed by the caller. */
> > > +                             scratch.d.iocoerce.escontext = state->escontext;
> > >
> > >                               ExprEvalPushStep(state, &scratch);
> > >                               break;
> >
> > I think it's likely that removing the optimization of not needing to set these
> > arguments ahead of time will result in a performance regression. Not to speak
> > of initializing the fcinfo from scratch on every evaluation of the expression.
>
> Yes, that's not good.  I agree with separating out the soft-error path.
>
> I'll post the patch and benchmarking results with the new patch shortly.

So here's 0001, rewritten to address the above comments.

It adds a new eval opcode EEOP_IOCOERCE_SAFE, which basically copies
the implementation of EEOP_IOCOERCE but passes the ErrorSaveContext
passed by the caller to the input function via the latter's
FunctionCallInfo.context.  However, unlike EEOP_IOCOERCE, it's
implemented in a separate function to encapsulate away the logic of
returning NULL when an error occurs.  This makes JITing much simpler,
because it now involves simply calling the function.

Here are the benchmark results:

Same DO block:

do $$
begin
for i in 1..20000000 loop
i := i::text;
end loop; end; $$ language plpgsql;

HEAD:
Time: 1629.461 ms (00:01.629)
Time: 1635.439 ms (00:01.635)
Time: 1634.432 ms (00:01.634)

Patched:
Time: 1657.657 ms (00:01.658)
Time: 1686.779 ms (00:01.687)
Time: 1626.985 ms (00:01.627)

Using the SQL/JSON query functions patch rebased over the new 0001, I
also compared the difference in performance between EEOP_IOCOERCE and
EEOP_IOCOERCE_SAFE:

-- uses EEOP_IOCOERCE because ERROR ON ERROR
do $$
begin
for i in 1..20000000 loop
i := JSON_VALUE(jsonb '1', '$' RETURNING text ERROR ON ERROR );
end loop; end; $$ language plpgsql;

-- uses EEOP_IOCOERCE because ERROR ON ERROR
do $$
begin
for i in 1..20000000 loop
i := JSON_VALUE(jsonb '1', '$' RETURNING text ERROR ON ERROR );
end loop; end; $$ language plpgsql;

Time: 2960.434 ms (00:02.960)
Time: 2968.895 ms (00:02.969)
Time: 3006.691 ms (00:03.007)

-- uses EEOP_IOCOERCE_SAFE because NULL ON ERROR
do $$
begin
for i in 1..20000000 loop
i := JSON_VALUE(jsonb '1', '$' RETURNING text NULL ON ERROR);
end loop; end; $$ language plpgsql;

Time: 3046.933 ms (00:03.047)
Time: 3073.385 ms (00:03.073)
Time: 3121.619 ms (00:03.122)

There's only a slight degradation with the SAFE variant presumably due
to the extra whether-error-occurred check after calling the input
function.  I'd think the difference would have been more pronounced
had I continued to use InputFunctionCallSafe().

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