Re: WIP Patch: Precalculate stable functions, infrastructure v1
От | Tom Lane |
---|---|
Тема | Re: WIP Patch: Precalculate stable functions, infrastructure v1 |
Дата | |
Msg-id | 27837.1516138246@sss.pgh.pa.us обсуждение исходный текст |
Ответ на | Re: WIP Patch: Precalculate stable functions, infrastructure v1 (Aleksander Alekseev <a.alekseev@postgrespro.ru>) |
Ответы |
Re: WIP Patch: Precalculate stable functions, infrastructure v1
Re: WIP Patch: Precalculate stable functions, infrastructure v1 |
Список | pgsql-hackers |
Aleksander Alekseev <a.alekseev@postgrespro.ru> writes: > I can confirm this code works. However, since this is quite a large patch, I believe we better have a second reviewer ora 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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Peter EisentrautДата:
Сообщение: Re: [HACKERS] replace GrantObjectType with ObjectType
Следующее
От: Tom LaneДата:
Сообщение: Re: WIP Patch: Precalculate stable functions, infrastructure v1