Obsolete comment in partbounds.c
While reviewing the partitionwise-join patch, I noticed $Subject，ie,
this in create_list_bounds():
/*
* Never put a null into the values array, flag instead for
* the code further down below where we construct the actual
* relcache struct.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
null_index = i;
"the code further down below where we construct the actual relcache
struct" isn't in the same file anymore by refactoring by commit
b52b7dc25. How about modifying it like the attached?
Best regards,
Etsuro Fujita
Attachments:
fix-obsolete-comment.patchapplication/octet-stream; name=fix-obsolete-comment.patchDownload
*** a/src/backend/partitioning/partbounds.c
--- b/src/backend/partitioning/partbounds.c
***************
*** 360,368 **** create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
else
{
/*
! * Never put a null into the values array, flag instead for
! * the code further down below where we construct the actual
! * relcache struct.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
--- 360,367 ----
else
{
/*
! * Never put a null into the values array; save the index of
! * the partition the null comes from, instead.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
On 2019-Oct-18, Etsuro Fujita wrote:
While reviewing the partitionwise-join patch, I noticed $Subject，ie,
this in create_list_bounds():/*
* Never put a null into the values array, flag instead for
* the code further down below where we construct the actual
* relcache struct.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
null_index = i;"the code further down below where we construct the actual relcache
struct" isn't in the same file anymore by refactoring by commit
b52b7dc25. How about modifying it like the attached?
Yeah, agreed. Instead of "the null comes from" I would use "the
partition that stores nulls".
While reviewing your patch I noticed a few places where we use an odd
pattern in switches, which can be simplified as shown here.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
simplify-returns.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 318d8ecae9..e051094d54 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -209,13 +209,9 @@ partition_bounds_create(PartitionBoundSpec **boundspecs, int nparts,
return create_range_bounds(boundspecs, nparts, key, mapping);
default:
- elog(ERROR, "unexpected partition strategy: %d",
- (int) key->strategy);
- break;
+ Assert(false);
+ return NULL; /* keep compiler quiet */
}
-
- Assert(false);
- return NULL; /* keep compiler quiet */
}
/*
@@ -1797,10 +1793,7 @@ qsort_partition_rbound_cmp(const void *a, const void *b, void *arg)
static int
get_partition_bound_num_indexes(PartitionBoundInfo bound)
{
- int num_indexes;
-
Assert(bound);
-
switch (bound->strategy)
{
case PARTITION_STRATEGY_HASH:
@@ -1809,24 +1802,20 @@ get_partition_bound_num_indexes(PartitionBoundInfo bound)
* The number of the entries in the indexes array is same as the
* greatest modulus.
*/
- num_indexes = get_hash_partition_greatest_modulus(bound);
- break;
+ return get_hash_partition_greatest_modulus(bound);
case PARTITION_STRATEGY_LIST:
- num_indexes = bound->ndatums;
+ return bound->ndatums;
break;
case PARTITION_STRATEGY_RANGE:
/* Range partitioned table has an extra index. */
- num_indexes = bound->ndatums + 1;
- break;
+ return bound->ndatums + 1;
default:
- elog(ERROR, "unexpected partition strategy: %d",
- (int) bound->strategy);
+ Assert(false);
+ return 0; /* keep compiler quiet */
}
-
- return num_indexes;
}
/*
Hi Alvaro,
On Fri, Oct 18, 2019 at 6:56 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Oct-18, Etsuro Fujita wrote:
While reviewing the partitionwise-join patch, I noticed $Subject，ie,
this in create_list_bounds():/*
* Never put a null into the values array, flag instead for
* the code further down below where we construct the actual
* relcache struct.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
null_index = i;"the code further down below where we construct the actual relcache
struct" isn't in the same file anymore by refactoring by commit
b52b7dc25. How about modifying it like the attached?Yeah, agreed. Instead of "the null comes from" I would use "the
partition that stores nulls".
I think your wording is better than mine. Thank you for reviewing!
While reviewing your patch I noticed a few places where we use an odd
pattern in switches, which can be simplified as shown here.
case PARTITION_STRATEGY_LIST:
- num_indexes = bound->ndatums;
+ return bound->ndatums;
break;Why not remove the break statement?
Other than that the patch looks good to me.
Best regards,
Etsuro Fujita
On Sat, Oct 19, 2019 at 5:56 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Fri, Oct 18, 2019 at 6:56 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Oct-18, Etsuro Fujita wrote:
While reviewing the partitionwise-join patch, I noticed $Subject，ie,
this in create_list_bounds():/*
* Never put a null into the values array, flag instead for
* the code further down below where we construct the actual
* relcache struct.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
null_index = i;"the code further down below where we construct the actual relcache
struct" isn't in the same file anymore by refactoring by commit
b52b7dc25. How about modifying it like the attached?Yeah, agreed. Instead of "the null comes from" I would use "the
partition that stores nulls".I think your wording is better than mine. Thank you for reviewing!
I applied the patch down to PG12.
Best regards,
Etsuro Fujita
On Mon, Oct 21, 2019 at 5:44 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Sat, Oct 19, 2019 at 5:56 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
On Fri, Oct 18, 2019 at 6:56 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Oct-18, Etsuro Fujita wrote:
While reviewing the partitionwise-join patch, I noticed $Subject，ie,
this in create_list_bounds():/*
* Never put a null into the values array, flag instead for
* the code further down below where we construct the actual
* relcache struct.
*/
if (null_index != -1)
elog(ERROR, "found null more than once");
null_index = i;"the code further down below where we construct the actual relcache
struct" isn't in the same file anymore by refactoring by commit
b52b7dc25. How about modifying it like the attached?Yeah, agreed. Instead of "the null comes from" I would use "the
partition that stores nulls".I think your wording is better than mine. Thank you for reviewing!
I applied the patch down to PG12.
Thank you Fujita-san and Alvaro.
Regards,
Amit
On 2019-Oct-19, Etsuro Fujita wrote:
On Fri, Oct 18, 2019 at 6:56 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Yeah, agreed. Instead of "the null comes from" I would use "the
partition that stores nulls".I think your wording is better than mine. Thank you for reviewing!
Thanks for getting this done.
While reviewing your patch I noticed a few places where we use an odd
pattern in switches, which can be simplified as shown here.case PARTITION_STRATEGY_LIST: - num_indexes = bound->ndatums; + return bound->ndatums; break;Why not remove the break statement?
You're right, I should have done that. However, I backed out of doing
this change after all; it seems a pretty minor stylistic adjustment of
little value.
Thanks for reviewing all the same,
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services