On Mon, 22 Jul 2019 at 08:01, Tom Lane <t...@sss.pgh.pa.us> wrote: > > I wrote: > >> * Rationalize places that are using combinations of list_copy and > >> list_concat, probably by inventing an additional list-concatenation > >> primitive that modifies neither input. > > > I poked around to see what we have in this department. There seem to > > be several identifiable use-cases: > > [ ... analysis ... ] > > Here's a proposed patch based on that. I added list_concat_copy() > and then simplified callers as appropriate.
I looked over this and only noted down one thing: In estimate_path_cost_size, can you explain why list_concat_copy() is needed here? I don't see remote_param_join_conds being used after this, so might it be better to just get rid of remote_param_join_conds and pass remote_conds to classifyConditions(), then just list_concat()? /* * The complete list of remote conditions includes everything from * baserestrictinfo plus any extra join_conds relevant to this * particular path. */ remote_conds = list_concat_copy(remote_param_join_conds, fpinfo->remote_conds); classifyConditions() seems to create new lists, so it does not appear that you have to worry about modifying the existing lists. > It turns out there are a *lot* of places where list_concat() callers > are now leaking the second input list (where before they just leaked > that list's header). So I've got mixed emotions about the choice not > to add a variant function that list_free's the second input. On the > other hand, the leakage probably amounts to nothing significant in > all or nearly all of these places, and I'm concerned about the > readability/understandability loss of having an extra version of > list_concat. Anybody have an opinion about that? In some of these places, for example, the calls to generate_join_implied_equalities_normal() and generate_join_implied_equalities_broken(), I wonder, since these are static functions if we could just change the function signature to accept a List to append to. This could save us from having to perform any additional pallocs at all, so there'd be no need to free anything or worry about any leaks. The performance of the code would be improved too. There may be other cases where we can do similar, but I wouldn't vote we change signatures of non-static functions for that. If we do end up with another function, it might be nice to stay away from using "concat" in the name. I think we might struggle if there are too many variations on concat and there's a risk we'll use the wrong one. If we need this then perhaps something like list_append_all() might be a better choice... I'm struggling to build a strong opinion on this though. (I know that because I've deleted this paragraph 3 times and started again, each time with a different opinion.) -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
