[PATCHES] fix a bogus line in dynahash.c

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES] fix a bogus line in dynahash.c

Qingqing Zhou

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 */
  }



---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] fix a bogus line in dynahash.c

Neil Conway-2
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

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
      subscribe-nomail command to [hidden email] so that your
      message can get through to the mailing list cleanly
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] fix a bogus line in dynahash.c

Tom Lane-2
In reply to this post by Qingqing Zhou
"Qingqing Zhou" <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] fix a bogus line in dynahash.c

Qingqing Zhou
In reply to this post by Neil Conway-2

"Neil Conway" <[hidden email]>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



---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

               http://archives.postgresql.org
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] fix a bogus line in dynahash.c

Neil Conway-2
In reply to this post by Tom Lane-2
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

---------------------------(end of broadcast)---------------------------
TIP 1: subscribe and unsubscribe commands go to [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] fix a bogus line in dynahash.c

Tom Lane-2
Neil Conway <[hidden email]> 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

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES] fix a bogus line in dynahash.c

Qingqing Zhou
In reply to this post by Tom Lane-2

"Tom Lane" <[hidden email]> writes
> "Qingqing Zhou" <[hidden email]> 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.



---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

               http://www.postgresql.org/docs/faq