Hello Ashutosh, Fujita, On Wed, Apr 8, 2020 at 3:49 PM Ashutosh Bapat <ashutosh.ba...@2ndquadrant.com> wrote: > On Wed, 8 Apr 2020 at 15:42, Etsuro Fujita <etsuro.fuj...@gmail.com> wrote: >> On Wed, Apr 8, 2020 at 4:30 PM Kuntal Ghosh <kuntalghosh.2...@gmail.com> >> wrote: >> > I'm getting the following warning during compilation. >> > >> > partbounds.c: In function ‘partition_bounds_merge’: >> > partbounds.c:1024:21: warning: unused variable ‘inner_binfo’ >> > [-Wunused-variable] >> > PartitionBoundInfo inner_binfo = inner_rel->boundinfo; >> > ^ >> > For fixing the same, we can declare inner_binfo as >> > PG_USED_FOR_ASSERTS_ONLY as it is not used for any other purpose. >> >> I'd propose to remove an assertion causing this (and the >> outer_binfo/inner_binfo variables) from partition_bounds_merge(), >> rather than doing so, because the assertion is redundant, as we have >> the same assertion in merge_list_bounds() and merge_range_bounds(). >> Please find attached a patch. > > > I think it's better to have the assertion in all the three places and also in > merge_hash_bounds() whenever that comes along. The assertion in > merge_*_bounds() will be good to in case those functions are called from > places other than partition_bounds_merge(). The assertion in > partition_bounds_merge() will make sure that when the individual > merge_*_bounds() functions are called based on one of the bounds both of the > bounds have same strategy.
Both of your patches fix the problem. I don't have much exposure in this area to comment on whether we should keep/remove the assertion from the code. But, here is my opinion: The code structure looks like following: Assert(condition A); if (Condition B) merge_*_bounds(....); Inside merge_*_bounds(), you have both the above assert and the if condition as another assert: Assert(condition A and Condition B); And, merge_*_bounds() are called from only one place. So, something is redundant here and I'm inclined towards removal of the assert condition. Another thing I noticed: /* The partitioning strategies should be the same. */ Assert(outer_binfo->strategy == inner_binfo->strategy); The comment just reads the assertion aloud which looks unnecessary. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com