Thread
Commits
-
Update obsolete comment.
- 48de3bb1a50d 12.1 landed
- 80831bcdbe80 13.0 landed
-
Refactor code creating PartitionBoundInfo
- b52b7dc250bb 12.0 cited
-
Obsolete comment in partbounds.c
Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-10-18T07:25:03Z
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 -
Re: Obsolete comment in partbounds.c
Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-10-18T09:56:36Z
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
-
Re: Obsolete comment in partbounds.c
Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-10-19T08:56:02Z
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 -
Re: Obsolete comment in partbounds.c
Etsuro Fujita <etsuro.fujita@gmail.com> — 2019-10-21T08:44:25Z
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
-
Re: Obsolete comment in partbounds.c
Amit Langote <amitlangote09@gmail.com> — 2019-10-23T03:08:04Z
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
-
Re: Obsolete comment in partbounds.c
Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-11-04T16:58:20Z
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