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 initialize proclock 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