On Sat, 25 May 2019 at 16:37, David Rowley <david.row...@2ndquadrant.com> wrote: > 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.
I've rebased this on top of the current master. d25ea0127 conflicted with the old version. I also tried to get the planner to crash by trying to find a case where a new eclass is added after setting the child relations eclass_indexes. I thought this could be done via a call to make_pathkey_from_sortinfo(), but I was unable to find any case that passes create_it as true. I added some code to emit a warning when this happens after a call to add_child_rel_equivalences() and found that the warning wasn't raised after running make check. However, I'm still pretty scared by not making a copy of the Bitmapset. It seems like if anything ever changed in this area then we could end up with some very rare crashes if the parent's eclass_indexes grew another bitmapword since the child took it's copy of them, so I added the bms_copy() in the attached version. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
eclass_indexes_v7.patch
Description: Binary data