Re: WIP: Faster Expression Processing v4 - Mailing list pgsql-hackers

Hi,

On 2017-03-24 17:16:15 -0400, Tom Lane wrote:
> Attached is an updated patch.  I believe this is committable, but
> since I whacked it around quite a bit, I'm sure you'll want to look
> it over first.

Indeed.

Points:

- It's going to take a while for me to get used to execExprInterp.c - I constantly try to switch to a nonexistant
buffer;)
 

- Like your approach with /* Check that current tupdesc doesn't have more fields than we allocated */ in
ExecEvalFieldStoreDeForm().

- I like EEO_FLAG_IS_QUAL.

- I think at some point (not sure whether in your or in my revision) ScalarArrayOpExpr lost its permission check. Added
that.

- Added note that we knowingly don't perform permission checks for input/output funcs.

- Both pre/post patch, CoerceViaIO doesn't invoke InvokeFunctionExecuteHook - I'm still unclear what that hook is
usefulfor, so ...
 

- The !caseExpr->defresult result branch is currently unreachable (and its equivalent was before the patch) because
transformCaseExpr()generates a default expression.  I'm inclined to replace the dead code with an assertion. Any reason
notto do that?
 

- I see you kept determination of the set of constraints to be checked for domain at initialization time. Made note
thatthat a) might change b) could change behaviour.
 


> Please be sure the commit message notes that function EXECUTE permissions
> are now checked at executor startup not first call.  We need to document
> that in the v10 release notes as an incompatibility, and I'm sure we'll
> forget if the commit log doesn't point it out.

Done.


> * As we discussed, ExecEvalWholeRowVar is now using a pretty inefficient
> method for flattening tuples into datums.  I will take a to-do item to
> fix this.

Thanks.


> * ExecInitCheck is really just ExecInitExpr with a make_ands_explicit
> call in front of it.  It turned out that most of the places that were
> (or should have been) calling it were doing a make_ands_implicit call
> for no other reason than to satisfy its API.  I changed most of those
> to just call ExecInitExpr directly.  There are just a couple of call
> sites left, and I think probably those are likewise not really that
> happy with this API --- but I didn't take the time to chase down where
> the expressions were coming from in those cases.  It seems possible
> that we could remove ExecInitCheck/ExecPrepareCheck entirely.  I'll
> look into this later, too.

Thanks^2.


> * As we discussed, CaseTestValue/DomainValue are pretty messy and need
> to be rethought.  That might not get done for v10 though.

Yea, I'm disinclined to tackle this just now, there's enough other stuff
pending - but I'm willing to tackle it for v11 unless you beat me to it.


> * I'm not very happy that execSRF.c has two somewhat different
> implementations of largely similar functionality, and
> SetExprState.elidedFuncState seems like a wart.  That's mostly a
> pre-existing problem of course.  I'm satisfied with leaving it as it is
> for now, but eventually some refactoring there would be good.

Yea, that's not pretty.  Given the historical difference in behaviour
between SRF and ROWS FROM (the latter always materializes, the former
only when the set function does so itself), I'm not sure they can easily
be simplified.

I'm not really sure how to get rid of the issue underlying
elidedFuncState?  I mean we could move that case into nodeFunctionscan,
above ExecMakeTableFunctionResult's level, but that doesn't really seem
like an improvement.


I think there's some argument to be made that we should move all of
execSRF.c's logic to their respective callsites.  There's not really
much shared code: ExecEvalFuncArgs(), tupledesc_match(), init_sexpr() -
and at least the latter would even become a bit simpler if we didn't
support both callers.


Thanks a lot for your work on the patch!

And pushed.

- Andres