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

On 2021-12-17 01:55, Fujii Masao wrote:
> On 2021/12/16 16:31, Shinya Kato wrote:
>> Thank you for the review and sorry for the late reply.
>> 
>> On 2021-11-16 19:25, Bharath Rupireddy wrote:
>>>> > 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)
>> 
>> I have made changes to achieve the above.
> 
> IMO this behavior change is not good. For example, because it seems to
> break at least the following case. I think that these are the valid
> steps, but with the patch, the server fails to start up at the step #2
> because pg_trgm's custom parameters are treated as invalid ones.
> 
> 1. Add the following two pg_trgm parameters to postgresql.conf
>     - pg_trgm.similarity_threshold
>     - pg_trgm.strict_word_similarity_threshold
> 
> 2. Start the server
> 
> 3. Run "CREATE EXTENSION pg_trgm" and then use pg_trgm

It can happen because the placeholder "pg_trgm" is not already 
registered.
I think too this behavior is not good.
I need to consider an another implementation or to allow undefined GUCs 
to be set.


> When I compiled PostgreSQL with the patch, I got the following
> compilation failure.
> 
> guc.c:5453:4: error: implicit declaration of function
> 'EmitErrorsOnPlaceholders' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
>                         EmitErrorsOnPlaceholders(placeholder);
>                         ^
> 
> 
> -            ereport(WARNING,
> +            ereport(ERROR,

Thanks! There was a correction omission, so I fixed it.


> I'm still not sure why you were thinking that ERROR is more proper 
> here.

Since I get an ERROR when I set the wrong normal GUCs, I thought I 
should also get an ERROR when I set the wrong extension's GUCs.
However, there are likely to be harmful effects, and I may need to think 
again.


For now, I'v attached the patch that fixed the compilation error.

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION