Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
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