On Thu, Sep 7, 2017 at 7:16 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > There is a patch in the Ashutosh's posted series of patches, which does > more or less the same thing that this patch does. He included it in his > series of patches, because, IIUC, the main partitionwise-join planning > logic that one of the later patch implements depends on being able to > consider applying that new planning technique individually for every > partition sub-tree, instead of just at the whole tree root. > > One notable difference from his patch is that while his patch will expand > in non-flattened manner even in the case where the parent is the result > relation of a query, my patch doesn't in that case, because the new > partition-pruning technique cannot be applied to inheritance parent that > is a result relation, for example, > > update partitioned_table set ... > > And AFAICS, partitionwise-join cannot be applied to such a parent either. > > Note however that if there are other instances of the same partitioned > table (in the FROM list of an update statement) or other partitioned > tables in the query, they will be expanded in a non-flattened manner, > because they are themselves not the result relations of the query. So, > the new partition-pruning and (supposedly) partitionwise-join can be > applied for those other partitioned tables.
It seems to me that it would be better not to write new patches for things that already have patches without a really clear explanation with what's wrong with the already-existing patch; I don't see any such explanation here. Instead of writing your own patch for this to duel with his his, why not review his and help him correct any deficiencies which you can spot? Then we have one patch with more review instead of two patches with less review both of which I have to read and try to decide which is better. In this case, I think Ashutosh has the right idea. I think that handling the result-relation and non-result-relation differently creates an unpleasant asymmetry. With your patch, we have to deal with three cases: (a) partitioned tables that were expanded level-by-level because they are not result relations, (b) partitioned tables that were expanded "flattened" because they are result relations, and (c) non-partitioned tables that were expanded "flattened". With Ashutosh's approach, we only have two cases to worry about in the future rather than three, and I like that better. Your patch also appears to change things so that the table actually referenced in the query ends up with an AppendRelInfo for the parent, which seems pointless. And it has no tests. There are a couple of hunks from your patch that we might want or need to incorporate into Ashutosh's patch. The change to relation_excluded_by_constraints() looks like it might be useful, although it needs a better comment and some tests. Also, Ashutosh's patch has no equivalent of your change to add_paths_to_append_rel(). I'm not clear what the code you've introduced there is supposed to be doing, and I'm suspicious that it is confusing "partition root" with "table named in the query", which will often be the same but not always; the user could have named an intermediate partition. Can you expand on what this is doing? -- 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
