On Thu, Jul 13, 2017 at 1:09 PM, Amit Khandekar <amitdkhan...@gmail.com> wrote: > Attached is a WIP patch (make_resultrels_ordered.patch) that generates > the result rels in canonical order. This patch is kept separate from > the update-partition-key patch, and can be applied on master branch.
Hmm, I like the approach you've taken here in general, but I think it needs cleanup. +typedef struct ParentChild This is a pretty generic name. Pick something more specific and informative. +static List *append_rel_partition_oids(List *rel_list, Relation rel); One could be forgiven for thinking that this function was just going to append OIDs, but it actually appends ParentChild structures, so I think the name needs work. +List *append_rel_partition_oids(List *rel_list, Relation rel) Style. Please pgindent your patches. +#ifdef DEBUG_PRINT_OIDS + print_oids(*leaf_part_oids); +#endif I'd just rip out this debug stuff once you've got this working, but if we keep it, it certainly can't have a name as generic as print_oids() when it's actually doing something with a list of ParentChild structures. Also, it prints names, not OIDs. And DEBUG_PRINT_OIDS is no good for the same reasons. + if (RelationGetPartitionDesc(rel)) + walker->rels_list = append_rel_partition_oids(walker->rels_list, rel); Every place that calls append_rel_partition_oids guards that call with if (RelationGetPartitionDesc(...)). It seems to me that it would be simpler to remove those tests and instead just replace the Assert(partdesc) inside that function with if (!partdesc) return; Is there any real benefit in this "walker" interface? It looks to me like it might be simpler to just change things around so that it returns a list of OIDs, like find_all_inheritors, but generated differently. Then if you want bound-ordering rather than OID-ordering, you just do this: list_free(inhOids); inhOids = get_partition_oids_in_bound_order(rel); That'd remove the need for some if/then logic as you've currently got in get_next_child(). + is_partitioned_resultrel = + (oldrelation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE + && rti == parse->resultRelation); I suspect this isn't correct for a table that contains wCTEs, because there would in that case be multiple result relations. I think we should always expand in bound order rather than only when it's a result relation. I think for partition-wise join, we're going to want to do it this way for all relations in the query, or at least for all relations in the query that might possibly be able to participate in a partition-wise join. If there are multiple cases that are going to need this ordering, it's hard for me to accept the idea that it's worth the complexity of trying to keep track of when we expanded things in one order vs. another. There are other applications of having things in bound order too, like MergeAppend -> Append strength-reduction (which might not be legal anyway if there are list partitions with multiple, non-contiguous list bounds or if any NULL partition doesn't end up in the right place in the order, but there will be lots of cases where it can work). On another note, did you do anything about the suggestion Thomas made in http://postgr.es/m/CAEepm=3sc_j1zwqdyrbu4dtfx5rhcamnnuaxrkwzfgt9m23...@mail.gmail.com ? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
