Robert Haas wrote:
> I don't see much advantage in changing these to asserts - in a
> debug build, that will promote ERROR to PANIC; whereas in a
> production build, they'll cause a random failure somewhere
> downstream.
The reason Assert is appropriate is that it is *impossible* to hit
that condition right now for two independent reasons. Both would
need to be broken by someone doing clumsy programming changes for
these assertions to fire.
(1) Both assertions follow the attempt to get record from the
structure allocated with this call:
SerializableXidHash = ShmemInitHash("SERIALIZABLEXID hash", max_table_size,
max_table_size, &info,
hash_flags);
Note that it is allocated at its maximum size, and that one of these
records can't exist without a corresponding SERIALIZABLEXACT
structure, and these are allocated out of a fixed-sized area which is
allocated based on the same max_table_size value.
(2) The preceding call to hash_search is made with a HASHACTION of
HASH_ENTER, not HASH_ENTER_NULL. So it can't return without a valid
pointer. It will ereport at the ERROR level rather than do that.
So I'm not sure the Assert is even warranted, versus just doing
nothing; but the ereport is certainly pointless.
> The HASH_ENTER to HASH_ENTER_NULL changes look like they might be
> needed, though.
That would provide a hint pointing toward the GUC which controls
allocation space for the structure we failed to get. That seems
better than the generic message we're now getting. If we don't
change them to HASH_ENTER_NULL we can get rid of our ereport since it
will never be hit.
Of course I'm sure Tom will clean such stuff up if it comes to him in
the current form; it just seems nicer to clean up known issues that
surface from testing before it gets to him, where possible.
-Kevin
