Re: POC: converting Lists into arrays

On Mon, 22 Jul 2019 at 08:01, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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