On Thu, May 26, 2022 at 11:01:44PM -0400, Tom Lane wrote: > Michael Paquier <mich...@paquier.xyz> writes: >> FWIW, I would be fine to backpatch the NULL handling for short_desc, >> while treating the addition of nonnull as a HEAD-only change. > > Yeah, sounds about right to me. My guess is that we will need > a configure check for nonnull, but perhaps I'm wrong.
Makes sense. Here's a new patch set. 0001 is the part intended for back-patching, and 0002 is the rest (i.e., adding pg_attribute_nonnull()). I switched to using __has_attribute to discover whether nonnull was supported, as that seemed cleaner. I didn't see any need for a new configure check, but maybe I am missing something. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
>From c2760bf3db292fb55b6448f22eaeee38656083b5 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nathandboss...@gmail.com> Date: Fri, 27 May 2022 09:48:51 -0700 Subject: [PATCH v4 1/2] Properly handle NULL short descriptions for custom variables. If a NULL short description is specified in one of the DefineCustomXXXVariable functions, SHOW ALL will segfault. This change teaches SHOW ALL to properly handle NULL short descriptions. Back-patch to all supported versions. Reported by: Steve Chavez Author: Steve Chavez Reviewed by: Nathan Bossart, Michael Paquier, Andred Freund, Tom Lane Discussion: https://postgr.es/m/CAGRrpzY6hO-Kmykna_XvsTv8P2DshGiU6G3j8yGao4mk0CqjHA%40mail.gmail.com --- src/backend/utils/misc/guc.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 8e9b71375c..55d41ae7d6 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -9780,7 +9780,16 @@ ShowAllGUCConfig(DestReceiver *dest) isnull[1] = true; } - values[2] = PointerGetDatum(cstring_to_text(conf->short_desc)); + if (conf->short_desc) + { + values[2] = PointerGetDatum(cstring_to_text(conf->short_desc)); + isnull[2] = false; + } + else + { + values[2] = PointerGetDatum(NULL); + isnull[2] = true; + } /* send it to dest */ do_tup_output(tstate, values, isnull); @@ -9792,7 +9801,8 @@ ShowAllGUCConfig(DestReceiver *dest) pfree(setting); pfree(DatumGetPointer(values[1])); } - pfree(DatumGetPointer(values[2])); + if (conf->short_desc) + pfree(DatumGetPointer(values[2])); } end_tup_output(tstate); @@ -10002,7 +10012,7 @@ GetConfigOptionByNum(int varnum, const char **values, bool *noshow) values[3] = _(config_group_names[conf->group]); /* short_desc */ - values[4] = _(conf->short_desc); + values[4] = conf->short_desc != NULL ? _(conf->short_desc) : NULL; /* extra_desc */ values[5] = conf->long_desc != NULL ? _(conf->long_desc) : NULL; -- 2.25.1
>From bd05e7092f45f416476cc02a21f8d0bb069a313f Mon Sep 17 00:00:00 2001 From: Nathan Bossart <nathandboss...@gmail.com> Date: Fri, 27 May 2022 10:10:09 -0700 Subject: [PATCH v4 2/2] Introduce pg_attribute_nonnull() and use it for DefineCustomXXXVariable(). pg_attribute_nonnull() can be used to generate compiler warnings when a function is called with the specified arguments set to NULL. An empty argument list indicates that all pointer arguments must not be NULL. pg_attribute_nonnull() only works for compilers that support the nonnull function attribute. If nonnull is not supported, pg_attribute_nonnull() has no effect. In addition to introducing pg_attribute_nonnull(), this change uses it for the DefineCustomXXXVariable() functions to generate warnings when the "name" and "valueAddr" arguments are set to NULL. Idea by: Andres Freund Author: Nathan Bossart Reviewed by: Michael Paquier, Tom Lane Discussion: https://postgr.es/m/20220525061739.ur7x535vtzyzkmqo%40alap3.anarazel.de --- src/include/c.h | 11 +++++++++++ src/include/utils/guc.h | 10 +++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/include/c.h b/src/include/c.h index 4f16e589b3..0c64557c62 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -144,6 +144,17 @@ #define pg_attribute_no_sanitize_alignment() #endif +/* + * pg_attribute_nonnull means the compiler should warn if the function is called + * with the listed arguments set to NULL. If no arguments are listed, the + * compiler should warn if any pointer arguments are set to NULL. + */ +#if __has_attribute (nonnull) +#define pg_attribute_nonnull(...) __attribute__((nonnull(__VA_ARGS__))) +#else +#define pg_attribute_nonnull(...) +#endif + /* * Append PG_USED_FOR_ASSERTS_ONLY to definitions of variables that are only * used in assert-enabled builds, to avoid compiler warnings about unused diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index efcbad7842..be691c5e9c 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -303,7 +303,7 @@ extern void DefineCustomBoolVariable(const char *name, int flags, GucBoolCheckHook check_hook, GucBoolAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomIntVariable(const char *name, const char *short_desc, @@ -316,7 +316,7 @@ extern void DefineCustomIntVariable(const char *name, int flags, GucIntCheckHook check_hook, GucIntAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomRealVariable(const char *name, const char *short_desc, @@ -329,7 +329,7 @@ extern void DefineCustomRealVariable(const char *name, int flags, GucRealCheckHook check_hook, GucRealAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomStringVariable(const char *name, const char *short_desc, @@ -340,7 +340,7 @@ extern void DefineCustomStringVariable(const char *name, int flags, GucStringCheckHook check_hook, GucStringAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void DefineCustomEnumVariable(const char *name, const char *short_desc, @@ -352,7 +352,7 @@ extern void DefineCustomEnumVariable(const char *name, int flags, GucEnumCheckHook check_hook, GucEnumAssignHook assign_hook, - GucShowHook show_hook); + GucShowHook show_hook) pg_attribute_nonnull(1, 4); extern void MarkGUCPrefixReserved(const char *className); -- 2.25.1
