Remove unused variable from SharedSort
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t43237psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 20, 2026 at 08:05 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t43237_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t43237_1 && git checkout t43237_1Patchset v1 (message #1) is on t43237_1
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
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