Re: Incorrect comments in partition.c

(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