Clean up hba.c of code freeing regexps
Hi all,
With db4f21e in place, there is no need to worry about explicitely
freeing any regular expressions that may have been compiled when
loading HBA or ident files because MemoryContextDelete() would be
able to take care of that now that these are palloc'd (the definitions
in regcustom.h superseed the ones of regguts.h).
The logic in hba.c that scans all the HBA and ident lines to any
regexps can be simplified a lot. Most of this code is new in 16~, so
I think that it is worth cleaning up this stuff now rather than wait
for 17 to open for business. Still, this is optional, and I don't
mind waiting for 17 if the regexp/palloc business proves to be an
issue during beta.
FWIW, one would see leaks in the postmaster process with files like
that on repeated SIGHUPs before db4f21e:
$ cat $PGDATA/pg_hba.conf
local "/^db\d{2,4}$" all trust
local "/^db\d{2,po}$" all trust
local "/^db\d{2,4}$" all trust
$ cat $PGDATA/pg_ident.conf
foo "/^user\d{2,po}$" bar
foo "/^uesr\d{2,4}$" bar
With this configuration, there are no leaks on SIGHUPs after db4f21e
as MemoryContextDelete() does all the job. Please see the attached.
Thoughts or opinions?
--
Michael
Attachments:
hba-free-regexps.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index adedbd3128..d786a01835 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -95,11 +95,6 @@ static MemoryContext parsed_hba_context = NULL;
/*
* pre-parsed content of ident mapping file: list of IdentLine structs.
* parsed_ident_context is the memory context where it lives.
- *
- * NOTE: the IdentLine structs can contain AuthTokens with pre-compiled
- * regular expressions that live outside the memory context. Before
- * destroying or resetting the memory context, they need to be explicitly
- * free'd.
*/
static List *parsed_ident_lines = NIL;
static MemoryContext parsed_ident_context = NULL;
@@ -316,30 +311,6 @@ free_auth_token(AuthToken *token)
pg_regfree(token->regex);
}
-/*
- * Free a HbaLine. Its list of AuthTokens for databases and roles may include
- * regular expressions that need to be cleaned up explicitly.
- */
-static void
-free_hba_line(HbaLine *line)
-{
- ListCell *cell;
-
- foreach(cell, line->roles)
- {
- AuthToken *tok = lfirst(cell);
-
- free_auth_token(tok);
- }
-
- foreach(cell, line->databases)
- {
- AuthToken *tok = lfirst(cell);
-
- free_auth_token(tok);
- }
-}
-
/*
* Copy a AuthToken struct into freshly palloc'd memory.
*/
@@ -2722,30 +2693,14 @@ load_hba(void)
if (!ok)
{
/*
- * File contained one or more errors, so bail out, first being careful
- * to clean up whatever we allocated. Most stuff will go away via
- * MemoryContextDelete, but we have to clean up regexes explicitly.
+ * File contained one or more errors, so bail out. MemoryContextDelete
+ * is enough to clean up everything, including regexes.
*/
- foreach(line, new_parsed_lines)
- {
- HbaLine *newline = (HbaLine *) lfirst(line);
-
- free_hba_line(newline);
- }
MemoryContextDelete(hbacxt);
return false;
}
/* Loaded new file successfully, replace the one we use */
- if (parsed_hba_lines != NIL)
- {
- foreach(line, parsed_hba_lines)
- {
- HbaLine *newline = (HbaLine *) lfirst(line);
-
- free_hba_line(newline);
- }
- }
if (parsed_hba_context != NULL)
MemoryContextDelete(parsed_hba_context);
parsed_hba_context = hbacxt;
@@ -3044,8 +2999,7 @@ load_ident(void)
{
FILE *file;
List *ident_lines = NIL;
- ListCell *line_cell,
- *parsed_line_cell;
+ ListCell *line_cell;
List *new_parsed_lines = NIL;
bool ok = true;
MemoryContext oldcxt;
@@ -3102,30 +3056,14 @@ load_ident(void)
if (!ok)
{
/*
- * File contained one or more errors, so bail out, first being careful
- * to clean up whatever we allocated. Most stuff will go away via
- * MemoryContextDelete, but we have to clean up regexes explicitly.
+ * File contained one or more errors, so bail out. MemoryContextDelete
+ * is enough to clean up everything, including regexes.
*/
- foreach(parsed_line_cell, new_parsed_lines)
- {
- newline = (IdentLine *) lfirst(parsed_line_cell);
- free_auth_token(newline->system_user);
- free_auth_token(newline->pg_user);
- }
MemoryContextDelete(ident_context);
return false;
}
/* Loaded new file successfully, replace the one we use */
- if (parsed_ident_lines != NIL)
- {
- foreach(parsed_line_cell, parsed_ident_lines)
- {
- newline = (IdentLine *) lfirst(parsed_line_cell);
- free_auth_token(newline->system_user);
- free_auth_token(newline->pg_user);
- }
- }
if (parsed_ident_context != NULL)
MemoryContextDelete(parsed_ident_context);
On Thu, Apr 13, 2023 at 12:16 PM Michael Paquier <michael@paquier.xyz> wrote:
The logic in hba.c that scans all the HBA and ident lines to any
regexps can be simplified a lot. Most of this code is new in 16~, so
I think that it is worth cleaning up this stuff now rather than wait
for 17 to open for business. Still, this is optional, and I don't
mind waiting for 17 if the regexp/palloc business proves to be an
issue during beta.
Up to the RMT of course, but it sounds a bit like (1) you potentially
had an open item already until last week (new code in 16 that could
leak regexes), and (2) I missed this when looking for manual memory
management code that could be nuked, probably because it's hiding
behind a few layers of functions call, but there are clearly comments
that are now wrong. So there are two different ways for a commitfest
lawyer to argue this should be tidied up for 16.
On Thu, Apr 13, 2023 at 12:53:51PM +1200, Thomas Munro wrote:
Up to the RMT of course, but it sounds a bit like (1) you potentially
had an open item already until last week (new code in 16 that could
leak regexes),
Well, not really.. Note that HEAD does not leak regexes, because
changes like 02d3448 have made sure that all these are explicitely
freed when a file is parsed and where there is no need to switch to
the new one because errors were found on the way. In short, one can
just take the previous conf files I pasted and there will be no leaks
on HEAD in the context of the postmaster, even before bea3d7e. Sure,
there could be issues if one changed the shape of the code, but all
the existing compiled regexes were covered (this stuff already exists
in ~15 for the regexps of the user name in ident lines).
and (2) I missed this when looking for manual memory
management code that could be nuked, probably because it's hiding
behind a few layers of functions call, but there are clearly comments
that are now wrong. So there are two different ways for a commitfest
lawyer to argue this should be tidied up for 16.
Yes, the comments are incorrect anyway.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Thu, Apr 13, 2023 at 12:53:51PM +1200, Thomas Munro wrote:
and (2) I missed this when looking for manual memory
management code that could be nuked, probably because it's hiding
behind a few layers of functions call, but there are clearly comments
that are now wrong. So there are two different ways for a commitfest
lawyer to argue this should be tidied up for 16.
Yes, the comments are incorrect anyway.
+1 for cleanup, if this is new code. It does us no good in the long
run for v16 to handle this differently from both earlier and later
versions.
regards, tom lane
On Wed, Apr 12, 2023 at 10:25:42PM -0400, Tom Lane wrote:
+1 for cleanup, if this is new code. It does us no good in the long
run for v16 to handle this differently from both earlier and later
versions.
Okidoki. Let me know if anybody has an objection about what's been
sent upthread. The sooner, the better I guess..
--
Michael
Hi,
On 4/13/23 5:48 AM, Michael Paquier wrote:
On Wed, Apr 12, 2023 at 10:25:42PM -0400, Tom Lane wrote:
+1 for cleanup, if this is new code. It does us no good in the long
run for v16 to handle this differently from both earlier and later
versions.Okidoki. Let me know if anybody has an objection about what's been
sent upthread. The sooner, the better I guess..
Thanks for the patch, nice catch related to db4f21e.
I had a look at the patch you shared up-thread and it looks good to me.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On 2023-Apr-13, Michael Paquier wrote:
With db4f21e in place, there is no need to worry about explicitely
freeing any regular expressions that may have been compiled when
loading HBA or ident files because MemoryContextDelete() would be
able to take care of that now that these are palloc'd (the definitions
in regcustom.h superseed the ones of regguts.h).
Hmm, nice.
The logic in hba.c that scans all the HBA and ident lines to any
regexps can be simplified a lot. Most of this code is new in 16~, so
I think that it is worth cleaning up this stuff now rather than wait
for 17 to open for business. Still, this is optional, and I don't
mind waiting for 17 if the regexp/palloc business proves to be an
issue during beta.
I agree with the downthread votes to clean this up now rather than
waiting. Also, you're adding exactly zero lines of new code, so I don't
think feature freeze affects the decision.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL. This is by far the most pleasant management experience of
any database I've worked on." (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php
On Thu, Apr 13, 2023 at 11:58:51AM +0200, Alvaro Herrera wrote:
I agree with the downthread votes to clean this up now rather than
waiting. Also, you're adding exactly zero lines of new code, so I don't
think feature freeze affects the decision.
Thanks, done that.
The commit log mentions Lab.c instead of hba.c. I had that fixed
locally, but it seems like I've messed up things a bit.. Sorry about
that.
--
Michael
On Fri, Apr 14, 2023 at 07:32:13AM +0900, Michael Paquier wrote:
On Thu, Apr 13, 2023 at 11:58:51AM +0200, Alvaro Herrera wrote:
I agree with the downthread votes to clean this up now rather than
waiting. Also, you're adding exactly zero lines of new code, so I don't
think feature freeze affects the decision.Thanks, done that.
The commit log mentions Lab.c instead of hba.c. I had that fixed
locally, but it seems like I've messed up things a bit.. Sorry about
that.
AFAICT this is committed, but the commitfest entry [0]https://commitfest.postgresql.org/43/4277/ is still set to
"needs review." Can it be closed now?
[0]: https://commitfest.postgresql.org/43/4277/
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com