Re: freeing bms explicitly
Show quoted text
Hi,
I was looking at calls to bms_free() in PG code.e.g. src/backend/commands/publicationcmds.c line 362
bms_free(bms);
The above is just an example, there're other calls to bms_free().
Since the bms is allocated from some execution context, I wonder why this
call is needed.When the underlying execution context wraps up, isn't the bms freed ?
Cheers
Import Notes
Reply to msg id not found: CALNJ-vS6ytPte_hngyQ_cYKjEhD4JA0kkn6v-eXHiyKezDVA@mail.gmail.comReference msg id not found: CALNJ-vS6ytPte_hngyQ_cYKjEhD4JA0kkn6v-eXHiyKezDVA@mail.gmail.com
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at calls to bms_free() in PG code.
e.g. src/backend/commands/publicationcmds.c line 362
bms_free(bms);
The above is just an example, there're other calls to bms_free().
Since the bms is allocated from some execution context, I wonder why this
call is needed.When the underlying execution context wraps up, isn't the bms freed ?
Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even
more pointless, since it'll free only the top node of that expression
tree. Not to mention the string returned by TextDatumGetCString, and
whatever might be leaked during the underlying catalog accesses.
If we were actually worried about transient space consumption of this
function, it'd be necessary to do a lot more than this. It doesn't
look to me like it's worth worrying about though -- it doesn't seem
like it could be hit more than once per query in normal cases.
regards, tom lane
On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at calls to bms_free() in PG code.
e.g. src/backend/commands/publicationcmds.c line 362
bms_free(bms);
The above is just an example, there're other calls to bms_free().
Since the bms is allocated from some execution context, I wonder whythis
call is needed.
When the underlying execution context wraps up, isn't the bms freed ?
Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even
more pointless, since it'll free only the top node of that expression
tree. Not to mention the string returned by TextDatumGetCString, and
whatever might be leaked during the underlying catalog accesses.If we were actually worried about transient space consumption of this
function, it'd be necessary to do a lot more than this. It doesn't
look to me like it's worth worrying about though -- it doesn't seem
like it could be hit more than once per query in normal cases.regards, tom lane
Thanks Tom for replying.
What do you think of the following patch ?
Cheers
Attachments:
rfcolumn-free.patchapplication/octet-stream; name=rfcolumn-free.patchDownload
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 1aad2e769c..d5ec7f8628 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -358,9 +358,6 @@ contain_invalid_rfcolumn(Oid pubid, Relation relation, List *ancestors,
context.bms_replident = bms;
rfnode = stringToNode(TextDatumGetCString(rfdatum));
result = contain_invalid_rfcolumn_walker(rfnode, &context);
-
- bms_free(bms);
- pfree(rfnode);
}
ReleaseSysCache(rftuple);
On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at calls to bms_free() in PG code.
e.g. src/backend/commands/publicationcmds.c line 362
bms_free(bms);
The above is just an example, there're other calls to bms_free().
Since the bms is allocated from some execution context, I wonder why this
call is needed.When the underlying execution context wraps up, isn't the bms freed ?
Yeah, that's kind of pointless --- and the pfree(rfnode) after it is even
more pointless, since it'll free only the top node of that expression
tree. Not to mention the string returned by TextDatumGetCString, and
whatever might be leaked during the underlying catalog accesses.If we were actually worried about transient space consumption of this
function, it'd be necessary to do a lot more than this. It doesn't
look to me like it's worth worrying about though -- it doesn't seem
like it could be hit more than once per query in normal cases.regards, tom lane
Thanks Tom for replying.
What do you think of the following patch ?
Your patch looks good to me. I have found one more similar instance in
the same file and changed that as well accordingly. Let me know what
you think of the attached?
--
With Regards,
Amit Kapila.
Attachments:
v2-0001-Remove-some-useless-free-calls.patchapplication/octet-stream; name=v2-0001-Remove-some-useless-free-calls.patchDownload
From 4f7b6dfa3b384d3aa870da8b37f500aa68e9451d Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Wed, 23 Mar 2022 08:51:05 +0530
Subject: [PATCH v2] Remove some useless free calls.
These were introduced in recent commit 52e4f0cd47. We were trying to free
some transient space consumption and that too was not entirely correct and
complete. We don't need this partial freeing of memory as it will be
allocated just once for a query and will be freed at the end of the query.
Author: Zhihong Yu
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CALNJ-vQORfQ=vicbKA_RmeGZGzm1y3WsEcZqXWi7qjN43Cz_vg@mail.gmail.com
---
src/backend/commands/publicationcmds.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 1aad2e7..9937adb 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -358,9 +358,6 @@ contain_invalid_rfcolumn(Oid pubid, Relation relation, List *ancestors,
context.bms_replident = bms;
rfnode = stringToNode(TextDatumGetCString(rfdatum));
result = contain_invalid_rfcolumn_walker(rfnode, &context);
-
- bms_free(bms);
- pfree(rfnode);
}
ReleaseSysCache(rftuple);
@@ -1046,9 +1043,6 @@ AlterPublicationTables(AlterPublicationStmt *stmt, HeapTuple tup,
}
}
- if (oldrelwhereclause)
- pfree(oldrelwhereclause);
-
/*
* Add the non-matched relations to a list so that they can be
* dropped.
--
1.8.3.1
On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at calls to bms_free() in PG code.
e.g. src/backend/commands/publicationcmds.c line 362
bms_free(bms);
The above is just an example, there're other calls to bms_free().
Since the bms is allocated from some execution context, I wonder whythis
call is needed.
When the underlying execution context wraps up, isn't the bms freed ?
Yeah, that's kind of pointless --- and the pfree(rfnode) after it is
even
more pointless, since it'll free only the top node of that expression
tree. Not to mention the string returned by TextDatumGetCString, and
whatever might be leaked during the underlying catalog accesses.If we were actually worried about transient space consumption of this
function, it'd be necessary to do a lot more than this. It doesn't
look to me like it's worth worrying about though -- it doesn't seem
like it could be hit more than once per query in normal cases.regards, tom lane
Thanks Tom for replying.
What do you think of the following patch ?
Your patch looks good to me. I have found one more similar instance in
the same file and changed that as well accordingly. Let me know what
you think of the attached?--
With Regards,
Amit Kapila.
Hi, Amit:
The patch looks good to me.
Cheers
On Tue, Mar 22, 2022 at 9:04 PM Zhihong Yu <zyu@yugabyte.com> wrote:
On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Mon, Mar 21, 2022 at 3:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Zhihong Yu <zyu@yugabyte.com> writes:
I was looking at calls to bms_free() in PG code.
e.g. src/backend/commands/publicationcmds.c line 362
bms_free(bms);
The above is just an example, there're other calls to bms_free().
Since the bms is allocated from some execution context, I wonderwhy this
call is needed.
When the underlying execution context wraps up, isn't the bms freed
?
Yeah, that's kind of pointless --- and the pfree(rfnode) after it is
even
more pointless, since it'll free only the top node of that expression
tree. Not to mention the string returned by TextDatumGetCString, and
whatever might be leaked during the underlying catalog accesses.If we were actually worried about transient space consumption of this
function, it'd be necessary to do a lot more than this. It doesn't
look to me like it's worth worrying about though -- it doesn't seem
like it could be hit more than once per query in normal cases.regards, tom lane
Thanks Tom for replying.
What do you think of the following patch ?
Your patch looks good to me. I have found one more similar instance in
the same file and changed that as well accordingly. Let me know what
you think of the attached?--
With Regards,
Amit Kapila.Hi, Amit:
The patch looks good to me.Cheers
Tom:
Do you mind taking a look at the latest patch ?
Thanks
On Wed, Mar 23, 2022 at 9:30 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
Your patch looks good to me. I have found one more similar instance in
the same file and changed that as well accordingly. Let me know what
you think of the attached?Hi, Amit:
The patch looks good to me.
Thanks. I'll push this tomorrow unless Tom or someone else wants to
look at it or would like to commit.
--
With Regards,
Amit Kapila.
On Thu, Mar 24, 2022 at 7:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Mar 23, 2022 at 9:30 AM Zhihong Yu <zyu@yugabyte.com> wrote:
On Tue, Mar 22, 2022 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Mar 22, 2022 at 3:39 AM Zhihong Yu <zyu@yugabyte.com> wrote:
Your patch looks good to me. I have found one more similar instance in
the same file and changed that as well accordingly. Let me know what
you think of the attached?Hi, Amit:
The patch looks good to me.Thanks. I'll push this tomorrow unless Tom or someone else wants to
look at it or would like to commit.
Pushed.
--
With Regards,
Amit Kapila.