cleanup in open_auth_file
Hi,
I was looking at the following commit:
commit efc981627a723d91e86865fb363d793282e473d1
Author: Michael Paquier <michael@paquier.xyz>
Date: Thu Nov 24 08:21:55 2022 +0900
Rework memory contexts in charge of HBA/ident tokenization
I think when the file cannot be opened, the context should be deleted.
Please see attached patch.
I also modified one comment where `deleted` would be more appropriate verb
for the context.
Cheers
Attachments:
open-auth-file-cleanup.patchapplication/octet-stream; name=open-auth-file-cleanup.patchDownload
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 432fbeeb1c..0995a61e25 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -603,7 +603,7 @@ open_auth_file(const char *filename, int elevel, int depth,
/*
* When opening the top-level file, create the memory context used for the
- * tokenization. This will be closed with this file when coming back to
+ * tokenization. This will be deleted with this file when coming back to
* this level of cleanup.
*/
if (depth == 0)
@@ -613,8 +613,8 @@ open_auth_file(const char *filename, int elevel, int depth,
* already.
*/
tokenize_context = AllocSetContextCreate(CurrentMemoryContext,
- "tokenize_context",
- ALLOCSET_START_SMALL_SIZES);
+ "tokenize_context",
+ ALLOCSET_START_SMALL_SIZES);
}
file = AllocateFile(filename, "r");
@@ -622,6 +622,11 @@ open_auth_file(const char *filename, int elevel, int depth,
{
int save_errno = errno;
+ if (depth == 0)
+ {
+ MemoryContextDelete(tokenize_context);
+ tokenize_context = NULL;
+ }
ereport(elevel,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
On Wed, Nov 23, 2022 at 4:54 PM Ted Yu <yuzhihong@gmail.com> wrote:
Hi,
I was looking at the following commit:commit efc981627a723d91e86865fb363d793282e473d1
Author: Michael Paquier <michael@paquier.xyz>
Date: Thu Nov 24 08:21:55 2022 +0900Rework memory contexts in charge of HBA/ident tokenization
I think when the file cannot be opened, the context should be deleted.
Please see attached patch.
I also modified one comment where `deleted` would be more appropriate verb
for the context.Cheers
Thinking more on this.
The context should be created when the file is successfully opened.
Please take a look at patch v2.
Attachments:
open-auth-file-cleanup-v2.patchapplication/octet-stream; name=open-auth-file-cleanup-v2.patchDownload
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 432fbeeb1c..5acd39ae5f 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -601,22 +601,6 @@ open_auth_file(const char *filename, int elevel, int depth,
return NULL;
}
- /*
- * When opening the top-level file, create the memory context used for the
- * tokenization. This will be closed with this file when coming back to
- * this level of cleanup.
- */
- if (depth == 0)
- {
- /*
- * A context may be present, but assume that it has been eliminated
- * already.
- */
- tokenize_context = AllocSetContextCreate(CurrentMemoryContext,
- "tokenize_context",
- ALLOCSET_START_SMALL_SIZES);
- }
-
file = AllocateFile(filename, "r");
if (file == NULL)
{
@@ -634,6 +618,22 @@ open_auth_file(const char *filename, int elevel, int depth,
return NULL;
}
+ /*
+ * When opening the top-level file, create the memory context used for the
+ * tokenization. This will be deleted with this file when coming back to
+ * this level of cleanup.
+ */
+ if (depth == 0)
+ {
+ /*
+ * A context may be present, but assume that it has been eliminated
+ * already.
+ */
+ tokenize_context = AllocSetContextCreate(CurrentMemoryContext,
+ "tokenize_context",
+ ALLOCSET_START_SMALL_SIZES);
+ }
+
return file;
}
On Wed, Nov 23, 2022 at 05:09:22PM -0800, Ted Yu wrote:
Thinking more on this.
The context should be created when the file is successfully opened.
Indeed. Both operations ought to be done in the reverse order, or we
would run into leaks in the postmaster on reload if pg_ident.conf has
been removed, for example, and this is prefectly valid. That's what
the previous logic did, actually. Will fix in a minute..
--
Michael