Thread: Re: [Util] Warn and Remove Invalid GUCs
Hi,
On Thu, May 22, 2025 at 2:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Shaik Mohammad Mujeeb <mujeeb.sk@zohocorp.com> writes:
> Currently, if there's a typo in an extension name while adding a GUC to postgresql.conf, PostgreSQL server starts up silently without any warning. This can be misleading, as one might assume the configuration has been correctly applied, when in fact the value hasn’t been set due to the typo.
Well, yeah, because the core server has no way to identify bogus
extension GUCs if the relevant extension isn't loaded. We do
already complain about such things at extension load time.
the extension is loaded and then i entered the bogus extension GUC into postgresql.conf and restarted, i did not observe any complain/warning .
> To improve this experience, I’m proposing a patch that issues a
> warning for such invalid GUC entries.
How will you know they are invalid? All I see in the patch is
a syntactic check, which looks quite redundant with
assignable_custom_variable_name().
after the extension is loaded, MarkGUCPrefixReserved() appends reserved_class_prefix with extension name ,so this patch has code to check if the the GUC's prefix from the postgresql.conf is in the reserved_class_prefix or not ,if not it invalidates and throws warning.Please correct me if i am wrong.
+static bool
+has_valid_class_prefix(const char *name){
+ /* If there's no separator, it can't be a custom variable */
+ const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR);
+ size_t class_len = sep - name;
+ ListCell *lc;
+ foreach(lc, reserved_class_prefix)
+ {
+ const char *rcprefix = lfirst(lc);
+
+ if (strlen(rcprefix) == class_len &&
+ strncmp(name, rcprefix, class_len) == 0)
+ {
+ return true;
+ }
+ }
+
+ return false;
+}
+void WarnAndRemoveInvalidGUCs(){
+ HASH_SEQ_STATUS status;
+ GUCHashEntry *hentry;
+
+ /* Warn and remove invalid placeholders. */
+ hash_seq_init(&status, guc_hashtab);
+ while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL)
+ {
+ struct config_generic *var = hentry->gucvar;
+
+ if((var->flags & GUC_CUSTOM_PLACEHOLDER) != 0 && !has_valid_class_prefix(var->name)){
+ ereport(WARNING,
+ (errcode(ERRCODE_INVALID_NAME),
+ errmsg("invalid configuration parameter name \"%s\", removing it",
+ var->name),
+ errdetail("\"%s\" doesn't has a reserved prefix.",
+ var->name)));
+ remove_gucvar(var);
+ }
+ }
+}
+
Srinath Reddy Sadipiralla <srinath2133@gmail.com> writes:
> the extension is loaded and then i entered the bogus extension GUC into
> postgresql.conf and restarted, i did not observe any complain/warning .
Were you looking in the right place? I experimented with adding
shared_preload_libraries = 'plpgsql' # (change requires restart)
plpgsql.bogus = 1
to postgresql.conf. Restarting the server, I see in the
postmaster log:
2025-05-22 11:16:45.724 EDT [1526138] WARNING: invalid configuration parameter name "plpgsql.bogus", removing it
2025-05-22 11:16:45.724 EDT [1526138] DETAIL: "plpgsql" is now a reserved prefix.
2025-05-22 11:16:45.728 EDT [1526138] LOG: starting PostgreSQL 18beta1 on x86_64-pc-linux-gnu, compiled by gcc (GCC)
8.5.020210514 (Red Hat 8.5.0-26), 64-bit
... etc etc ...
(I also tried the other order of these settings, to see if that made
any difference, but it didn't.) I also tried
session_preload_libraries = 'plpgsql'
plpgsql.bogus = 1
and then the complaint comes out during any session start:
$ psql
WARNING: invalid configuration parameter name "plpgsql.bogus", removing it
DETAIL: "plpgsql" is now a reserved prefix.
psql (18beta1)
Type "help" for help.
That's all operating as intended, and I'm not seeing how the proposed
patch isn't completely redundant with it.
regards, tom lane
On Thu, May 22, 2025 at 9:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Srinath Reddy Sadipiralla <srinath2133@gmail.com> writes:
> the extension is loaded and then i entered the bogus extension GUC into
> postgresql.conf and restarted, i did not observe any complain/warning .
Were you looking in the right place? I experimented with adding
shared_preload_libraries = 'plpgsql' # (change requires restart)
plpgsql.bogus = 1
to postgresql.conf. Restarting the server, I see in the
postmaster log:
2025-05-22 11:16:45.724 EDT [1526138] WARNING: invalid configuration parameter name "plpgsql.bogus", removing it
2025-05-22 11:16:45.724 EDT [1526138] DETAIL: "plpgsql" is now a reserved prefix.
2025-05-22 11:16:45.728 EDT [1526138] LOG: starting PostgreSQL 18beta1 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-26), 64-bit
... etc etc ...
Tom, the problem is when the prefix is a typo ,my bad i should have specified as bogus prefix instead of bogus GUC ,can you please try again with
"lpgsql.bogus = 1" .
Srinath Reddy Sadipiralla <srinath2133@gmail.com> writes:
> Tom, the problem is when the prefix is a typo ,my bad i should have
> specified as bogus prefix instead of bogus GUC ,can you please try again
> with
> "lpgsql.bogus = 1" .
[ shrug... ] How do you know that's a bogus prefix? It could
perfectly well be a fully valid setting for an extension that
the installation doesn't choose to preload.
regards, tom lane
On Thu, May 22, 2025 at 12:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "lpgsql.bogus = 1" .
[ shrug... ] How do you know that's a bogus prefix? It could perfectly well be a fully valid setting for an extension that
the installation doesn't choose to preload.
Well, we do have ways to view all *potential* extensions. I find myself more sympathetic to the OP than the others here, as I think it's a good idea to display a warning for a potentially misspelled GUC prefix (but do nothing else - certainly not remove it!). Will such a warning be seen? Perhaps not, but that's a risk any warning message has to face. And obviously there could be false positives if an extension decides to name their GUCs something other than pg_available_extensions()->name but hopefully that's a rare case (and shame on them if so.)
Cheers,
Greg
--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support
On Thu, May 22, 2025 at 1:20 PM Greg Sabino Mullane <htamfids@gmail.com> wrote:
On Thu, May 22, 2025 at 12:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:> "lpgsql.bogus = 1" .
[ shrug... ] How do you know that's a bogus prefix? It could perfectly well be a fully valid setting for an extension that
the installation doesn't choose to preload.Well, we do have ways to view all *potential* extensions. I find myself more sympathetic to the OP than the others here, as I think it's a good idea to display a warning for a potentially misspelled GUC prefix (but do nothing else - certainly not remove it!). Will such a warning be seen? Perhaps not, but that's a risk any warning message has to face. And obviously there could be false positives if an extension decides to name their GUCs something other than pg_available_extensions()->name but hopefully that's a rare case (and shame on them if so.)
We don't have proper server variables for applications and non-extensions to use in things like RLS so choosing a non-extension-registered prefix is a perfectly valid thing to do. Maybe it would seldom be done in the config file, and be nominally reserved for SET/set_config, but it is still valid.
IMO, an improvement in this area would revolve around not making people type out setting names manually at all. If they don't type them they cannot typo them. Again, this also only improves the config file setup, but why not add pre-made configuration samples in a subdirectory that people can either copy/paste individually; or setup a default include_dir directive for contrib and people can place the "sample" files into the live "contrib.d" directory making them no longer just samples but live configuration that can then be edited without having to type names - just remove comment symbols (maybe) and change values.
David J.