Thread: 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
Attachment
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
Attachment
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