hash_create(): check return code
hash_create() can return a NULL pointer if an error occurs (principally,
when we're out of memory). Some of the call sites checked for this
condition, but some did not. This patch fixes the remaining call sites
to check the return value, and ereport(ERROR) if it is NULL.
I'm not really sure whether this is the correct fix, but it certainly
seems wrong to be doing the check in some places and not in others.
Another approach would be to elog(ERROR) when an error occurs in
hash_create(), which would be fine except there might be some
circumstances in which hash_create() is invoked but elog(ERROR) is not
setup yet.
Comments?
-Neil
(Patch attached as a unified diff -- apologies for that, but I'm playing
with a new version control tool that most easily emits unified diffs.
http://www.venge.net/monotone/, if you're curious.)
Attachments:
hash_create_oom-1.patchtext/x-patch; charset=ISO-8859-1; name=hash_create_oom-1.patchDownload+48-0
Neil Conway <neilc@samurai.com> writes:
I'm not really sure whether this is the correct fix, but it certainly
seems wrong to be doing the check in some places and not in others.
Another approach would be to elog(ERROR) when an error occurs in
hash_create(), which would be fine except there might be some
circumstances in which hash_create() is invoked but elog(ERROR) is not
setup yet.
There are no places where hash_create is called before elog() is
functional. I'd go for putting the error into hash_create, I think,
because I can't imagine any caller not wanting to error out. (If
there is any such caller, it can always catch it with PG_TRY.)
regards, tom lane
On Fri, 2004-10-22 at 16:13, Tom Lane wrote:
There are no places where hash_create is called before elog() is
functional.
Well, it's invoked from the statistics collector, which avoids doing
elog(ERROR) for some reason. But my guess is that it should be workable
to get elog(ERROR) / elog(FATAL) working in the statistics collector,
and it will mean a cleanup of existing code as well (which laboriously
invokes elog(LOG) followed by exit(1)). I'm working on that now...
-Neil
Neil Conway <neilc@samurai.com> writes:
On Fri, 2004-10-22 at 16:13, Tom Lane wrote:
There are no places where hash_create is called before elog() is
functional.
Well, it's invoked from the statistics collector, which avoids doing
elog(ERROR) for some reason.
With all due respect to Jan, that coding seems 100% bogus. elog(ERROR)
will work (it had better, because pgstat.c certainly calls routines that
might do it) and the insistence on using exit() rather than proc_exit()
is just plain wrong anyway.
Note that there is really no difference between elog(ERROR) and
elog(FATAL) in this context, since pgstat doesn't have an outer
sigsetjmp call.
regards, tom lane
Tom Lane wrote:
With all due respect to Jan, that coding seems 100% bogus. elog(ERROR)
will work (it had better, because pgstat.c certainly calls routines that
might do it) and the insistence on using exit() rather than proc_exit()
is just plain wrong anyway.
Attached is a patch that makes the following cleanups:
- use elog(ERROR) in pgstat.c to indicate fatal errors, rather than
elog(LOG) followed by exit(1)
- use elog(ERROR) to indicate an error in hash_create() rather than
returning a NULL pointer
- adjust callers of hash_create() to assume the return value is non-NULL
There was one case in pgstat.c where I had to wrap the hash_create()
call in a PG_TRY() block to ensure a file handle is closed (this code
might be invoked by a regular backend it appears, so elog(ERROR) won't
necessarily close the file handle).
Barring any objections, I'll apply this to HEAD on Monday.
-Neil
Attachments:
hash_create-3.patchtext/plain; name=hash_create-3.patch; x-mac-creator=0; x-mac-type=0Download+40-166
Neil Conway <neilc@samurai.com> writes:
There was one case in pgstat.c where I had to wrap the hash_create()
call in a PG_TRY() block to ensure a file handle is closed (this code
might be invoked by a regular backend it appears, so elog(ERROR) won't
necessarily close the file handle).
A better solution is to use AllocateFile/FreeFile; I'm not 100%
certain that that works in the pgstat context, but I think it should.
regards, tom lane
On Mon, 2004-10-25 at 00:25, Tom Lane wrote:
A better solution is to use AllocateFile/FreeFile; I'm not 100%
certain that that works in the pgstat context, but I think it should.
I applied the patch I posted earlier to HEAD (post beta4). I'll look at
doing this separately.
-Neil