Fix a wrong comment in load_file()
Hi,
The comment in load_file():
/* If the same shlib has previously been loaded, unload and reload it. */
But we currently don't support to unload a shlib.
Here is a small patch to fix it.
--
Regards,
ChangAo Chen
Attachments:
0001-Fix-a-wrong-comment-in-load_file.patchapplication/octet-stream; charset=ISO-8859-1; name=0001-Fix-a-wrong-comment-in-load_file.patchDownload
From b627a1c43f301912226abec77b15f9c55b561d68 Mon Sep 17 00:00:00 2001
From: ChangAo Chen <cca5507@qq.com>
Date: Mon, 23 Dec 2024 10:52:08 +0800
Subject: [PATCH] Fix a wrong comment in load_file().
---
src/backend/utils/fmgr/dfmgr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c
index 8e81ecc749..2169b75c30 100644
--- a/src/backend/utils/fmgr/dfmgr.c
+++ b/src/backend/utils/fmgr/dfmgr.c
@@ -125,7 +125,7 @@ load_external_function(const char *filename, const char *funcname,
/*
* This function loads a shlib file without looking up any particular
* function in it. If the same shlib has previously been loaded,
- * unload and reload it.
+ * we don't load it again.
*
* When 'restricted' is true, only libraries in the presumed-secure
* directory $libdir/plugins may be referenced.
@@ -142,7 +142,7 @@ load_file(const char *filename, bool restricted)
/* Expand the possibly-abbreviated filename to an exact path name */
fullname = expand_dynamic_library_name(filename);
- /* Load the shared library */
+ /* Load the shared library, unless we already did */
(void) internal_load_library(fullname);
pfree(fullname);
--
2.34.1
On Mon, Dec 23, 2024 at 10:58:49AM +0800, cca5507 wrote:
/* If the same shlib has previously been loaded, unload and reload it. */
But we currently don't support to unload a shlib.
* This function loads a shlib file without looking up any particular
* function in it. If the same shlib has previously been loaded,
- * unload and reload it.
+ * we don't load it again.Right. This comment is misleading.
- /* Load the shared library */
+ /* Load the shared library, unless we already did */
(void) internal_load_library(fullname);This comment is still true, though. internal_load_library() has some
logic to make sure that we don't load twice the same library on
repeated calls of the function as the file_list is checked.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Dec 23, 2024 at 10:58:49AM +0800, cca5507 wrote:
* This function loads a shlib file without looking up any particular
* function in it. If the same shlib has previously been loaded,
- * unload and reload it.
+ * we don't load it again.
Right. This comment is misleading.
I think it was once correct, decades ago. Obviously we missed
fixing it when we ripped out the unload functionality.
- /* Load the shared library */ + /* Load the shared library, unless we already did */
This comment is still true, though.
Seems like an improvement to me?
regards, tom lane
On Sun, Dec 22, 2024 at 10:50:47PM -0500, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
Right. This comment is misleading.
I think it was once correct, decades ago. Obviously we missed
fixing it when we ripped out the unload functionality.
This was recent enough, and easy to miss.
- /* Load the shared library */ + /* Load the shared library, unless we already did */This comment is still true, though.
Seems like an improvement to me?
Fine by me.
While on it, I think that we should adjust all these ones too:
contrib/sepgsql/hooks.c: * Module load/unload callback
contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
unload */
contrib/passwordcheck/passwordcheck.c:/* Saved hook value in case of
unload */
contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
in case of unload */
The saved hooks are not here to readjust the stack based on a reload,
just to make sure that the existing paths loaded are all taken. I
would just remove the "in case of unload" part for the last three, and
"unload" for the first one. Thoughts?
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
While on it, I think that we should adjust all these ones too:
contrib/sepgsql/hooks.c: * Module load/unload callback
contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
unload */
contrib/passwordcheck/passwordcheck.c:/* Saved hook value in case of
unload */
contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
in case of unload */
Check.
The saved hooks are not here to readjust the stack based on a reload,
just to make sure that the existing paths loaded are all taken. I
would just remove the "in case of unload" part for the last three, and
"unload" for the first one. Thoughts?
WFM
regards, tom lane
> The saved hooks are not here to readjust the stack based on a reload,
> just to make sure that the existing paths loaded are all taken. I
> would just remove the "in case of unload" part for the last three, and
> "unload" for the first one. Thoughts?
LGTM.
--
Regards,
ChangAo Chen