(2018/01/24 14:44), Etsuro Fujita wrote: > (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!
I'll add this to the upcoming commitfest. Best regards, Etsuro Fujita
