Re: Letting plpgsql in on the fun with the new expression eval stuff

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2017-12-20 12:12:48 -0500, Tom Lane wrote:
>> I'm using several different test cases, but one that shows up the problem
>> is [...]

> Which certainly seems interesting. The outer ExecInterpExpr() indeed
> doesn't do that much, it's the inner call that's the most relevant
> piece. That so much time is spent in SPI_fnumber() and the functions it
> calls, namely strcmp(), certainly doesn't seem right. I suspect that
> without addressing that cost, a lot of the other potential improvements
> aren't going to mean much.

Right, that's mostly coming from the fact that exec_eval_datum on
a RECFIELD does SPI_fnumber() every time. I have a patch in the
pipeline to address that, but this business with the expression eval
API is a separable issue (and it stands out a lot more with that
patch in place than it does in HEAD ;-)).

>> Um, you left out something here? I don't mind changing the callback
>> signature, but it seems like it generally ought to look the same as
>> the other out-of-line eval functions.

> Yes, I did. Intercontinental travel does wonders.

> I was thinking that it might be better not to expose the exact details
> of the expression evaluation step struct to plpgsql etc. I'm kinda
> forseeing a bit of further churn there that I don't think other parts of
> the code necessarily need to be affected by. We could have the callback
> have a signature roughly like
> Datum callback(void *private_data, ExprContext econtext, bool *isnull);

I don't especially like that, because it forces the callback to use a
separately allocated private_data area even when the available space
in the op step struct would serve perfectly well. In my draft patch
I have

--- 338,352 ----
Oid paramtype; /* OID of parameter's datatype */
} param;

+ /* for EEOP_PARAM_CALLBACK */
+ struct
+ {
+ ExecEvalSubroutine paramfunc; /* add-on evaluation subroutine */
+ void *paramarg; /* private data for same */
+ int paramid; /* numeric ID for parameter */
+ Oid paramtype; /* OID of parameter's datatype */
+ } cparam;
+
/* for EEOP_CASE_TESTVAL/DOMAIN_TESTVAL */
struct
{

and it turns out that plpgsql isn't bothering with paramarg because
paramid and paramtype are all it needs. I doubt that whatever you
have in mind to do to struct ExprEvalStep is likely to be so major
that it changes what we can keep in these fields.

>> I'm not seeing anything that needs to be reset?

> I was kind of thinking of the params_dirty business in
> plpgsql_param_fetch(), setup_param_list() etc. But I'm not quite sure
> how you're thinking of representing parameters on the plpgsql side after
> changing the callbacks style.

Turns out we can just get rid of that junk altogether. I've redesigned
the ParamListInfo API a bit in service of this, and there no longer seems
to be a need for plpgsql to use a mutable ParamListInfo at all.

Will send a patch in a bit. I need to write an explanation of what all
I changed.

regards, tom lane