Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables
От | Ashutosh Bapat |
---|---|
Тема | Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables |
Дата | |
Msg-id | CAFjFpRd=1venqLL7oGU=C1dEkuvk2DJgvF+7uKbnPHaum1mvHQ@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables (Robert Haas <robertmhaas@gmail.com>) |
Ответы |
Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables
(Robert Haas <robertmhaas@gmail.com>)
|
Список | pgsql-hackers |
On Thu, Sep 14, 2017 at 4:13 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Sep 13, 2017 at 12:56 PM, Ashutosh Bapat > <ashutosh.bapat@enterprisedb.com> wrote: >> I debugged what happens in case of query "select 1 from t1 union all >> select 2 from t1;" with the current HEAD (without multi-level >> expansion patch attached). It doesn't set partitioned_rels in Append >> path that gets converted into Append plan. Remember t1 is a >> multi-level partitioned table here with t1p1 as its immediate >> partition and t1p1p1 as partition of t1p1. So, the >> set_append_rel_pathlist() recurses once as shown in the following >> stack trace. > > Nice debugging. I spent some time today looking at this and I think > it's a bug in v10, and specifically in add_paths_to_append_rel(), > which only sets partitioned_rels correctly when the appendrel is a > partitioned rel, and not when it's a subquery RTE with one or more > partitioned queries beneath it. > > Attached are two patches either one of which will fix it. First, I > wrote mechanical-partrels-fix.patch, which just mechanically > propagates partitioned_rels lists from accumulated subpaths into the > list used to construct the parent (Merge)AppendPath. I wasn't entire > happy with that, because it ends up building multiple partitioned_rels > lists for the same RelOptInfo. That seems silly, but there's no > principled way to avoid it; avoiding it amounts to hoping that all the > paths for the same relation carry the same partitioned_rels list, > which is uncomfortable. > > So then I wrote pcinfo-for-subquery.patch. That patch notices when an > RTE_SUBQUERY appendrel is processed and accumulates the > partitioned_rels of its immediate children; in case there can be > multiple nested levels of subqueries before we get down to the actual > partitioned rel, it also adds a PartitionedChildRelInfo for the > subquery RTE, so that there's no need to walk the whole tree to build > the partitioned_rels list at higher levels, just the immediate > children. I find this fix a lot more satisfying. It adds less code > and does no extra work in the common case. Thanks a lot for the patch. I have included pcinfo-for-subquery.patch in my patchset as the first patch with typo corrections suggested by Amit Langote. > > Notice that the choice of fix we adopt has consequences for your > 0001-Multi-level-partitioned-table-expansion.patch -- with > mechanical-partrels-fix.patch, that patch could either associated all > partitioned_rels with the top-parent or it could work level by level > and everything would get properly assembled later. But with > pcinfo-for-subquery.patch, we need everything associated with the > top-parent. That doesn't seem like a problem to me, but it's > something to note. > I have few changes to multi-level expansion patch as per discussion in earlier mails 1. expand_single_inheritance_child() gets the top parent's PlanRowMark from which it builds the child's PlanRowMark and also update allMarkTypes of the top parent's PlanRowMark. The chlid's PlanRowMark contains the RTI of the top parent, which is pulled from the top parent's PlanRowMark. This is to keep the old behaviour intact. 2. Updated expand_single_inheritance_child's prologue to explain various output arguments, per suggestion from Amit Langote. Also included comments about the way we construct child PlanRowMark. Please see if the comments look good. 3. As suggested by Amit Langote, with multi-level partitioned table expansion, intermediate partitioned tables won't have pcinfo associated them. So, that patch removes the assertion Assert(list_length(partitioned_rels) >= 1) in add_paths_to_append_rels(). I didn't remove that assertion from your patch so that you could cherry-pick that commit to v10 where that assertion holds true. 4. Fixed inheritance_planner() to use top parent's RTE to pull partitioned_rels per discussion with Amit few mails back [1]. Please let me know if I have missed anything; it's been some long discussion. Apart from this I have included fix to reparameterize parallel nested loop paths as per discussion in [2]. Please note that I have removed the advanced partitioning patches from the attached patchset since those need a rebase because of default partition support. [1] https://www.postgresql.org/message-id/CAFjFpRe62H0rTb4Rb7wOVSR25xfNW+mt1Ncp-OtzGaEtZBTLwA@mail.gmail.com [2] https://www.postgresql.org/message-id/CAJ3gD9ctVgv6r0-7B6js7Z5uPHXx+KA5jK-3=uFsGwKOXfTddg@mail.gmail.com -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Ashutosh SharmaДата:
Сообщение: Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
Следующее
От: Alvaro HerreraДата:
Сообщение: Re: [HACKERS] DROP SUBSCRIPTION hangs if sub is disabled in the sametransaction