Re: WIP Patch: Precalculate stable functions, infrastructure v1

Aleksander Alekseev <a(dot)alekseev(at)postgrespro(dot)ru> writes:
> I can confirm this code works. However, since this is quite a large patch, I believe we better have a second reviewer or a very attentive committer.
> The new status of this patch is: Ready for Committer

This is indeed quite a large patch, but it seems to me it could become
smaller. After a bit of review:

1. I do not like what you've done with ParamListInfo. The changes around
that are invasive, accounting for a noticeable part of the patch bulk,
and I don't think they're well designed. Having to cast back and forth
between ParamListInfo and ParamListInfoCommon and so on is ugly and prone
to cause (or hide) errors. And I don't really understand why
ParamListInfoPrecalculationData exists at all. Couldn't you have gotten
the same result with far fewer notational changes by defining another
PARAM_FLAG bit in the existing pflags field? (Or alternatively, maybe
the real need here is for another ParamKind value for Param nodes?)

I also dislike this approach because it effectively throws away the
support for "virtual" param arrays that I added in commit 6719b238e:
ParamListInfoPrecalculationData has no support for dynamically determined
parameter properties, which is surely something that somebody will need.
(It's just luck that the patch doesn't break plpgsql today.) I realize
that that's a recent commit and the code I'm complaining about predates
it, but we need to adjust this so that it fits in with the new approach.
See comment block at lines 25ff in params.h.

2. I don't follow the need for the also-rather-invasive changes to domain
constraint data structures. I do see that the patch attempts to make
CoerceToDomain nodes cacheable, which is flat wrong and has to be ripped
out. You *cannot* assume that the planner has access to the same domain
constraints that will apply at runtime.

I've occasionally thought that we should hook domain constraint changes
into the plan invalidation mechanism, which would make it possible for
the planner to assume that the constraints seen at planning time will
apply at execution. Whereupon we could have the planner insert domain
constraint expressions into the plan rather than leaving those to be
collected at query startup by execExpr.c, and then do things like
constant-folding and cacheing CoerceToDomain nodes. But that would be
a rather large and very domain-specific change, and so it would be fit
material for a different patch IMO. I recommend that for now you just
treat CoerceToDomain as an uncacheable expression type and rip all the
domain-related changes out of this patch.

3. I think you should also try hard to get rid of the need for
PlannedStmt.hasCachedExpr. AFAICS there's only one place that is
using that flag, which is exec_simple_check_plan, and I have to
think there are better ways we could deal with that. In particular,
I don't understand why you haven't simply set up plpgsql parameter
references to be noncacheable. Or maybe what we'd better do is
disable CacheExpr insertions into potentially-simple plans in the
first place. As you have it here, it's possible for recompilation
of an expression to result in a change in whether it should be deemed
simple or not, which will break things (cf commit 00418c612).

4. I don't like the way that you've inserted
"replace_qual_cached_expressions" and
"replace_pathtarget_cached_expressions" calls into seemingly random places
in the planner. Why isn't that being done uniformly during expression
preprocessing? There's no apparent structure to where you've put these
calls, and so they seem really vulnerable to errors of omission. Also,
if this were done in expression preprocessing, there'd be a chance of
combining it with some existing pass over expression trees instead of
having to do a separate (and expensive) expression tree mutation.
I can't help suspecting that eval_const_expressions could take this on
as an additional responsibility with a lot less than a thousand new lines
of code.

5. BTW, cost_eval_cacheable_expr seems like useless restructuring as well.
Why aren't you just recursively applying the regular costing function?

If you did all of the above it would result in a pretty significant
reduction of the number of places touched by the patch, which would make
it easier to see what's going on. Then we could start to discuss, for
instance, what does the "isConstParam" flag actually *mean* and why
is it different from PARAM_FLAG_CONST? And what in the world is
CheckBoundParams about? The internal documentation in this patch
isn't quite nonexistent, but it's well short of being in a
committable state IMO.

regards, tom lane