Re: Emit a warning if the extension's GUC is set incorrectly

On Tue, Nov 16, 2021 at 2:50 PM Shinya Kato
<Shinya11(dot)Kato(at)oss(dot)nttdata(dot)com> wrote:
> > I'm not sure if we have "how to create an extension/a bg worker?" in
> > the core docs, if we have, it is a good idea to make note of using
> > EmitWarningsOnPlaceholders so that it will not be missed in the future
> > modules.
>
> I cannot find any such docs, so I add a comment to
> src/backend/utils/misc/guc.c.
>
> > I observed an odd behaviour:
> > 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
> > 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
> > contrib module, I created the extension, have seen the following
> > warning:
> > 2021-11-15 06:02:31.198 UTC [2018111] WARNING: unrecognized
> > configuration parameter "postgres_fdw.XXX"
> > 3) I further did, "alter system set
> > postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
> > pg_reload_conf();", it silently accepts.
> >
> > postgres=# create extension postgres_fdw ;
> > WARNING: unrecognized configuration parameter "postgres_fdw.XXX"
> > CREATE EXTENSION
> > postgres=# alter system set
> > postgres_fdw.XXX='I_further_messed_up_conf_file';
> > ALTER SYSTEM
> > postgres=# select pg_reload_conf();
> > pg_reload_conf
> > ----------------
> > t
> > (1 row)
> >
> > My point is, why should the "create extension" succeed with a WARNING
> > when a wrong parameter related to it is set in the postgresql.conf
> > file so that we don't see further problems as shown in (3). I think
> > EmitWarningsOnPlaceholders should emit an error so that the extension
> > will not get created in the first place if a wrong configuration
> > parameter is set in the postgresql.conf file. Many times, users will
> > not have access to server logs so it is good to fail the "create
> > extension" statement.
> >
> > Thoughts?
> >
> > postgres=# create extension postgres_fdw ;
> > ERROR: unrecognized configuration parameter "postgres_fdw.XXX"
> > postgres=#
>
> I confirmed it too, and I think that's definitely a problem.
> I also thought it would be a good idea to emit an ERROR as well as when
> an invalid normal GUC is set.
> I didn't change the function name because it would affect many third
> party extensions.

For backward compatibility you can retain the function
EmitWarningsOnPlaceholders as-is and have another similar function
that emits an error:

In guc.c, have something like below:
static void
check_conf_params(const char *className, bool emit_error)
{
/* have the existing code of EmitWarningsOnPlaceholders here */
ereport(emit_error ? ERROR : WARNING,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("unrecognized configuration parameter \"%s\"",
var->name)));
}

void
EmitErrorOnPlaceholders(const char *className)
{
check_conf_params(className, true);
}

void
EmitWarningsOnPlaceholders(const char *className)
{
check_conf_params(className, false);
}

And change all the core extensions to use EmitErrorOnPlaceholders.
This way you don't break the backward compatibility of the outside
extensions, if they want they get to choose which behaviour they want
for their extension.

Actually, I didn't quite like the function name
EmitWarningsOnPlaceholders or EmitErrorOnPlaceholders, it could have
been something else. But let's not go that direction of changing the
function name for backward compatibility.

Regards,
Bharath Rupireddy.