From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Letting plpgsql in on the fun with the new expression eval stuff
In-reply-to: <20171220174243.n4y3hgzf7xd3mm5e@alap3.anarazel.de>
References: <32589.1513706441@sss.pgh.pa.us>
 <20171220135602.4ixfvaqpeb6z7dil@alap3.anarazel.de>
 <27674.1513789968@sss.pgh.pa.us>
 <20171220174243.n4y3hgzf7xd3mm5e@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Wed, 20 Dec 2017 09:42:43 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <24879.1513793601.1@sss.pgh.pa.us>
Date: Wed, 20 Dec 2017 13:13:21 -0500
Message-ID: <24880.1513793601@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.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