Message-ID: <5A96674D.8090707@lab.ntt.co.jp>
Date: Wed, 28 Feb 2018 17:24:45 +0900
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64;
 rv:11.0) Gecko/20120327 Thunderbird/11.0.1
MIME-Version: 1.0
Subject: Re: Incorrect comments in partition.c
References: <5A671FE1.6020305@lab.ntt.co.jp>
 <d9716065-5b30-9743-f00b-49144675a890@lab.ntt.co.jp>
 <5A681D27.1050705@lab.ntt.co.jp>
In-Reply-To: <5A681D27.1050705@lab.ntt.co.jp>
Content-Type: text/plain; charset=ISO-2022-JP
Content-Transfer-Encoding: 7bit
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: pgsql-hackers@lists.postgresql.org
Precedence: bulk

(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