On Sat, 25 May 2019 at 16:37, David Rowley <david.rowley@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