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

Hi,

On 2017-03-15 20:09:03 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > [ new patches ]
>
> I've started to look at 0004, and the first conclusion I've come to
> is that it's *direly* short of documentation.  To the point that I'd
> vote against committing it if something isn't done about that.

Yea, I asked for input about what's hard to understand and what's not -
I stared at this for a *lot* of time, and it all kinda looks easy-ish
now.  I'm more than willing to expand on the below, and other pieces.


> As  an example, it's quite unclear how ExprEvalSteps acquire correct
> resnull/resvalue pointers, and the algorithm for that seems nontrivial.
> It doesn't help any that the arguments of ExecInitExprRec are entirely
> undocumented.

Generally whatever wants the result of a (sub-)expression passes in the
desired resvalue/resnull.  E.g. when doing a function call the
individual arguments are each prepared for evaluation using
ExecInitExprRec() and resvalue/resnull are pointing into fcinfo's
arg/nulls[i].


> I think it would be worth creating a README file giving an overview
> of how all of this patch is supposed to work.  You also need to do a
> whole lot more work on the function-level comments.

Ok.


> A specific thing I noticed in the particular area of
> what-gets-returned-where is this bit in EEOP_SCALARARRAYOP setup:
>
> +                /*
> +                 * Evaluate array argument into our return value, overwrite
> +                 * with comparison results afterwards.
> +                 */
> +                ExecInitExprRec((Expr *) lsecond(opexpr->args), parent, state,
> +                                resv, resnull);
>
> That scares me quite a bit, because it smells exactly like the sort of
> premature optimization that bit us on the rear in CVE-2016-5423 (cf commit
> f0c7b789a).

I don't think there's a danger similar to f0c7b789a here, because the
"caller" (i.e. the node that needs the expression's result) expects
resvalue/null to be overwritten. It'll e.g. be the value "slot" of one
arm (is there a better name for one part of a boolean expression?) of a
boolean expression.


> What's it really buying us to overwrite the return value
> early rather than storing into the fcinfo's second argument slot?

That'd work just as well.


> (The memory of that CVE is part of what's prompting me to demand a clear
> explanation of the algorithm for deciding where steps return their
> results.  Even if this particular code is safe, somebody is going to do
> something unsafe in future if there's not a documented rule to follow.)

I don't think there's a danger here, but I think you more generally have
a point.


> Another thing that ties into the do-I-understand-this-at-all question
> is this bit:
>
> +        EEO_CASE(EEOP_BOOL_AND_STEP_FIRST)
> +        {
> +            *op->d.boolexpr.anynull = false;
> +
> +            /*
> +             * Fallthrough (can't be last - ANDs have two arguments at least).
> +             */
> +        }
> +
> +        EEO_CASE(EEOP_BOOL_AND_STEP)
>
> It seems like this is missing an "op++;" before falling through.  If it
> isn't, because really BOOL_AND_STEP_FIRST is defined as clearing anynull
> and then also doing a regular BOOL_AND_STEP, then the comment seems rather
> off point.

It's intended to fall through this way, i.e. the difference between
_FIRST and not is just that only the former clears anynull.  What the
comment is about, admittedly too cryptically, is that the _LAST step
that then evaluates anynull cannot be the same step as
EEOP_BOOL_AND_STEP_FIRST, because bool AND/OR always has at least two
"arms".  Will expand / move.


> BTW, it sure seems like ExecInitExprRec and related code ought to set
> off all sorts of valgrind complaints?  It's not being careful at all
> to ensure that all fields of the "scratch" record get initialized before
> we memcpy it to someplace.

It worked not long ago - valgrind's replacment memcpy() doesn't trigger
undefined memory warnings, it just copies the "definedness" of each byte
(or bit?).  But your point gives me an idea: It seems like a good idea
to VALGRIND_MAKE_MEM_UNDEFINED() the "scratch" step at some convenient
places, so the definedness of individual operations is more useful.


Thanks for the look!


- Andres