Mention ordered datums in PartitionBoundInfoData comment

Started by Ashutosh Bapatabout 8 years ago6 messages

ashutosh.bapat@enterprisedb.com

about 8 years ago

1 attachment(s)

Hi,

Julien Rouhaund, who has proposed a patch for partition-wise ordering
mentioned to me offlist that the comments for PartitionBoundInfoData
do not mention the fact that the datums in datums array are ordered. I
think that's important to mention there. So here's patch to do that.

The comment I have added refers to the functions which order the
datums, since every partition kind has different method of ordering
datums and I think the prologue is not a suitable place to explain
that ordering. I have added a sentence for range and list partitioning
since the ordering is easier to explain in those cases. Also added a
sentence about canonical PartitionBoundInfoData without using that
word.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

pbid_ordering_comment.patchtext/x-patch; charset=US-ASCII; name=pbid_ordering_comment.patchDownload

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dd4a8d3..865f572 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -72,6 +72,13 @@
  * of datum-tuples with 2 datums, modulus and remainder, corresponding to a
  * given partition.
  *
+ * The datums in datums array are arranged in the increasing order defined by
+ * functions qsort_partition_rbound_cmp(), qsort_partition_list_value_cmp() and
+ * qsort_partition_hbound_cmp() for range, list and hash partitioned tables
+ * resp. For range and list partitions this simply means that the datums in the
+ * datums array are arranged in the increasing order defined by the partition
+ * key collation.
+ *
  * In the case of list partitioning, the indexes array stores one entry for
  * every datum, which is the index of the partition that accepts a given datum.
  * In case of range partitioning, it stores one entry per distinct range
@@ -82,6 +89,9 @@
  * partition which would accept that datum-tuple would be given by the entry
  * pointed by remainder produced when hash value of the datum-tuple is divided
  * by the greatest modulus.
+ *
+ * PartitionBoundInfoData structures for two partitioned table with exactly same
+ * bounds look exactly same.
  */
 
 typedef struct PartitionBoundInfoData

Julien Rouhaud

rjuju123@gmail.com

about 8 years ago

In reply to: Ashutosh Bapat (#1)

Re: Mention ordered datums in PartitionBoundInfoData comment

On Tue, Dec 5, 2017 at 5:48 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Hi,

Hi,

Julien Rouhaund, who has proposed a patch for partition-wise ordering
mentioned to me offlist that the comments for PartitionBoundInfoData
do not mention the fact that the datums in datums array are ordered. I
think that's important to mention there. So here's patch to do that.

Thanks for the patch! I agree this should mentioned in the comment.

small typo:

+ * PartitionBoundInfoData structures for two partitioned table with
exactly same

should be "tables".

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

about 8 years ago

In reply to: Julien Rouhaud (#2)

1 attachment(s)

Re: Mention ordered datums in PartitionBoundInfoData comment

On Tue, Dec 5, 2017 at 11:20 AM, Julien Rouhaud <rjuju123@gmail.com> wrote:

+ * PartitionBoundInfoData structures for two partitioned table with
exactly same

should be "tables".

Sorry. Thanks for pointing it out. fixed in the attached patch.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

pbid_ordering_comment_v2.patchtext/x-patch; charset=US-ASCII; name=pbid_ordering_comment_v2.patchDownload

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dd4a8d3..4cb3e16 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -72,6 +72,13 @@
  * of datum-tuples with 2 datums, modulus and remainder, corresponding to a
  * given partition.
  *
+ * The datums in datums array are arranged in the increasing order defined by
+ * functions qsort_partition_rbound_cmp(), qsort_partition_list_value_cmp() and
+ * qsort_partition_hbound_cmp() for range, list and hash partitioned tables
+ * resp. For range and list partitions this simply means that the datums in the
+ * datums array are arranged in the increasing order defined by the partition
+ * key collation.
+ *
  * In the case of list partitioning, the indexes array stores one entry for
  * every datum, which is the index of the partition that accepts a given datum.
  * In case of range partitioning, it stores one entry per distinct range
@@ -82,6 +89,9 @@
  * partition which would accept that datum-tuple would be given by the entry
  * pointed by remainder produced when hash value of the datum-tuple is divided
  * by the greatest modulus.
+ *
+ * PartitionBoundInfoData structures for two partitioned tables with exactly
+ * same bounds look exactly same.
  */
 
 typedef struct PartitionBoundInfoData

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Ashutosh Bapat (#3)

Re: Mention ordered datums in PartitionBoundInfoData comment

On Tue, Dec 5, 2017 at 1:03 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Sorry. Thanks for pointing it out. fixed in the attached patch.

+ * The datums in datums array are arranged in the increasing order defined by

Suggest: in increasing order as defined

There's a second place where the same change is needed.

+ * resp. For range and list partitions this simply means that the datums in the

I think you should spell out "respectively" instead of abbreviating to "resp".

+ * datums array are arranged in the increasing order defined by the partition
+ * key collation.

It's not just the collation but also, and I think more importantly,
the operator class. And there can be multiple columns, and thus
multiple opclases/collations. Maybe "defined by the partition key's
operator classes and collations".

+ * PartitionBoundInfoData structures for two partitioned tables with exactly
+ * same bounds look exactly same.

This doesn't seem to me to add much.

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

Ashutosh Bapat

ashutosh.bapat@enterprisedb.com

about 8 years ago

In reply to: Robert Haas (#4)

1 attachment(s)

Re: Mention ordered datums in PartitionBoundInfoData comment

On Fri, Dec 8, 2017 at 12:25 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Dec 5, 2017 at 1:03 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Sorry. Thanks for pointing it out. fixed in the attached patch.

+ * The datums in datums array are arranged in the increasing order defined by

Suggest: in increasing order as defined

Done.

There's a second place where the same change is needed.

Done.

+ * resp. For range and list partitions this simply means that the datums in the

I think you should spell out "respectively" instead of abbreviating to "resp".

Done.

+ * datums array are arranged in the increasing order defined by the partition
+ * key collation.
It's not just the collation but also, and I think more importantly,
the operator class. And there can be multiple columns, and thus
multiple opclases/collations. Maybe "defined by the partition key's
operator classes and collations".

I had forgot about the operator class. Sorry. Done.

+ * PartitionBoundInfoData structures for two partitioned tables with exactly
+ * same bounds look exactly same.

This doesn't seem to me to add much.

We have a comment in partition_bounds_equal()'s prologue

"PartitionBoundInfo is a canonical representation of partition bounds.".

But we may use that property in other places also, so having it in
prologue of PartitionBoundsInfoData makes sense. For now, I have
removed those lines.

PFA updated patch.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

pbid_ordering_comment_v3.patchtext/x-patch; charset=US-ASCII; name=pbid_ordering_comment_v3.patchDownload

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index dd4a8d3..ef156e4 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -72,6 +72,13 @@
  * of datum-tuples with 2 datums, modulus and remainder, corresponding to a
  * given partition.
  *
+ * The datums in datums array are arranged in increasing order as defined by
+ * functions qsort_partition_rbound_cmp(), qsort_partition_list_value_cmp() and
+ * qsort_partition_hbound_cmp() for range, list and hash partitioned tables
+ * respectively. For range and list partitions this simply means that the
+ * datums in the datums array are arranged in increasing order as defined by
+ * the partition key's operator classes and collations.
+ *
  * In the case of list partitioning, the indexes array stores one entry for
  * every datum, which is the index of the partition that accepts a given datum.
  * In case of range partitioning, it stores one entry per distinct range

Robert Haas

robertmhaas@gmail.com

about 8 years ago

In reply to: Ashutosh Bapat (#5)

Re: Mention ordered datums in PartitionBoundInfoData comment

On Mon, Dec 11, 2017 at 7:28 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

PFA updated patch.

Committed.

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