On Thu, Sep 21, 2017 at 8:21 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: > Here's set of rebased patches. The patch with extra tests is not for > committing. All other patches, except the last one, will need to be > committed together. The last patch may be committed along with other > patches or as a separate patch.
In set_append_rel_size, is it necessary to set attr_needed = bms_copy(rel->attr_needed[index]) rather than just pointing to the existing value? If so, perhaps the comments should explain the reasons. I would have thought that the values wouldn't change after this point, in which case it might not be necessary to copy them. Regarding nomenclature and my previous griping about wisdom, I was wondering about just calling this a "partition join" like you have in the regression test. So the GUC would be enable_partition_join, you'd have generate_partition_join_paths(), etc. Basically just delete "wise" throughout. The elog(DEBUG3) in try_partition_wise_join() doesn't follow message style guidelines and I think should just be removed. It was useful for development, I'm sure, but it's time for it to go. + elog(ERROR, "unrecognized path node type %d", (int) nodeTag(path)); I think we should use the same formulation as elsewhere, namely "unrecognized node type: %d". And likewise probably "unexpected join type: %d". partition_join_extras.sql has a bunch of whitespace damage, although it doesn't really matter since, as you say, that's not for commit. (This is not a full review, just a few thoughts.) -- 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
