Thread: 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
Attachment
On Thu, Nov 12, 2020 at 5:29 PM Dilip Kumar <dilipbalaut@gmail.com> 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
On Sun, Nov 15, 2020 at 12:50 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Nov 12, 2020 at 5:29 PM Dilip Kumar <dilipbalaut@gmail.com> 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
On Sun, Nov 15, 2020 at 03:49:58PM +0530, Dilip Kumar wrote: > On Sun, Nov 15, 2020 at 12:50 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> 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