Missing list_free in publicationcmds.c:OpenTableList
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.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
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:t253220psql -h localhost -U postgresBuilt from patchset v7 (message #7), August 11, 2026 at 11:35 PM.
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 t253220_7 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 t253220_7 && git checkout t253220_7Patchset v7 (message #7) is on t253220_7
FWIW, I detected a missing `list_free`, which seems to have been
accidentally left out from commit 923def9.
Added:
list_free(relids_with_collist);
Consistent with the existing:
list_free(relids_with_rf);
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jul 28, 2026 at 10:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
FWIW, I detected a missing `list_free`, which seems to have been
accidentally left out from commit 923def9.Added:
list_free(relids_with_collist);Consistent with the existing:
list_free(relids_with_rf);
Okay since list_free for both relids and relids_with_rf are there, I
think list_free for 'relids_with_rf' also make sense. So LGTM.
thanks
Shveta
On Tue, Jul 28, 2026 at 11:44 PM shveta malik <shveta.malik@gmail.com> wrote:
On Tue, Jul 28, 2026 at 10:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
FWIW, I detected a missing `list_free`, which seems to have been
accidentally left out from commit 923def9.Added:
list_free(relids_with_collist);Consistent with the existing:
list_free(relids_with_rf);Okay since list_free for both relids and relids_with_rf are there, I
think list_free for 'relids_with_rf' also make sense. So LGTM.thanks
Shveta
This is a great find, LGTM!
shihao zhong <zhong950419@gmail.com> writes:
On Tue, Jul 28, 2026 at 11:44 PM shveta malik <shveta.malik@gmail.com> wrote:
On Tue, Jul 28, 2026 at 10:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
FWIW, I detected a missing `list_free`, which seems to have been
accidentally left out from commit 923def9.
Okay since list_free for both relids and relids_with_rf are there, I
think list_free for 'relids_with_rf' also make sense. So LGTM.
AFAICS, that function is executed at most once per DDL command,
in a command-lifetime memory context. I think the right fix
is not to add another list_free but to remove the misleadingly
useless ones that are there.
regards, tom lane
On Wed, Jul 29, 2026 at 2:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
shihao zhong <zhong950419@gmail.com> writes:
On Tue, Jul 28, 2026 at 11:44 PM shveta malik <shveta.malik@gmail.com> wrote:
On Tue, Jul 28, 2026 at 10:30 AM Peter Smith <smithpb2250@gmail.com> wrote:
FWIW, I detected a missing `list_free`, which seems to have been
accidentally left out from commit 923def9.Okay since list_free for both relids and relids_with_rf are there, I
think list_free for 'relids_with_rf' also make sense. So LGTM.AFAICS, that function is executed at most once per DDL command,
in a command-lifetime memory context. I think the right fix
is not to add another list_free but to remove the misleadingly
useless ones that are there.
I added list_free to be consistent with the existing logic.
Removing everything is also a consistent treatment for all lists. Done
as suggested in patch v2.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Peter,
+1 for v2, I confirmed the premise: OpenTableList() is only reached from
CreatePublication() and AlterPublicationTables(), i.e. once
per DDL statement, with the lists allocated in the command's context.
Can you add a comment before the return something like
/*
* relids, relids_with_rf and relids_with_collist are deliberately
not
* freed here. This function runs at most once per DDL command, the
* surrounding command-lifetime memory context frees them later.
*/
So future readers will not think of it as a bug.
This is only a suggestion, feel free to skip it if you'd rather keep the
patch minimal.
Regards,
Surya Poondla
On Tue, Aug 11, 2026 at 8:20 AM surya poondla <suryapoondla4@gmail.com> wrote:
Hi Peter,
+1 for v2, I confirmed the premise: OpenTableList() is only reached from CreatePublication() and AlterPublicationTables(), i.e. once
per DDL statement, with the lists allocated in the command's context.
Thanks for your review!
Can you add a comment before the return something like
/*
* relids, relids_with_rf and relids_with_collist are deliberately not
* freed here. This function runs at most once per DDL command, the
* surrounding command-lifetime memory context frees them later.
*/
So future readers will not think of it as a bug.
This is only a suggestion, feel free to skip it if you'd rather keep the patch minimal.
Fair enough. TBH I'm not sure if comments like this are usual
practice, but I did find a couple of similar examples, so I've added a
comment to v3 as suggested, and a committer can decide to keep it or
not.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Aug 11, 2026 at 4:51 AM Peter Smith <smithpb2250@gmail.com> wrote:
Fair enough. TBH I'm not sure if comments like this are usual
practice,
Yes, I have a similar understanding so let's not set a precedent that
could invite similar comments sprinkled everywhere a list_free/pfree
is not called, which is noise rather than signal.
BTW, as this is not a bug, I intend to push it only in HEAD.
--
With Regards,
Amit Kapila.
Hi Amit,
Yes, I have a similar understanding so let's not set a precedent that
could invite similar comments sprinkled everywhere a list_free/pfree
is not called, which is noise rather than signal.
Sounds good, v2 looks good to me.
My only thought was that the next reader might report it as a leak
again but I agree the comment isn't necessary.
Regards,
Surya Poondla
On 11 Aug 2026, at 04:27, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 11, 2026 at 4:51 AM Peter Smith <smithpb2250@gmail.com> wrote:
Fair enough. TBH I'm not sure if comments like this are usual
practice,Yes, I have a similar understanding so let's not set a precedent that
could invite similar comments sprinkled everywhere a list_free/pfree
is not called, which is noise rather than signal.
A very big +1 on that.
--
Daniel Gustafsson
On Tue, Aug 11, 2026 at 6:06 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2026, at 04:27, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 11, 2026 at 4:51 AM Peter Smith <smithpb2250@gmail.com> wrote:
Fair enough. TBH I'm not sure if comments like this are usual
practice,Yes, I have a similar understanding so let's not set a precedent that
could invite similar comments sprinkled everywhere a list_free/pfree
is not called, which is noise rather than signal.A very big +1 on that.
Pushed.
--
With Regards,
Amit Kapila.
On Wed, Aug 12, 2026 at 9:57 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 11, 2026 at 6:06 PM Daniel Gustafsson <daniel@yesql.se> wrote:
On 11 Aug 2026, at 04:27, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 11, 2026 at 4:51 AM Peter Smith <smithpb2250@gmail.com> wrote:
Fair enough. TBH I'm not sure if comments like this are usual
practice,Yes, I have a similar understanding so let's not set a precedent that
could invite similar comments sprinkled everywhere a list_free/pfree
is not called, which is noise rather than signal.A very big +1 on that.
Pushed.
Thanks for pushing.
======
Kind Regards,
Peter Smith.
Fujitsu Australia