From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
Subject: | Re: Performance issue in foreign-key-aware join estimation |
Date: | 2019-05-25 04:37:54 |
Message-ID: | CAKJS1f_jLb=Z+-Szwk5shjjCpnZt8dUNH3qwHeSTexyi58MsWg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, 22 Mar 2019 at 10:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)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
Attachment | Content-Type | Size |
---|---|---|
eclass_indexes_v6.patch | application/octet-stream | 19.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Mark Kirkwood | 2019-05-25 04:48:24 | Re: Zedstore - compressed in-core columnar storage |
Previous Message | David Rowley | 2019-05-25 03:26:33 | Re: POC: converting Lists into arrays |