On Fri, 8 Feb 2019 at 22:27, David Rowley <david.row...@2ndquadrant.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
