Re: Remove unused variable from SharedSort
On Sun, Nov 15, 2020 at 03:49:58PM +0530, Dilip Kumar wrote: > On Sun, Nov 15, 2020 at 12:50 PM Bharath Rupireddy > wrote: >> We could have used that variable for an assert like >> Assert(state->worker <= shared->nTapes) in worker_freeze_result_tape() >> before accessing shared->tapes[state->worker] = output; as sometimes >> state->worker is being set to -1. But, it seems like we reach >> worker_freeze_result_tape(), only when WORKER(state) is true. So, we >> don't need that extra Assert and removing nTapes variable makes sense >> to me. > > Right, but anyway IMHO adding extra shared memory variables for just > and assert purposes doesn't make sense. FWIW, I disagree with the removal of this variable because it is useful to track down the number of members in a flexible array at shmem level. Even if you don't use that in some sanity checks for code paths, which I think we actually should really do for at least inittapes() and leader_takeover_tapes() when it comes to the number of participants assumed to exist, that's useful for debugging purposes. Robert, this code has been introduced by 9da0cc3, could you comment on that? -- Michael signature.asc Description: PGP signature
Re: Remove unused variable from SharedSort
On Sun, Nov 15, 2020 at 12:50 PM Bharath Rupireddy wrote: > > On Thu, Nov 12, 2020 at 5:29 PM Dilip Kumar wrote: > > > > While going through the code I noticed that the nTapes member in > > SharedSort is unused. This is just initialized with nworkers but > > never used. The attached patch removes this variable. > > > > We could have used that variable for an assert like > Assert(state->worker <= shared->nTapes) in worker_freeze_result_tape() > before accessing shared->tapes[state->worker] = output; as sometimes > state->worker is being set to -1. But, it seems like we reach > worker_freeze_result_tape(), only when WORKER(state) is true. So, we > don't need that extra Assert and removing nTapes variable makes sense > to me. Right, but anyway IMHO adding extra shared memory variables for just and assert purposes doesn't make sense. > Patch looks good to me. Regression tests make check and make > check-world ran successfully. Thanks for looking into this. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Remove unused variable from SharedSort
On Thu, Nov 12, 2020 at 5:29 PM Dilip Kumar wrote: > > While going through the code I noticed that the nTapes member in > SharedSort is unused. This is just initialized with nworkers but > never used. The attached patch removes this variable. > We could have used that variable for an assert like Assert(state->worker <= shared->nTapes) in worker_freeze_result_tape() before accessing shared->tapes[state->worker] = output; as sometimes state->worker is being set to -1. But, it seems like we reach worker_freeze_result_tape(), only when WORKER(state) is true. So, we don't need that extra Assert and removing nTapes variable makes sense to me. Patch looks good to me. Regression tests make check and make check-world ran successfully. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Remove unused variable from SharedSort
While going through the code I noticed that the nTapes member in SharedSort is unused. This is just initialized with nworkers but never used. The attached patch removes this variable. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com v1-0001-Remove-unused-structure-member-from-Sharedsort.patch Description: Binary data
