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. +
Attachments:
psql.difftext/x-diff; charset=us-asciiDownload+4-2
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
Attachments:
psql.difftext/x-diff; charset=us-asciiDownload+10-8
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers