From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
Cc: | "Imai, Yoshikazu" <imai(dot)yoshikazu(at)jp(dot)fujitsu(dot)com>, Amit Langote <amitlangote09(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: speeding up planning with partitions |
Date: | 2019-02-08 12:22:26 |
Message-ID: | CAKJS1f-rV0G4ynF+6eTw2=SROrfezxLbECwz6TL8PbLHDBKfSw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, 8 Feb 2019 at 22:27, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> I had started looking at v20's 0001. I've not done a complete pass
> over it yet, but I'll likely just start again since v21 is out now:
I've now done a complete pass over v21.
Here's what I wrote down.
8. Is this code in the wrong patch? I don't see any function named
build_dummy_partition_rel in this patch.
* Make child entries in the EquivalenceClass as well. If the childrel
* appears to be a dummy one (one built by build_dummy_partition_rel()),
* no need to make any new entries, because anything that'd need those
* can instead use the parent's (rel).
*/
if (childrel->relid != rel->relid &&
9. "to use" seems out of place here. It makes more sense if you remove
those words.
* Add child subroots needed to use during planning for individual child
* targets
10. Is this comment really needed?
/*
* This is needed, because inheritance_make_rel_from_joinlist needs to
* translate root->join_info_list executing make_rel_from_joinlist for a
* given child.
*/
None of the other node types mention what they're used for. Seems
like something that might get outdated pretty quickly.
11. adjust_appendrel_attrs_mutator: This does not seem very robust:
/*
* No point in searching if parent rel not mentioned in eclass; but we
* can't tell that for sure if parent rel is itself a child.
*/
for (cnt = 0; cnt < nappinfos; cnt++)
{
if (bms_is_member(appinfos[cnt]->parent_relid, ec->ec_relids))
{
appinfo = appinfos[cnt];
break;
}
}
What if the caller passes multiple appinfos and actually wants them
all processed? You'll only process the first one you find that has an
eclass member. I think you should just loop over each appinfo and
process all the ones that have a match, not just the first.
I understand the current caller only passes 1, but I don't think that
gives you an excuse to take a shortcut on the implementation. I think
probably you've done this as that's what is done for Var in
adjust_appendrel_attrs_mutator(), but a Var can only belong to a
single relation. An EquivalenceClass can have members for multiple
relations.
13. adjust_appendrel_attrs_mutator: This seems wrong:
/*
* We have found and replaced the parent expression, so done
* with EC.
*/
break;
Surely there could be multiple members for the parent. Say:
UPDATE parted SET ... WHERE x = y AND y = 1;
14. adjust_appendrel_attrs_mutator: Comment is wrong. There's no
function called adjust_inherited_target_child_root and the EC is
copied in the function, not the caller.
/*
* Now fix up EC's relids set. It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/
15. I don't think "Copy it before modifying though." should be part of
this comment.
/*
* Adjust all_baserels to replace the original target relation with the
* child target relation. Copy it before modifying though.
*/
subroot->all_baserels = adjust_child_relids(subroot->all_baserels,
1, &appinfo);
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Raúl Marín Rodríguez | 2019-02-08 12:33:30 | Re: [PATCH] pgbench tap tests fail if the path contains a perl special character |
Previous Message | Daniel Gustafsson | 2019-02-08 11:37:48 | Re: Inconsistent error handling in the openssl init code |