Thread: Clean up hba.c of code freeing regexps

Clean up hba.c of code freeing regexps

From
Michael Paquier
Date:
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

Attachment

Re: Clean up hba.c of code freeing regexps

From
Thomas Munro
Date:
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.



Re: Clean up hba.c of code freeing regexps

From
Michael Paquier
Date:
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

Attachment

Re: Clean up hba.c of code freeing regexps

From
Tom Lane
Date:
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



Re: Clean up hba.c of code freeing regexps

From
Michael Paquier
Date:
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

Attachment

Re: Clean up hba.c of code freeing regexps

From
"Drouvot, Bertrand"
Date:
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



Re: Clean up hba.c of code freeing regexps

From
Alvaro Herrera
Date:
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



Re: Clean up hba.c of code freeing regexps

From
Michael Paquier
Date:
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

Attachment

Re: Clean up hba.c of code freeing regexps

From
Nathan Bossart
Date:
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] 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



Re: Clean up hba.c of code freeing regexps

From
Michael Paquier
Date:
On Mon, Apr 17, 2023 at 02:21:41PM -0700, Nathan Bossart wrote:
> AFAICT this is committed, but the commitfest entry [0] is still set to
> "needs review."  Can it be closed now?

Yes, done.  Thanks.
--
Michael

Attachment