Re: Emit a warning if the extension's GUC is set incorrectly

On 2021-12-22 02:23, Tom Lane wrote:
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>> At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato 
>> <Shinya11.Kato@oss.nttdata.com> wrote in
>>> We should use EmitWarningsOnPlaceholders when we use
>>> DefineCustomXXXVariable.
>>> I don't think there is any room for debate.
> 
>> Unfortunately, pltcl.c defines variables both in pltcl and pltclu but
>> the patch adds the warning only for pltclu.
> 
> Right.  The rest of 0001 looks fine, so pushed with that correction.
> 
> I concur that 0002 is unacceptable.  The result of it will be that
> a freshly-loaded extension will fail to hook into the system as it
> should, because it will fail to complete its initialization.
> That will cause user frustration and bug reports.
> 
> (BTW, I wonder if EmitWarningsOnPlaceholders should be using LOG
> rather than WARNING when it's running in the postmaster.  Use of
> WARNING is moderately likely to result in nothing getting printed
> at all.)
> 
> 0003 looks to me like it was superseded by 75d22069e.  I do not
> particularly like that patch though; it seems both inefficient
> and ill-designed.  0003 is adding a check in an equally bizarre
> place.  Why isn't add_placeholder_variable the place to deal
> with this?  Also, once we know that a prefix is reserved,
> why don't we throw an error not just a warning for attempts to
> set some unknown variable under that prefix?  We don't allow
> you to set unknown non-prefixed variables.
> 
> 			regards, tom lane

Thank you for pushing!
Thank you all for the reviews, I think I will take down 0002 and 0003.

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION