Re: WIP Patch: Precalculate stable functions, infrastructure v1

As I said, thank you so much for your comments!!

On 17-01-2018 0:30, Tom Lane wrote:
> ...
> 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.

I'll try to use ParamListInfoData for generic plans (= to get cached
expressions for params of prepared statements where possible) without
changing its infrastructure.

> 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'm sorry, I did not know about this :-[

> 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.

I'll fix this.

> 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).

I'm sorry, I'll fix the use of parameters in this case. And I'll think
how to get rid of the need for PlannedStmt.hasCachedExpr when there're
possible cached expressions without parameters.

> 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.

I'll fix this.

> 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.

From quick look I see no contradictions so I'll try to implement it.

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

Such a stupid mistake :( I'll fix this.

> 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?

AFAIU they do not differ, and as I said above I'll try not to change the
infrastructure of ParamListInfoData.

> 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.

I'll try to improve it, for CheckBoundParams (if I understood you
correctly) and others.

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company