Fujita-san, On 2019/01/11 20:04, Etsuro Fujita wrote: > (2019/01/11 13:46), Amit Langote wrote: >> On 2019/01/10 15:07, Etsuro Fujita wrote: >>> (The name of the flag isn't >>> good? If so, that would be my fault because I named that flag.) >> >> If it's really just to store the fact that the relation's targetlist >> contains expressions that partitionwise join currently cannot handle, then >> setting it like this in set_append_rel_size seems a bit misleading: >> >> if (enable_partitionwise_join&& >> rel->reloptkind == RELOPT_BASEREL&& >> rte->relkind == RELKIND_PARTITIONED_TABLE&& >> rel->attr_needed[InvalidAttrNumber - rel->min_attr] == NULL) >> rel->consider_partitionwise_join = true; >> >> Sorry, I wasn't around to comment on the patch which got committed in >> 7cfdc77023a, but checking the value of enable_partitionwise_join and other >> things in set_append_rel_size() to set the value of >> consider_partitionwise_join seems a bit odd to me. Perhaps, >> consider_partitionwise_join should be initially set to true for a relation >> (actually, to rel->part_scheme != NULL) and only set it to false if the >> relation's targetlist is found to contain unsupported expressions. > > One thing I intended in that commit was to set the flag to false for > partitioned tables contained in inheritance trees where the top parent is > a UNION ALL subquery, because we don't consider PWJ for those tables. > Actually we wouldn't need to care about that, because we don't do PWJ for > those tables regardless of what the flag is set, but I think that would > make the code a bit cleaner.
Yeah, we wouldn't do partitionwise join between partitioned tables that are under UNION ALL. > However, what you proposed here as-is would > not keep that behavior, because rel->part_scheme is created for those > tables as well It'd be easy to prevent set consider_partitionwise_join to false in that case as: + rel->consider_partitionwise_join = (rel->part_scheme != NULL && + (parent == NULL || + parent->rtekind != RTE_SUBQUERY)); > (even though there would be no need IIUC). Partition pruning uses part_scheme and pruning can occur even if a partitioned table is under UNION ALL, so it *is* needed in that case. >> I think >> enable_partitionwise_join should only be checked in relnode.c or >> joinrels.c. > > Sorry, I don't understand this. What I was trying to say is that we should check the GUC close to where partitionwise join is actually implemented even though there is no such hard and fast rule. Or maybe I'm just a bit uncomfortable with setting consider_partitionwise_join based on the GUC. >> I've attached a patch to show what I mean. Can you please >> take a look? > > Thanks for the patch! Maybe I'm missing something, but I don't have a > strong opinion about that change. I'd rather think to modify > build_simple_rel so that it doesn't create rel->part_scheme if unnecessary > (ie, partitioned tables contained in inheritance trees where the top > parent is a UNION ALL subquery). As I said above, partition pruning can occur even if a partitioned table happens to be under UNION ALL. However, we *can* avoid creating part_scheme and setting other partitioning properties if *all* of enable_partition_pruning, enable_partitionwise_join, and enable_partitionwise_aggregate are turned off. >> If you think that this patch is a good idea, then you'll need to >> explicitly set consider_partitionwise_join to false for a dummy partition >> rel in set_append_rel_size(), because the assumption of your patch that >> such partition's rel's consider_partitionwise_join would be false (as >> initialized with the current code) would be broken by my patch. But that >> might be a good thing to do anyway as it will document the special case >> usage of consider_partitionwise_join variable more explicitly, assuming >> you'll be adding a comment describing why it's being set to false >> explicitly. > > I'm not sure we need this as part of a fix for the issue reported on this > thread. I don't object to what you proposed here, but that would be > rather an improvement, so I think we should leave that for another patch. Sure, no problem with committing it separately if at all. Thanks for considering. Thanks, Amit
