On 2021-12-22 02:23, Tom Lane wrote:
Kyotaro Horiguchi <horikyota....@gmail.com> writes:
At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato
<shinya11.k...@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
