Re: Assert name/short_desc to prevent SHOW ALL segfault

On Thu, May 26, 2022 at 02:45:50PM -0700, Nathan Bossart wrote:
> On Tue, May 24, 2022 at 11:17:39PM -0700, Andres Freund wrote:
>> On 2022-05-24 11:41:49 -0700, Nathan Bossart wrote:
>>> I would actually ERROR on this so that we aren't relying on
>>> --enable-cassert builds to catch it.
>> 
>> How about adding pg_nonnull(...) (ending up as __attribute__((nonnull(...))?
>> Then code passing NULLs would get compiler warnings? It'd be useful in quite a
>> few more places.
> 
> I attached an attempt at this for the "name" and "valueAddr" arguments for
> the DefineCustomXXXVariable functions.  It looked like nonnull was
> supported by GCC and Clang, but I haven't looked too closely to see whether
> we need version checks as well.

Adding pg_attribute_nonnull() is a neat idea.  It looks like nonnull
was added in GCC 4.0, and I can see it first appearing in clang 3.7.
The only buildfarm member claiming to use a version of clang older
than that is dangomushi, aka 3.6.0.  That's my machine, and the
information on the buildfarm website is outdated as the version of
clang available there is 13.0 as of today.  I think that we are going
to run into problems with buildfarm member protosciurus, running
Solaris 10 under sparc? It claims to use gcc 3.4.3.  I would worry
also about prairiedog, we've hard our share of compatibility issues
with this one in the past.  It claims to use gcc 4.0.1 but Apple has
its own idea of versioning, and that's our oldest macos member.

>>> That being said, if there's no strong reason to enforce that a short
>>> description be provided, then why not adjust ShowAllGUCConfig() to set that
>>> column to NULL when short_desc is missing?
>> 
>> There's a bunch more places that'd need to be adjusted, if we go that way. I
>> don't really have an opinion on it.
> 
> I looked around and didn't see anywhere else obvious that needed adjustment
> besides what Michael pointed out (3ac7d024).  Am I missing
> something?

I don't think so.  help_config.c is able to handle NULL for the short
description already. 

FWIW, I would be fine to backpatch the NULL handling for short_desc,
while treating the addition of nonnull as a HEAD-only change.
--
Michael