(2018/01/24 13:06), Amit Langote wrote: > On 2018/01/23 20:43, Etsuro Fujita wrote: >> Here is a comment for get_qual_for_list in partition.c: >> >> * get_qual_for_list >> * >> * Returns an implicit-AND list of expressions to use as a list partition's >> - * constraint, given the partition key and bound structures. >> >> I don't think the part "given the partition key and bound structures." >> is correct because we pass the *parent relation* and partition bound >> structure to that function. So I think we should change that part as >> such. get_qual_for_range has the same issue, so I think we need this >> change for that function as well. > > Yeah. It seems 6f6b99d1335 [1] changed their signatures to support > handling default partitions. > >> Another one I noticed in comments in partition.c is: >> >> * get_qual_for_hash >> * >> * Given a list of partition columns, modulus and remainder >> corresponding to a >> * partition, this function returns CHECK constraint expression Node for >> that >> * partition. >> >> I think the part "Given a list of partition columns, modulus and >> remainder corresponding to a partition" is wrong because we pass to that >> function the parent relation and partition bound structure the same way >> as for get_qual_for_list/get_qual_for_range. So what about changing the >> above to something like this, similarly to >> get_qual_for_list/get_qual_for_range: >> >> Returns a CHECK constraint expression to use as a hash partition's >> constraint, given the parent relation and partition bound structure. > > Makes sense. > >> Also: >> >> * The partition constraint for a hash partition is always a call to the >> * built-in function satisfies_hash_partition(). The first two >> arguments are >> * the modulus and remainder for the partition; the remaining arguments >> are the >> * values to be hashed. >> >> I also think the part "The first two arguments are the modulus and >> remainder for the partition;" is wrong (see satisfies_hash_partition). >> But I don't think we need to correct that here because we have described >> about the arguments in comments for that function. So I'd like to >> propose removing the latter comment entirely from the above. > > Here, too. The comment seems to have been obsoleted by f3b0897a121 [2].
Thanks for the review and related git info! Best regards, Etsuro Fujita
