Thread: read-only planner input
I've been taking a look at how to stop the planner from scribbling on
its input. This is my first modification of any significance to the
planner, so don't hesitate to tell me what I've gotten wrong :)
I think the planner makes two kinds of modifications to the input Query:
(a) rewriting of the Query to improve planning (b) as a convenient place
to store planner working state. Some examples of the former include
transforming IN clauses to joins, transforming simple FROM-clause
subselects into joins, preprocessing expressions, and so forth. Examples
of the latter are mostly the "internal to planner" fields denoted in the
Query struct definition.
(b) should be pretty easy to solve; we can create a per-Query PlanState
struct that contains this information, as well as holding a pointer to
the Query (and perhaps the in-construct Plan tree).
I'm still trying to figure out how to handle (a). Perhaps we can create
an additional plan node that always sits at the top of the plan tree.
This would hold derivations of data from the input Query. A lot of the
code that implements (a) is actually already applicative in nature, but
any code that modifies a Query destructively would need to be changed.
In other words, rather than
query->jointree = pull_up_subqueries(parse, query->jointree);
We'd have:
top_plan_node->jointree = pull_up_subqueries(plan_state,
query->jointree);
(Possibly passing PlanState rather than `parse', which is a Query, if
needed. The example is also somewhat simplified.)
BTW, I wonder whether it would be possible to move some preprocessing
from the early stages of the planner to a "preprocessing" phase that
would run after the rewriter but before the planner proper. The
preprocessor would maintain the essential properties of the input Query,
but it wouldn't need to be re-run when the query is replanned due to a
modification to a dependent database object. For example, the decision
about whether to pull-up a subquery could be done once and not redone in
subsequent invocations of the planner on the same Query. On the other
hand, I'm not sure how much preprocessing could be rearranged like this,
and since replanning ought to be relatively rare, I'm not sure it's
worth spending a whole lot of time trying to optimize it...
Comments welcome.
-Neil
Neil Conway wrote: > (b) should be pretty easy to solve; we can create a per-Query PlanState > struct that contains this information, as well as holding a pointer to > the Query (and perhaps the in-construct Plan tree). I just noticed that there is a `PlanState' node in the executor, of all places. I'm thinking of using `QueryState' instead -- this parallels the usage of PlanState in the executor, to some degree (PlanState holds some of the state of the executor as it examines a Plan; QueryState holds some of the state of the planner as it examines a Query). I also considered `PlannerState', but that doesn't seem best, as there can be multiple instances of this struct in existence for a single invocation of planner() (due to subqueries). I also created a new header file, `plan_internals.h' for the declaration of this struct, and moved a few planner-internal declarations from planner.h to plan_internals.h -- I think it would be best to only define external interfaces in planner.h -Neil
"Neil Conway" <neilc@samurai.com> > I've been taking a look at how to stop the planner from scribbling on > its input. This is my first modification of any significance to the > planner, so don't hesitate to tell me what I've gotten wrong :) > So is this change the preparation work of caching query plans? Like cleaning the plans so they could be well shared? Regards, Qingqing
Qingqing Zhou wrote: > So is this change the preparation work of caching query plans? Like cleaning > the plans so they could be well shared? Yeah, it is somewhat related to the centralized plan caching module that Tom and I have been discussing in the "cached plan invalidation" thread. When a cached plan has become invalidated, we want to re-run the planner on the original Query that has been saved away. If the planner modifies its input, that means we need to copy the Query it before passing it to the planner. While that is trivial to do, it is enough of an ugly hack that I decided to sit down and fix this problem before doing the rest of the cached plan invalidation work. Hopefully I haven't bitten off more than I can chew, so to speak :) -Neil
Neil Conway <neilc@samurai.com> writes:
> I just noticed that there is a `PlanState' node in the executor, of all
> places. I'm thinking of using `QueryState' instead -- this parallels the
> usage of PlanState in the executor, to some degree (PlanState holds some
> of the state of the executor as it examines a Plan; QueryState holds
> some of the state of the planner as it examines a Query). I also
> considered `PlannerState', but that doesn't seem best, as there can be
> multiple instances of this struct in existence for a single invocation
> of planner() (due to subqueries).
I'd go with PlannerState. QueryState for some reason sounds more like
execution-time state. I don't buy the notion that PlannerState sounds
like something there should be only one of; or at least, by the time
you've found out that the planner is potentially re-entrant, that should
not bother you ...
I'm not really sure what to do about the problem ofplan->foo = process(plan->foo)
... it's just *such* a convenient notation. Maybe it would be
sufficient to legislate that you can only do that when you know that you
are working with a copied "plan" object. The various recursive
"process" routines can be designed to return fresh structure whenever
they change anything --- for the most part we do that already, I think.
So this might just boil down to having to make a "flat" copy of the
top-level Query node before we start whacking at it.
Pulling the "planner internal" stuff out of the Query node does seem
like a good idea, even so.
regards, tom lane
Neil Conway <neilc@samurai.com> writes:
> BTW, I wonder whether it would be possible to move some preprocessing
> from the early stages of the planner to a "preprocessing" phase that
> would run after the rewriter but before the planner proper. The
> preprocessor would maintain the essential properties of the input Query,
> but it wouldn't need to be re-run when the query is replanned due to a
> modification to a dependent database object.
I don't believe there is any very significant amount of planner work
that is completely independent of any external database object. For
that matter, even the rewriter needs to be rerun when any views or
defaults change in the query. And for that matter, even the parse
analysis phase is dependent on external definitions. It's fairly likely
that the plan cache cannot safely use any upstream representation later
than the "raw parse tree" that's output by gram.y. You could make a
good case that we just ought to save query text and start from there in
any replanning; it'd be the most compact representation, the easiest to
copy around, and the least likely to break.
Which of course calls into question whether your current thoughts about
making the planner read-only are really going to advance the plan
caching project at all.
regards, tom lane
Tom Lane wrote: > I don't believe there is any very significant amount of planner work > that is completely independent of any external database object. For > that matter, even the rewriter needs to be rerun when any views or > defaults change in the query. And for that matter, even the parse > analysis phase is dependent on external definitions. It's fairly likely > that the plan cache cannot safely use any upstream representation later > than the "raw parse tree" that's output by gram.y. Hmm, I suppose that's true -- I had in mind that we would track dependencies with sufficient granularity that we would know when invoking each of those modules is necessary. For example, when a rule is added to the database that affects one of the dependent tables of a plan, we needn't rerun the parser or the analyzer. But on reflection I think you're right -- the scheme above doesn't really buy us much, and it is much simpler to just start with the query string or raw parsetree whenever we need to recreate an invalidate plan. Plus the above scheme might lead to some subtle bugs. > Which of course calls into question whether your current thoughts about > making the planner read-only are really going to advance the plan > caching project at all. True, but I've crossed the Rubicon already :) (Actually, I might stop after I've introduced the QueryState struct and moved planner-internal fields out of Query -- that will at least be a significant step closer to the goal.) -Neil
Tom Lane wrote: > You could make a > good case that we just ought to save query text and start from there in > any replanning; it'd be the most compact representation, the easiest to > copy around, and the least likely to break. What happens if (for example) DateStyle changes between the two parses? (not that I'm sure what the expected behaviour is in that case anyway..) -O
Oliver Jowett wrote: > What happens if (for example) DateStyle changes between the two parses? From my original email: This is the common case of a more general problem: a query plan depends on various parts of the environment at plan-creation time. That environment includes the definitions of database objects, but also GUC variables (most importantly search_path, but also optimizer-tuning variables for example), the state of database statistics, and so on. I'll leave resolution of the more general problem to someone else -- I think if we can manage to invalidate plans automatically when dependent objects change, that's better than nothing. I'm don't think recreating the plan from the query string changes this fundamentally -- the interaction between (for example) GUC variables and prepared plans will likely not be well-defined (which will be no worse than it is today). (It might be plausible to work something out where a backend-local SET of some subset of the GUC variables flushes all the cached query plans, but I'm don't have any plans to look at it myself.) -Neil
Neil Conway <neilc@samurai.com> writes:
> Oliver Jowett wrote:
>> What happens if (for example) DateStyle changes between the two parses?
> I'm don't think recreating the plan from the query string changes this
> fundamentally -- the interaction between (for example) GUC variables and
> prepared plans will likely not be well-defined (which will be no worse
> than it is today).
It is well defined, because we insist that the gram.y transformation not
depend on any changeable state. So if we forced replan after a change
of DateStyle, the "right thing" would happen with either plain text or
raw parsetrees as the starting point.
There is a separate issue here of course, which is whether it's really
the "right thing" --- if the plan got through parsing the first time
then it's pretty likely that it would fail under a different datestyle.
But certainly we've seen requests for cached plans to respond to changes
in search_path, and I doubt you can make a principled distinction
between that and datestyle.
From a practical point of view, I suspect the path of least resistance
is to use plain text as the saved representation, because (a) it's
smaller and (b) IIRC we don't currently have a complete set of copyfuncs
for raw parsetree nodes.
regards, tom lane
Tom Lane wrote: > It is well defined, because we insist that the gram.y transformation not > depend on any changeable state. That's my point -- whether we begin from the query string or the raw parsetree shouldn't make a difference. By not well-defined, I meant that if the user is changing GUC variables on the fly, they can't rely on their prepared query being planned under any particular datestyle (or search path, etc.), since they can't really predict when replanning will take place (e.g. an sinval overflow could occur spontaneously and cause all cached plans to be invalidated). This is similar to how search_path and pl/pgsql works right now -- we'll use the search_path in effect when the query is planned, which may or may not be what the user would expect. -Neil
Neil Conway <neilc@samurai.com> writes:
> ... By not well-defined, I meant that
> if the user is changing GUC variables on the fly, they can't rely on
> their prepared query being planned under any particular datestyle (or
> search path, etc.), since they can't really predict when replanning will
> take place (e.g. an sinval overflow could occur spontaneously and cause
> all cached plans to be invalidated). This is similar to how search_path
> and pl/pgsql works right now -- we'll use the search_path in effect when
> the query is planned, which may or may not be what the user would expect.
As soon as we have the replanning mechanism, I think that there will be
considerable pressure to use it to ensure that search_path changes and
so on *do* behave consistently.
The question here is what does "consistently" mean.
My first thought is that the best idea in most scenarios would be to
associate a search_path value with each function, rather than allowing
the meaning of queries within each function to vary depending on the
caller's search_path. We have seen one or two examples where the user
would rather have the other behavior --- but they are surely a small
minority.
I'm not sure though that I care to extend that thought to each
individual GUC variable ... even though it's surely true that changes
in e.g. datestyle could break a particular function. The contrary
argument is that GUC variables are useful just because they represent
a single place to set some behavior, and having to fix the values
associated with N different functions would be a nightmare outweighing
the benefits of stable values for individual functions.
Any thoughts on how this stuff "ought to" behave?
regards, tom lane
Tom Lane wrote: > I'd go with PlannerState. QueryState for some reason sounds more like > execution-time state. Well, not to me :) It just makes sense to me that QueryState as the working state associated with a Query. Not sure it makes a big difference, though. > Pulling the "planner internal" stuff out of the Query node does seem > like a good idea, even so. I've got most of this finished; I'll post a patch soon. One issue I ran into is how to handle query_tree_mutator() and query_tree_walker(): they both expect to be able to traverse a Query's in_info_list, which my patch moves into the QueryState struct. If maintaining this property is essential, it seems that we'll need a way to get the QueryState associated with a given Query. We can't just change the query tree walker to be a "query state walker", since we need to be able to recurse into subqueries, and the RTE for a subquery will only contain a Query, not its QueryState. Any thoughts on the best way to fix this? -Neil
Neil Conway <neilc@samurai.com> writes:
> I've got most of this finished; I'll post a patch soon. One issue I ran
> into is how to handle query_tree_mutator() and query_tree_walker(): they
> both expect to be able to traverse a Query's in_info_list, which my
> patch moves into the QueryState struct. If maintaining this property is
> essential, it seems that we'll need a way to get the QueryState
> associated with a given Query.
whereupon the entire premise of a read-only tree collapses ...
That's a bit nasty. I'm fairly sure that I added in_info_list to the
walker recursion because I had to; I don't recall the exact scenario,
but I think it needs to be possible to reassign relation numbers
within that data structure if we are doing it elsewhere in a query
tree.
regards, tom lane
Tom Lane wrote:
> That's a bit nasty. I'm fairly sure that I added in_info_list to the
> walker recursion because I had to; I don't recall the exact scenario,
> but I think it needs to be possible to reassign relation numbers
> within that data structure if we are doing it elsewhere in a query
> tree.
It was r1.125 of clauses.c, and yes, it seems pretty important. It's
used in adjust_inherited_attrs_mutator() in prep/prepunion.c,
flatten_join_alias_vars_mutator() in util/var.c, and three different
walkers in rewriteManip.c. That's from trawling through the original
(Jan of '03) patch -- it may have been used elsewhere subsequently.
Here's one idea to fix this: when planning a Query, transform the Query
into a "PlannedQuery". This would essentially be the same as the
QueryState we discussed earlier, except that we would also walk through
the Query and adjust references to nested Queries to refer to
PlannedQueries instead (so RTEs for subqueries would reference the
PlannedQuery, not the Query, for example). There would then be a
"planned query walker" that would walk both the original query and
additional planner-specific working state, and so on.
Perhaps we could use some trickery to avoid the PlannedQuery vs. Query
distinction when a particular piece of code doesn't care, by making
Query the first field of PlannedQuery. In other words:
struct PlannedQuery { Query q; /* other fields */
};
So we could treat a PlannedQuery * like a Query *. I don't really like
this solution.
Another possibility would be to punt, and keep in_info_list as part of
Query. We'd then need to resolve modifications to it in the same we way
will need to resolve modifications to legitimate parts of the Query
(e.g. by making an initial shallow copy and avoiding destructive
updates, per earlier discussion).
-Neil
Neil Conway <neilc@samurai.com> writes:
> Here's one idea to fix this: when planning a Query, transform the Query
> into a "PlannedQuery". This would essentially be the same as the
> QueryState we discussed earlier, except that we would also walk through
> the Query and adjust references to nested Queries to refer to
> PlannedQueries instead (so RTEs for subqueries would reference the
> PlannedQuery, not the Query, for example). There would then be a
> "planned query walker" that would walk both the original query and
> additional planner-specific working state, and so on.
> Perhaps we could use some trickery to avoid the PlannedQuery vs. Query
> distinction when a particular piece of code doesn't care, by making
> Query the first field of PlannedQuery. In other words:
> struct PlannedQuery {
> Query q;
> /* other fields */
> };
> So we could treat a PlannedQuery * like a Query *. I don't really like
> this solution.
No. At that point you've essentially booted away the entire point of
the change :-(
IIRC one of the main reasons for wanting to make the planner read-only
is so that it does *not* modify subquery RTE contents --- there are all
sorts of uglinesses involved in the fact that it presently does, mainly
having to be sure that we plan each subquery exactly once. If we go
this route then we won't be able to fix any of that stuff.
> Another possibility would be to punt, and keep in_info_list as part of
> Query.
That's seeming like the path of least resistance at the moment ... but
it still isn't going to solve the subquery RTE issues. I'm feeling a
bit discouraged about this concept right now ... maybe we need to back
off and think about a fresh start.
regards, tom lane