pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
To: pgsql-hackers@lists.postgresql.org
Subject: Incorrect comments in partition.c
Date: Tue, 23 Jan 2018 20:43:29 +0900
Message-ID: <5A671FE1.6020305@lab.ntt.co.jp> (raw)

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

Attachments:

partition-comments.patch (text/x-diff, 2.6K) download *** a/src/backend/catalog/partition.c --- b/src/backend/catalog/partition.c *************** *** 1660,1673 **** make_partition_op_expr(PartitionKey key, int keynum, /* * 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. * * 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. */ static List * get_qual_for_hash(Relation parent, PartitionBoundSpec *spec) --- 1660,1670 ---- /* * get_qual_for_hash * ! * Returns a CHECK constraint expression to use as a hash partition's ! * constraint, given the parent relation and partition bound structure. * * The partition constraint for a hash partition is always a call to the ! * built-in function satisfies_hash_partition(). */ static List * get_qual_for_hash(Relation parent, PartitionBoundSpec *spec) *************** *** 1747,1753 **** get_qual_for_hash(Relation parent, PartitionBoundSpec *spec) * 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. * * The function returns NIL for a default partition when it's the only * partition since in that case there is no constraint. --- 1744,1750 ---- * get_qual_for_list * * Returns an implicit-AND list of expressions to use as a list partition's ! * constraint, given the parent relation and partition bound structure. * * The function returns NIL for a default partition when it's the only * partition since in that case there is no constraint. *************** *** 2027,2033 **** get_range_nulltest(PartitionKey key) * get_qual_for_range * * Returns an implicit-AND list of expressions to use as a range partition's ! * constraint, given the partition key and bound structures. * * For a multi-column range partition key, say (a, b, c), with (al, bl, cl) * as the lower bound tuple and (au, bu, cu) as the upper bound tuple, we --- 2024,2030 ---- * get_qual_for_range * * Returns an implicit-AND list of expressions to use as a range partition's ! * constraint, given the parent relation and partition bound structure. * * For a multi-column range partition key, say (a, b, c), with (al, bl, cl) * as the lower bound tuple and (au, bu, cu) as the upper bound tuple, we

view thread (6+ messages)  latest in thread

Message-ID: <5A671FE1.6020305@lab.ntt.co.jp>
Permalink:  ../5A671FE1.6020305@lab.ntt.co.jp/
Also on:    postgresql.org/message-id/5A671FE1.6020305@lab.ntt.co.jp

 ·  · 

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-hackers@postgresql.org
  Cc: fujita.etsuro@lab.ntt.co.jp, pgsql-hackers@lists.postgresql.org
  Subject: Re: Incorrect comments in partition.c
  In-Reply-To: <5A671FE1.6020305@lab.ntt.co.jp>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox