Re: speeding up planning with partitions - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: speeding up planning with partitions |
Date | |
Msg-id | 23831.1553873385@sss.pgh.pa.us Whole thread Raw |
In response to | Re: speeding up planning with partitions (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Responses |
Re: speeding up planning with partitions
Re: speeding up planning with partitions |
List | pgsql-hackers |
Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes: > Here are some comments on v38. Thanks for looking it over! I'll just reply to points worth discussing: > - Assert(rte->rtekind == RTE_RELATION || > - rte->rtekind == RTE_SUBQUERY); > - add_appendrel_other_rels(root, rel, rti); > + if (rte->rtekind == RTE_RELATION) > + expand_inherited_rtentry(root, rel, rte, rti); > + else > + expand_appendrel_subquery(root, rel, rte, rti); > Wouldn't it be a good idea to keep the Assert? There's an Assert in expand_appendrel_subquery that what it got is an RTE_SUBQUERY, so I thought the one at the call site was redundant. I suppose another way to do this would be like if (rte->rtekind == RTE_RELATION) expand_inherited_rtentry(root, rel, rte, rti); else if (rte->rtekind == RTE_SUBQUERY) expand_appendrel_subquery(root, rel, rte, rti); else Assert(false); Not sure if that's better or not. Or we could go back to the design of just having one function and letting it dispatch the case it doesn't want to the other function --- though I think I'd make expand_inherited_rtentry be the primary function, rather than the other way around as you had it in v37. > + forboth(lc, old_child_rtis, lc2, new_child_rtis) > + { > + int old_child_rti = lfirst_int(lc); > + int new_child_rti = lfirst_int(lc2); > + > + if (old_child_rti == new_child_rti) > + continue; /* nothing to do */ > + > + Assert(old_child_rti > new_child_rti); > + > + ChangeVarNodes((Node *) child_appinfos, > + old_child_rti, new_child_rti, 0); > + } > This seems necessary? RTEs of children of the target table should be in > the same position in the final_rtable as they are in the select_rtable. Well, that's what I'm not very convinced of. I have observed that the regression tests don't reach this ChangeVarNodes call, but I think that might just be lack of test cases rather than a proof that it's impossible. The question is whether it'd ever be possible for the update/delete target to not be the first "inh" table that gets expanded. Since that expansion is done in RTE order, it reduces to "is the target always before any other RTE entries that could need inheritance expansion?" Certainly that would typically be true, but I don't feel very comfortable about assuming that it must be true, when you start thinking about things like updatable views, rules, WITH queries, and so on. It might be worth trying to devise a test case that does reach this code. If we could convince ourselves that it's really impossible, I'd be willing to drop it in favor of putting a test-and-elog check in the earlier loop that the RTI pairs are all equal. But I'm not willing to do it without more investigation. > + /* XXX wrong? */ > + parentrte->inh = false; > About the XXX: I think resetting inh flag is unnecessary, so we should > just remove the line. Possibly. I hadn't had time to follow up the XXX annotation. > If we do that, we can also get rid of the following > code in set_rel_size(): > else if (rte->relkind == RELKIND_PARTITIONED_TABLE) > { > /* > * A partitioned table without any partitions is marked as > * a dummy rel. > */ > set_dummy_rel_pathlist(rel); > } Not following? Surely we need to mark the childless parent as dummy at some point, and that seems like as good a place as any. > Finally, it's not in the patch, but how about visiting > get_relation_constraints() for revising this block of code: That seems like probably an independent patch --- do you want to write it? regards, tom lane
pgsql-hackers by date: