On Sat, Jun 30, 2018 at 5:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I also think that there's some horribly unsafe coding in
> apply_scanjoin_target_to_paths: it clobbers a RelOptInfo's reltarget,
> with no thought for whether that might affect things elsewhere,
> and it also clobbers individual paths' PathTargets like so:
>
> if (tlist_same_exprs)
> subpath->pathtarget->sortgrouprefs =
> scanjoin_target->sortgrouprefs;
>
> with similarly little regard for whether those PathTarget structures are
> shared with anything else.
Regarding this point specifically, I definitely was not thrilled about
that hack, but it was tricky to avoid having the tlist_same_exprs case
regress in terms of performance, so I didn't feel I could turn down
performance optimizations without some evidence of an actual problem.
For this to be a problem, it would have to be the case that somebody
builds a path, returns it, and then later mutates the sortgrouprefs
array in place. Does that happen? Off-hand I would have thought that
you'd have to apply the correct sortgroupref labeling to a path before
using it as a building block in some other path, just like it's not
really OK to change the cost of a path once you've passed it to
add_path() or add_partial_path(). Decisions might have already been
made based on the old information.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
