Incorrect comments in partition.c

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.

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.

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.

Attached is a proposed patch for that.

Best regards,
Etsuro Fujita

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,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=6f6b99d1335

[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f3b0897a121

(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

(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

On Wed, Feb 28, 2018 at 3:24 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I'll add this to the upcoming commitfest.

Committed. Sorry that I didn't notice this thread sooner (and that
the original commits didn't take care of it).

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(2018/03/01 0:12), Robert Haas wrote:
> On Wed, Feb 28, 2018 at 3:24 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> I'll add this to the upcoming commitfest.
>
> Committed. Sorry that I didn't notice this thread sooner (and that
> the original commits didn't take care of it).

Thanks!

Best regards,
Etsuro Fujita