Uncleared result sets in describeOneTableDetails()
While I was poking around in src/bin/psql/describe.c, I noticed that
when the query for inherited tables is opened, the code checks whether
the result is valid and if not, it goes straight to the error_return,
without clearing result sets that may have been open at the time. See
line 1174 in revision 1.147.
Contrast with other instances of result sets being opened; if it
fails, the code first clears all previously opened result sets, then
goes to error_return (e.g., line 1138).
Is it crucial that result sets be cleared before going out of scope?
If so, this looks like it needs to be patched.
Regards,
BJ
"Brendan Jurd" <direvus@gmail.com> writes:
Is it crucial that result sets be cleared before going out of scope?
It sounds like it'd leak memory inside psql; but realistically that's
probably not an enormous problem for this usage. How much uglification
of the code are we talking about to fix it?
regards, tom lane
On 11/7/06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Brendan Jurd" <direvus@gmail.com> writes:
Is it crucial that result sets be cleared before going out of scope?
It sounds like it'd leak memory inside psql; but realistically that's
probably not an enormous problem for this usage. How much uglification
of the code are we talking about to fix it?
Should be just six extra lines (patch attached, untested). This isn't
really an uglification of the code, so much as bringing this
particular code segment into line with the existing ugliness standard
of the rest of the function. =)
It certainly isn't pretty. It's been a long time since I looked down
the barrel of a 'goto'. This sort of thing feels like it should be
dealt with by RAII or try/catch, but this is C we're talking about.
It's hard to do things gracefully.
Regards,
BJ
Attachments:
describe.c.diffapplication/octet-stream; name=describe.c.diffDownload
Index: src/bin/psql/describe.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/psql/describe.c,v
retrieving revision 1.147
diff -c -r1.147 describe.c
*** src/bin/psql/describe.c 9 Oct 2006 23:30:33 -0000 1.147
--- src/bin/psql/describe.c 7 Nov 2006 06:20:55 -0000
***************
*** 1171,1177 ****
--- 1171,1184 ----
result6 = PSQLexec(buf.data, false);
if (!result6)
+ {
+ PQclear(result1);
+ PQclear(result2);
+ PQclear(result3);
+ PQclear(result4);
goto error_return;
+ }
else
inherits_count = PQntuples(result6);
On Tue, 2006-11-07 at 17:56 +1100, Brendan Jurd wrote:
It certainly isn't pretty. It's been a long time since I looked down
the barrel of a 'goto'.
I don't think there's anything wrong with using "goto" for error
handling in this style. Personally, I think the main stylistic problem
is that the function is 600 lines long: it would be a lot cleaner if
refactored into smaller functions with smaller individual scopes.
-Neil
On Tue, 2006-11-07 at 17:56 +1100, Brendan Jurd wrote:
Should be just six extra lines (patch attached, untested).
Applied to HEAD, with an additional fix: you need to clear "result5" as
well. I didn't bother applying it to backbranches, on the grounds that a
memory leak in psql is not serious.
I think refactoring this function is the right long-term fix, but that
will have to wait for 8.2 to branch.
-Neil