Remove unused variable from SharedSort

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