On Fri, 22 Mar 2019 at 10:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <david.rowley@2ndquadrant.com> writes:
> > [ eclass_indexes_v4.patch ]
>
> I still don't like this approach too much. I think we can fairly easily
> construct the eclass_indexes at a higher level instead of trying to
> manage them in add_eq_member, and after some hacking I have the attached.
I've rebased this on top of the pgindent changes (attached) and looked over it.
The only problem I see is that you're not performing a bms_copy() of
the parent's eclass_indexes in add_child_rel_equivalences(). You've
written a comment that claims it's fine to just point to the parent's
in:
/*
* The child is now mentioned in all the same eclasses as its parent ---
* except for corner cases such as a volatile EC. But it's okay if
* eclass_indexes lists too many rels, so just borrow the parent's index
* set rather than making a new one.
*/
child_rel->eclass_indexes = parent_rel->eclass_indexes;
but that's not true since in get_eclass_for_sort_expr() we perform:
rel->eclass_indexes = bms_add_member(rel->eclass_indexes,
ec_index);
which will work fine about in about 63 out of 64 cases, but once
bms_add_member has to reallocate the set then it'll leave the child
rel's eclass_indexes pointing to freed memory. I'm not saying I have
any reproducible test case that can crash the backend from this, but
it does seem like a bug waiting to happen.
Apart from that, as far as I can tell, the patch seems fine.
I didn't add the bms_copy(). I'll wait for your comments before doing so.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services