From: Tom Lane <tgl@sss.pgh.pa.us>
To: Matheus Alcantara <matheusssilv97@gmail.com>
cc: Etsuro Fujita <etsuro.fujita@gmail.com>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Fixing memory leaks in postgres_fdw
In-reply-to: 
 <CAFY6G8fEa9sZ1V9cJpMOZAp6Wfa35U_rE0Y=eoQLS2G3OAqiAw@mail.gmail.com>
References: <2976982.1748049023@sss.pgh.pa.us>
 <CAPmGK17iOh6W25UswGcn5k75jEukSgLMpiaJEATcAZQTec7xxg@mail.gmail.com>
 <216286.1748202797@sss.pgh.pa.us> <519144.1748217540@sss.pgh.pa.us>
 <918254.1748288174@sss.pgh.pa.us>
 <CAFY6G8fEa9sZ1V9cJpMOZAp6Wfa35U_rE0Y=eoQLS2G3OAqiAw@mail.gmail.com>
Comments: In-reply-to Matheus Alcantara <matheusssilv97@gmail.com>
	message dated "Tue, 27 May 2025 17:45:17 -0300"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1346939.1748381911.1@sss.pgh.pa.us>
Date: Tue, 27 May 2025 17:38:31 -0400
Message-ID: <1346940.1748381911@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1346940.1748381911%40sss.pgh.pa.us>
Precedence: bulk

Matheus Alcantara <matheusssilv97@gmail.com> writes:
> I think that we can delay the allocation a bit more. The
> dblink_security_check just use the rconn to pfree in case of a failure,
> so I think that we can remove this parameter and move the rconn
> allocation to the next if (connname) block. See attached as an example.

Yeah, I thought of that too.  But I think the point of the current
setup is to ensure we have the rconn block before we create the PGconn
object, because if we were to hit OOM after creating the connection,
we'd leak the PGconn, which would be quite bad.

Having said that, the idea that this sequence is OOM-safe is pretty
silly anyway, considering that createNewConnection does a pstrdup,
and creates a new hashtable entry which might require enlarging the
hashtable, and for that matter might even create the hashtable.
So maybe rather than continuing to adhere to a half-baked coding
rule, we need to think of some other way to do that.  Maybe it'd be
reasonable to create a hashtable entry with NULL rconn, and then
open the connection?  This'd require rejiggering the lookup
code to treat a hashtable entry with NULL rconn as not really
being there.  But there's not too many routines touching that
hashtable, so maybe it's not hard.

			regards, tom lane