On Fri, 22 Mar 2019 at 10:10, Tom Lane <t...@sss.pgh.pa.us> wrote: > > David Rowley <david.row...@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
eclass_indexes_v6.patch
Description: Binary data
