Re: Emit a warning if the extension's GUC is set incorrectly
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Shinya11.Kato@oss.nttdata.com, peter.eisentraut@enterprisedb.com,
masao.fujii@oss.nttdata.com, bharath.rupireddyforpostgres@gmail.com,
daniel@yesql.se, pgsql-hackers@postgresql.org
Date: 2021-12-21T17:23:25Z
Lists: pgsql-hackers
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
Commits
-
Disallow setting bogus GUCs within an extension's reserved namespace.
- 88103567cb8f 15.0 landed
-
Revert changes about warnings/errors for placeholders.
- cab5b9ab2c06 15.0 landed
-
Rename EmitWarningsOnPlaceholders() to MarkGUCPrefixReserved().
- 5609cc01c69b 15.0 landed
-
Rethink handling of settings with a prefix reserved by an extension.
- 2ed8a8cc5b63 15.0 landed
-
Add missing EmitWarningsOnPlaceholders() calls.
- 1fada5d81e67 15.0 landed
-
Add EmitWarningsOnPlaceholders calls to contrib modules that are likely to
- da2c1b8a27af 8.4.0 cited