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
