On Fri, Jun 22, 2018 at 6:58 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Fri, Jun 22, 2018 at 2:26 AM, Rajkumar Raghuwanshi > <rajkumar.raghuwan...@enterprisedb.com> wrote: > > I have applied patch and checked reported issue. Patch applied cleanly > and > > issues not reproducible any more. > > Committed, with a few changes: > Thanks Robert. > > - Renamed found_partially_grouped_child to partial_grouping_valid. > The old name seemed to me to be a poor choice, because it sounds from > the name like it gets set to true whenever we've found at least one > partially grouped child, whereas really it gets set to false whenever > we've failed to find at least one partially grouped child. The new > name is also more like the names in add_paths_to_append_rel. > > - Modified the wording of the comment. > > - Omitted the new assertion in add_paths_to_append_rel. I doubt > whether that's correct. I don't see any obvious reason why > live_childrels can't be NIL there, and further down I see this: > > /* > * If we found unparameterized paths for all children, build > an unordered, > * unparameterized Append path for the rel. (Note: this is > correct even > * if we have zero or one live subpath due to constraint > exclusion.) > */ > > If it's not possible to have no live_childrels, then that comment is > highly suspect. > > OK, do these comments also holds true for partial_subpaths? If we have NIL live_childrels, then the Assert() present inside the "if (partial_subpaths_valid)" block will hit. Which I think is wrong. We either need to remove these Asserts altogether or we should enter inside this block only when we have non-NIL partial_subpaths. Also, if we don't have any partial_subpaths, then variable partial_subpaths_valid should be false. Currently, by default, it is set to true due to which we are ending up inside that if block and then hitting an Assert. > Also, even if this assertion *is* correct, I think it needs a better > comment explaining why it's correct, because there isn't anything > obvious in set_append_rel_pathlist that keeps IS_DUMMY_REL() from > being true for every child. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Jeevan Chalke Technical Architect, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
