On 2021/12/22 3:33, Tom Lane wrote:
> I wrote:
>> 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.
> Concretely, I think we should do the attached, which removes much of
> 75d22069e and does the check at the point of placeholder creation.
> (After looking closer, add_placeholder_variable's caller is the
> function that is responsible for rejecting bad names, not
> add_placeholder_variable itself.)
>
> This also fixes an indisputable bug in 75d22069e, namely that it
> did prefix comparisons incorrectly and would throw warnings for
> cases it shouldn't. Reserving "plpgsql" shouldn't have the effect
> of reserving "pl" as well.
Thank you for creating the patch.
This is exactly what I wanted to create. It looks good to me.
> I'm tempted to propose that we also rename EmitWarningsOnPlaceholders
> to something like MarkGUCPrefixReserved, to more clearly reflect
> what it does now. (We could provide the old name as a macro alias
> to avoid breaking extensions needlessly.)
+1
--
Regards,
--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION