Freeing sortgroupatts in use_physical_tlist
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:t46242psql -h localhost -U postgresBuilt from patchset v1 (message #1), September 20, 2026 at 11:24 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 t46242_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 t46242_1 && git checkout t46242_1Patchset v1 (message #1) is on t46242_1
Hi,
I was looking at the code in use_physical_tlist().
In the code block checking CP_LABEL_TLIST, I noticed that
the Bitmapset sortgroupatts is not freed before returning from the method.
Looking at create_foreignscan_plan() (in the same file):
bms_free(attrs_used);
It seems the intermediate Bitmapset is freed before returning.
I would appreciate review comments for the proposed patch.
Thanks
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at the code in use_physical_tlist().
In the code block checking CP_LABEL_TLIST, I noticed that
the Bitmapset sortgroupatts is not freed before returning from the method.
Looking at create_foreignscan_plan() (in the same file):
bms_free(attrs_used);
It seems the intermediate Bitmapset is freed before returning.
TBH, I'd say that it's probably the former code not the latter
that is good practice. Retail pfree's in code that's not in a
loop very possibly expend more cycles than they are worth, because
the space will get cleaned up anyway when the active memory
context is reset, and pfree is not as cheap as one might wish.
It might be possible to make a case for one method over the other
with some study of the particular situation, but you can't say
a priori which way is better.
On the whole, I would not bother changing either of these bits
of code without some clear evidence that it matters. It likely
doesn't. It's even more likely that it doesn't matter enough
to be worth investigating.
regards, tom lane
On Fri, Jul 15, 2022 at 8:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at the code in use_physical_tlist().
In the code block checking CP_LABEL_TLIST, I noticed that
the Bitmapset sortgroupatts is not freed before returning from themethod.
Looking at create_foreignscan_plan() (in the same file):
bms_free(attrs_used);
It seems the intermediate Bitmapset is freed before returning.TBH, I'd say that it's probably the former code not the latter
that is good practice. Retail pfree's in code that's not in a
loop very possibly expend more cycles than they are worth, because
the space will get cleaned up anyway when the active memory
context is reset, and pfree is not as cheap as one might wish.
It might be possible to make a case for one method over the other
with some study of the particular situation, but you can't say
a priori which way is better.On the whole, I would not bother changing either of these bits
of code without some clear evidence that it matters. It likely
doesn't. It's even more likely that it doesn't matter enough
to be worth investigating.regards, tom lane
Hi, Tom:
Thanks for responding over the weekend.
I will try to remember what you said.
Cheers