Thread: Error handling for ShmemInitStruct and ShmemInitHash

The functions ShmemInitStruct and ShmemInitHash will return NULL on
certain failure conditions, apparently on the grounds that their caller
can print a more useful error message than they can.  A quick survey
shows that about half the callers aren't remembering to check for NULL,
and none of the other half are printing messages that are more useful
than "out of shared memory" (which isn't even necessarily correct).

I think that this is pretty error-prone, and that considering that
PG hackers are accustomed to not checking palloc() results, it's
inevitable that we'll make the same mistake in future if we leave
this API as it is.  I suggest making these functions throw
their own errors rather than returning NULL on failure, and removing
the redundant error reports from the callers that have 'em.

Comments?
        regards, tom lane

Tom Lane wrote:
> The functions ShmemInitStruct and ShmemInitHash will return NULL on
> certain failure conditions, apparently on the grounds that their caller
> can print a more useful error message than they can.  A quick survey
> shows that about half the callers aren't remembering to check for NULL,
> and none of the other half are printing messages that are more useful
> than "out of shared memory" (which isn't even necessarily correct).
> 
> I think that this is pretty error-prone, and that considering that
> PG hackers are accustomed to not checking palloc() results, it's
> inevitable that we'll make the same mistake in future if we leave
> this API as it is.  I suggest making these functions throw
> their own errors rather than returning NULL on failure, and removing
> the redundant error reports from the callers that have 'em.

+1. I was just annoyed by this when working on the known-assigned-xids
hash -> sorted-array patch.

--  Heikki Linnakangas EnterpriseDB   http://www.enterprisedb.com

Tom Lane <tgl@sss.pgh.pa.us> wrote:
> none of the other half are printing messages that are more useful
> than "out of shared memory" (which isn't even necessarily
> correct).
I think the messages in the locking area are a bit more useful than
"out of shared memory", but it would be trivial to build the
equivalent message in the ShmemInitHash function, based on the first
parameter.   LockMethodProcLockHash = ShmemInitHash("PROCLOCK hash",
init_table_size,                                         max_table_size,
&info,                                         hash_flags);   if (!LockMethodProcLockHash)       elog(FATAL, "could not
initializeproclock hash table");
 
Presumably the ShmemInitHash function could add other information
which would make the message *more* useful.  (Perhaps other
parameter information or maybe even the actual *cause* of the
failure.)

> I suggest making these functions throw their own errors rather
> than returning NULL on failure, and removing the redundant error
> reports from the callers that have 'em.
+1  It would be low priority if the return value was currently being
consistently checked for NULL; but since that's not the case we have
to do something, and what you suggest sounds best, long term.
-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> none of the other half are printing messages that are more useful
>> than "out of shared memory" (which isn't even necessarily
>> correct).
> I think the messages in the locking area are a bit more useful than
> "out of shared memory", but it would be trivial to build the
> equivalent message in the ShmemInitHash function, based on the first
> parameter.

Right, I was intending to include the "name" parameter in the messages.
This would actually represent an improvement in message quality in a lot
of the cases.
        regards, tom lane