Thread: Small psql memory fix
The attached tiny patch fixes a small leak in psql's \gset command and simplifies memory freeing in two places. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> The attached tiny patch fixes a small leak in psql's \gset command and
> simplifies memory freeing in two places.
The first and third hunks look to me like they introduce memory
leaks, not fix them. The second hunk is probably safe enough,
although I'm not sure the case can actually occur --- gset should
free the prefix before any new backslash command can be executed.
regards, tom lane
On Fri, Feb 14, 2014 at 11:28:49PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > The attached tiny patch fixes a small leak in psql's \gset command and > > simplifies memory freeing in two places. > > The first and third hunks look to me like they introduce memory > leaks, not fix them. The second hunk is probably safe enough, The first and third just move the free into blocks where we have already tested the value is not null. It just more clearly matches the surrounding code. Do you see something different? > although I'm not sure the case can actually occur --- gset should > free the prefix before any new backslash command can be executed. Oh, interesting idea. Updated patch attached. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Attachment
Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Feb 14, 2014 at 11:28:49PM -0500, Tom Lane wrote:
>> The first and third hunks look to me like they introduce memory
>> leaks, not fix them. The second hunk is probably safe enough,
> The first and third just move the free into blocks where we have already
> tested the value is not null. It just more clearly matches the
> surrounding code. Do you see something different?
[ looks closer... ] OK, they're not actually broken, but I question
whether they're improvements. The existing coding is clear enough
that the result of psql_scan_slash_option will be freed. What you're
doing is conflating that requirement with some other processing.
>> although I'm not sure the case can actually occur --- gset should
>> free the prefix before any new backslash command can be executed.
> Oh, interesting idea. Updated patch attached.
I don't think that's an improvement at all. My point was that I
didn't think gset_prefix would ever be set at entry to this code,
because the setting should never survive for more than one backslash
command execution cycle.
If you want to add probably-useless code to free it, go ahead, but
this isn't a clearer way to do that than your previous version.
regards, tom lane
On Fri, Feb 14, 2014 at 11:52:42PM -0500, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Fri, Feb 14, 2014 at 11:28:49PM -0500, Tom Lane wrote: > >> The first and third hunks look to me like they introduce memory > >> leaks, not fix them. The second hunk is probably safe enough, > > > The first and third just move the free into blocks where we have already > > tested the value is not null. It just more clearly matches the > > surrounding code. Do you see something different? > > [ looks closer... ] OK, they're not actually broken, but I question > whether they're improvements. The existing coding is clear enough > that the result of psql_scan_slash_option will be freed. What you're > doing is conflating that requirement with some other processing. > > >> although I'm not sure the case can actually occur --- gset should > >> free the prefix before any new backslash command can be executed. > > > Oh, interesting idea. Updated patch attached. > > I don't think that's an improvement at all. My point was that I > didn't think gset_prefix would ever be set at entry to this code, > because the setting should never survive for more than one backslash > command execution cycle. > > If you want to add probably-useless code to free it, go ahead, but > this isn't a clearer way to do that than your previous version. If you think this code isn't helping, I will just discard it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +