Thread: fix a bogus line in dynahash.c
Change elog(ERROR) to Assert(false) for two reasons:
(1) "unrecognized hash action code" could hardly really happen;
(2) even if it could happen, elog(ERROR) won't save us since in many places
we have to check the return code of hash_search() and decide the error
level.
On a separate matter, can anyone please explain me how this piece of code
works:
/* no free elements. allocate another chunk of buckets */
if (!element_alloc(hashp, HASHELEMENT_ALLOC_INCR))
return NULL; /* out of memory */
element_alloc() in fact uses MemoryContextAlloc() stuff. So if
element_alloc() fails, it actually elog(ERROR) itself and jump out of our
control, and we could never get a chance to check its returned value. So
many places like this:
if (!hash_search(... HASH_ENTER ...))
elog( ...)
could never happen?
Regards,
Qingqing
Index: dynahash.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/hash/dynahash.c,v
retrieving revision 1.60
diff -c -r1.60 dynahash.c
*** dynahash.c 16 May 2005 00:19:04 -0000 1.60
--- dynahash.c 25 May 2005 01:57:42 -0000
***************
*** 680,687 ****
return (void *) ELEMENTKEY(currBucket);
}
! elog(ERROR, "unrecognized hash action code: %d", (int) action);
!
return NULL; /* keep compiler quiet */
}
--- 680,688 ----
return (void *) ELEMENTKEY(currBucket);
}
! /* Should never be here */
! Assert(false);
!
return NULL; /* keep compiler quiet */
}
Qingqing Zhou wrote: > On a separate matter, can anyone please explain me how this piece of code > works: > > /* no free elements. allocate another chunk of buckets */ > if (!element_alloc(hashp, HASHELEMENT_ALLOC_INCR)) > return NULL; /* out of memory */ > > element_alloc() in fact uses MemoryContextAlloc() stuff. Well, element_alloc() uses the hash table's alloc function pointer. In theory, that could be malloc() or anything else, although I notice this abstraction is not consistently maintained (e.g. dir_realloc assumes pfree() is sufficient to free an allocation). I think it would be a good idea to change dynahash.c to assume that the hash table's allocation function will elog(ERROR) on out-of-memory, so its return value will always be non-NULL. This would allow for a bunch of code to be simplified. -Neil
"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> Change elog(ERROR) to Assert(false) for two reasons:
No. Remember that in most installations Assert() is a no-op.
> (2) even if it could happen, elog(ERROR) won't save us since in many places
> we have to check the return code of hash_search() and decide the error
> level.
You do know that elog(ERROR) doesn't return control to the caller?
> On a separate matter, can anyone please explain me how this piece of code
> works:
> /* no free elements. allocate another chunk of buckets */
> if (!element_alloc(hashp, HASHELEMENT_ALLOC_INCR))
> return NULL; /* out of memory */
That test is a no-op in the case where hashp->alloc in fact points to
palloc. But it doesn't always point there --- see shmem_alloc.
regards, tom lane
Tom Lane wrote: > That test is a no-op in the case where hashp->alloc in fact points to > palloc. But it doesn't always point there --- see shmem_alloc. Perhaps it would be a net win to change ShmemAlloc() to elog(ERROR) on out-of-memory? A fair few of the ShmemAlloc() call sites don't bother to check the return value anyway, and a few more just elog(ERROR). For the few cases when we do need to do some cleanup, PG_TRY() could be used, or we could just provide a variant of ShmemAlloc() that returns NULL on OOM and could be used when error recovery is required. -Neil
"Neil Conway" <neilc@samurai.com>writes
> Well, element_alloc() uses the hash table's alloc function pointer. In
> theory, that could be malloc() or anything else, although I notice this
> abstraction is not consistently maintained (e.g. dir_realloc assumes
> pfree() is sufficient to free an allocation).
>
Yes, in theory it could do so. But in fact element_alloc() uses
DynaHashAlloc() for ordinary hash table or uses ShmemAlloc() for shared
memory case. The problem is that both of these could elog(ERROR) themselves.
> I think it would be a good idea to change dynahash.c to assume that the
> hash table's allocation function will elog(ERROR) on out-of-memory,
DynaHashAlloc() looks like this. ShmemAlloc() just elog(WARNING), we could
revise it ... but if you do so, how to rewrite
if (!hash_search(HASH_ENTER)) /* for example: RememberFsyncRequest() */
elog(FATAL/PANIC);
Use critical_section?
Regards,
Qingqing
Neil Conway <neilc@samurai.com> writes:
> Tom Lane wrote:
>> That test is a no-op in the case where hashp->alloc in fact points to
>> palloc. But it doesn't always point there --- see shmem_alloc.
> Perhaps it would be a net win to change ShmemAlloc() to elog(ERROR) on
> out-of-memory?
Possibly. I haven't looked through the call sites to make an estimate
on whether this would be worthwhile or not. One thing you'd have to
look at carefully is whether any of the call sites are inside critical
sections --- if so, an elog(ERROR) inside ShmemAlloc would become
elog(PANIC), which might be more severe than is warranted.
> A fair few of the ShmemAlloc() call sites don't bother to
> check the return value anyway,
Really? But it wouldn't surprise me that much --- the vast majority of
the individual calls are during postmaster initialization, and could not
possibly fail unless the initial-shmem-space-request calculation is
wrong. The only case that can actually fail on-the-fly is allocation of
a new entry in a shared-memory hash table, and we know that case is
checked.
regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes > "Qingqing Zhou" <zhouqq@cs.toronto.edu> writes: > > No. Remember that in most installations Assert() is a no-op. > Well, I still suggest to change it :( The only chance elog(ERROR, "unrecognized hash action code") could be triggered is the *unbelievable* programming typo.