Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-07-01 Thread Nathan Bossart
On Sat, Jul 02, 2022 at 12:33:28PM +0900, Michael Paquier wrote: > Now that v16 is open for business, I have been able to look at it > again, and applied the patch on HEAD. My apologies for the wait. Thanks! -- Nathan Bossart Amazon Web Services: https://aws.amazon.com

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-07-01 Thread Michael Paquier
On Sat, May 28, 2022 at 05:50:18AM -0700, Nathan Bossart wrote: > Yeah, I see no reason that this should go into v15. I created a new > commitfest entry so that this isn't forgotten: > > https://commitfest.postgresql.org/38/3655/ > > And I've reposted 0002 here so that we get some cfbot co

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-28 Thread Nathan Bossart
On Sat, May 28, 2022 at 12:26:34PM +0900, Michael Paquier wrote: > On Fri, May 27, 2022 at 10:43:17AM -0700, Nathan Bossart wrote: >> 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 u

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-27 Thread Tom Lane
Michael Paquier writes: > And I've learnt today that we enforce a definition of __has_attribute > at the top of c.h, and that we already rely on that. So I agree that > what you are doing in 0002 should be enough. Should we wait until 16~ > opens for business though? I don't see a strong argume

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-27 Thread Michael Paquier
On Fri, May 27, 2022 at 10:43:17AM -0700, Nathan Bossart wrote: > 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 Okay, I have loo

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-27 Thread Nathan Bossart
On Thu, May 26, 2022 at 11:01:44PM -0400, Tom Lane wrote: > Michael Paquier 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 confi

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-26 Thread Tom Lane
Michael Paquier 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.

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-26 Thread Michael Paquier
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

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-26 Thread Nathan Bossart
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(...)

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-25 Thread Steve Chavez
Thank you for the reviews Nathan, Michael. I agree with handling NULL in ShowAllGUCConfig() instead. I've attached the updated patch. -- Steve Chavez Engineering at https://supabase.com/ On Tue, 24 May 2022 at 20:21, Michael Paquier wrote: > On Tue, May 24, 2022 at 11:41:49AM -0700, Nathan Bo

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-24 Thread Andres Freund
Hi, On 2022-05-24 11:41:49 -0700, Nathan Bossart wrote: > On Mon, May 23, 2022 at 11:39:16PM -0500, Steve Chavez wrote: > > The DefineCustomStringVariable function(or any > > other DefineCustomXXXVariable) has a short_desc parameter that can be > > NULL and it's not apparent that this will lead to

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-24 Thread Michael Paquier
On Wed, May 25, 2022 at 12:05:55AM -0500, Steve Chavez wrote: > Thank you for the reviews Nathan, Michael. > > I agree with handling NULL in ShowAllGUCConfig() instead. > > I've attached the updated patch. Shouldn't the same check as extra_desc be done in GetConfigOptionByNum() for short_desc (p

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-24 Thread Michael Paquier
On Tue, May 24, 2022 at 11:41:49AM -0700, Nathan Bossart wrote: > I would actually ERROR on this so that we aren't relying on > --enable-cassert builds to catch it. That being said, if there's no strong > reason to enforce that a short description be provided, then why not adjust > ShowAllGUCConfi

Re: Assert name/short_desc to prevent SHOW ALL segfault

2022-05-24 Thread Nathan Bossart
On Mon, May 23, 2022 at 11:39:16PM -0500, Steve Chavez wrote: > The DefineCustomStringVariable function(or any > other DefineCustomXXXVariable) has a short_desc parameter that can be > NULL and it's not apparent that this will lead to a segfault when SHOW ALL > is used. > This happens because the S