On Thu, 26 May 2022 at 10:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Maybe a better fix is to add a new Bitmapset field to WindowClause and
> > have find_window_run_conditions() record the attno in that field when
> > it appends the new runCondition to the runCondition field.
> > remove_unused_subquery_outputs() can just bms_add_members that field
> > to attrs_used. This just means having to add a field to WindowClause
> > post-beta. Is that going to be a problem?
>
> It'd mean a forced initdb, which is not great, but unless 0ff20288e
> gets reverted there'd be no additional impact on beta testers.
I'm partially surprised that we'd consider rolling that back to what
it was before that commit if it was reverted. I didn't know that was
the policy. Likely it's less painful for hackers to deal with than all
beta testers.
> A bigger problem with what you describe is that I don't really think
> the planner should be mucking with the input parse tree like that.
> Can't we retain this information somewhere else instead, in storage
> associated with the PlannerInfo?
Yeah, I'm definitely onboard with not messing around with the parse
when we don't have to. I just can't quite see how this could be done
for this case.
The problem is that the qual pushdown stuff all happens in
set_subquery_pathlist() before we call subquery_planner() for the
subquery. We don't yet have a PlannerInfo made for the subquery when
we call check_and_push_window_quals(). We don't really have any other
means to communicate to subquery_planner() what the run conditions are
for the given Query object. Plus, we *do* really need to know what the
runConditions are before we call subquery_planner() so that those
conditions can be properly tagged onto WindowAggPaths. I don't really
think it would be right to pluck those from the PlannerInfo when we
later call create_plan(). That wouldn't leave us any opportunity to do
any costing related stuff with run conditions if we decide to do that
later.
The best way I can think to do that is to have some supplementary
struct that we can pass to subquery_planner() like QueryInfo that
contains additional info we've learned about the query. Having
something like that might help us get away from other places where we
modify the parse, such as adding inheritance RTEs for partitioned
tables. However, that does not feel like a topic for PG15.
David