Re: remaining sql/json patches

Hi Andres,

On Sat, Oct 7, 2023 at 6:49 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> Hi,
>
> 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.

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