MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_3cb3cdf22c37a3ed7047b935b59884a4"
Content-Transfer-Encoding: 8bit
Date: Fri, 17 Dec 2021 11:25:22 +0900
From: Shinya Kato <Shinya11.Kato@oss.nttdata.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Daniel
 Gustafsson <daniel@yesql.se>, PostgreSQL-development
 <pgsql-hackers@postgresql.org>
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
In-Reply-To: <9956b3b9-9cc3-49d2-f9e8-7f3ed3c63994@oss.nttdata.com>
References: <524fa2c0a34f34b68fbfa90d0760d515@oss.nttdata.com>
 <681A95E0-0CCA-4C66-A12B-CE0D8D5076D7@yesql.se>
 <25643eaa81a69416317e85c2fd7d4c0a@oss.nttdata.com>
 <CALj2ACX9eDm9bPmcZDTGazEVrH+p7jUXfN0miBc_VVze_mjw9A@mail.gmail.com>
 <98d354d3b099d8e24c6b653336bd29d7@oss.nttdata.com>
 <CALj2ACUBKmDa35PmcfGZ==4rLNm4GW04D7o-ARkeipHijr5yuw@mail.gmail.com>
 <4a2c5e69b98bf8ffbef2fad2f1850c97@oss.nttdata.com>
 <9956b3b9-9cc3-49d2-f9e8-7f3ed3c63994@oss.nttdata.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <9f5e96ca44891915f1d2736bea9fdf45@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/9f5e96ca44891915f1d2736bea9fdf45%40oss.nttdata.com>
Precedence: bulk

--=_3cb3cdf22c37a3ed7047b935b59884a4
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset=UTF-8;
 format=flowed

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
--=_3cb3cdf22c37a3ed7047b935b59884a4
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=v5_Add_EmitWarningsOnPlaceholders.patch
Content-Disposition: attachment;
 filename=v5_Add_EmitWarningsOnPlaceholders.patch;
 size=4766

ZGlmZiAtLWdpdCBhL2NvbnRyaWIvYXV0aF9kZWxheS9hdXRoX2RlbGF5LmMgYi9jb250cmliL2F1
dGhfZGVsYXkvYXV0aF9kZWxheS5jCmluZGV4IDU4MjBhYzMyOGQuLmQxMWRkMWU0MTYgMTAwNjQ0
Ci0tLSBhL2NvbnRyaWIvYXV0aF9kZWxheS9hdXRoX2RlbGF5LmMKKysrIGIvY29udHJpYi9hdXRo
X2RlbGF5L2F1dGhfZGVsYXkuYwpAQCAtNjcsNiArNjcsOSBAQCBfUEdfaW5pdCh2b2lkKQogCQkJ
CQkJCU5VTEwsCiAJCQkJCQkJTlVMTCwKIAkJCQkJCQlOVUxMKTsKKworCUVtaXRXYXJuaW5nc09u
UGxhY2Vob2xkZXJzKCJhdXRoX2RlbGF5Iik7CisKIAkvKiBJbnN0YWxsIEhvb2tzICovCiAJb3Jp
Z2luYWxfY2xpZW50X2F1dGhfaG9vayA9IENsaWVudEF1dGhlbnRpY2F0aW9uX2hvb2s7CiAJQ2xp
ZW50QXV0aGVudGljYXRpb25faG9vayA9IGF1dGhfZGVsYXlfY2hlY2tzOwpkaWZmIC0tZ2l0IGEv
Y29udHJpYi9wZ190cmdtL3RyZ21fb3AuYyBiL2NvbnRyaWIvcGdfdHJnbS90cmdtX29wLmMKaW5k
ZXggZmIzODEzNWY3YS4uMDQwN2M3ZGQ2NCAxMDA2NDQKLS0tIGEvY29udHJpYi9wZ190cmdtL3Ry
Z21fb3AuYworKysgYi9jb250cmliL3BnX3RyZ20vdHJnbV9vcC5jCkBAIC0xMDAsNiArMTAwLDgg
QEAgX1BHX2luaXQodm9pZCkKIAkJCQkJCQkgTlVMTCwKIAkJCQkJCQkgTlVMTCwKIAkJCQkJCQkg
TlVMTCk7CisKKwlFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVycygicGdfdHJnbSIpOwogfQogCiAv
KgpkaWZmIC0tZ2l0IGEvY29udHJpYi9wb3N0Z3Jlc19mZHcvb3B0aW9uLmMgYi9jb250cmliL3Bv
c3RncmVzX2Zkdy9vcHRpb24uYwppbmRleCA0OGM3NDE3ZTZlLi4zNjU1NTM5OGVjIDEwMDY0NAot
LS0gYS9jb250cmliL3Bvc3RncmVzX2Zkdy9vcHRpb24uYworKysgYi9jb250cmliL3Bvc3RncmVz
X2Zkdy9vcHRpb24uYwpAQCAtNDY5LDQgKzQ2OSw2IEBAIF9QR19pbml0KHZvaWQpCiAJCQkJCQkJ
ICAgTlVMTCwKIAkJCQkJCQkgICBOVUxMLAogCQkJCQkJCSAgIE5VTEwpOworCisJRW1pdFdhcm5p
bmdzT25QbGFjZWhvbGRlcnMoInBvc3RncmVzX2ZkdyIpOwogfQpkaWZmIC0tZ2l0IGEvY29udHJp
Yi9zZXBnc3FsL2hvb2tzLmMgYi9jb250cmliL3NlcGdzcWwvaG9va3MuYwppbmRleCAxOWEzZmZi
N2ZmLi40NGEwOWMzNWE3IDEwMDY0NAotLS0gYS9jb250cmliL3NlcGdzcWwvaG9va3MuYworKysg
Yi9jb250cmliL3NlcGdzcWwvaG9va3MuYwpAQCAtNDU1LDYgKzQ1NSw4IEBAIF9QR19pbml0KHZv
aWQpCiAJCQkJCQkJIE5VTEwsCiAJCQkJCQkJIE5VTEwpOwogCisJRW1pdFdhcm5pbmdzT25QbGFj
ZWhvbGRlcnMoInNlcGdzcWwiKTsKKwogCS8qIEluaXRpYWxpemUgdXNlcnNwYWNlIGFjY2VzcyB2
ZWN0b3IgY2FjaGUgKi8KIAlzZXBnc3FsX2F2Y19pbml0KCk7CiAKZGlmZiAtLWdpdCBhL3NyYy9i
YWNrZW5kL3V0aWxzL21pc2MvZ3VjLmMgYi9zcmMvYmFja2VuZC91dGlscy9taXNjL2d1Yy5jCmlu
ZGV4IGU5MWQ1YTNjZmQuLmM3YzAxNzljNWMgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxz
L21pc2MvZ3VjLmMKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMvbWlzYy9ndWMuYwpAQCAtNTQzMSwx
MSArNTQzMSwxOSBAQCB2YWxpZF9jdXN0b21fdmFyaWFibGVfbmFtZShjb25zdCBjaGFyICpuYW1l
KQogewogCWJvb2wJCXNhd19zZXAgPSBmYWxzZTsKIAlib29sCQluYW1lX3N0YXJ0ID0gdHJ1ZTsK
KwlpbnQgCQlwbGFjZWhvbGRlcl9sZW5ndGggPSAwOwogCiAJZm9yIChjb25zdCBjaGFyICpwID0g
bmFtZTsgKnA7IHArKykKIAl7CisJCXBsYWNlaG9sZGVyX2xlbmd0aCsrOwogCQlpZiAoKnAgPT0g
R1VDX1FVQUxJRklFUl9TRVBBUkFUT1IpCiAJCXsKKwkJCS8qIENoZWNrIGludmFsaWQgcGxhY2Vo
b2xkZXIgYW5kIGN1c3RvbSBwYXJhbWV0ZXIqLworCQkJY2hhciAqcGxhY2Vob2xkZXIgPSBtYWxs
b2Moc2l6ZW9mKGNoYXIpICogKHBsYWNlaG9sZGVyX2xlbmd0aCAtIDEpKTsKKwkJCXN0cm5jcHko
cGxhY2Vob2xkZXIsIG5hbWUsIHBsYWNlaG9sZGVyX2xlbmd0aCAtIDEpOworCQkJcGxhY2Vob2xk
ZXJbcGxhY2Vob2xkZXJfbGVuZ3RoIC0xXSA9ICdcMCc7CisJCQlFbWl0V2FybmluZ3NPblBsYWNl
aG9sZGVycyhwbGFjZWhvbGRlcik7CisKIAkJCWlmIChuYW1lX3N0YXJ0KQogCQkJCXJldHVybiBm
YWxzZTsJLyogZW1wdHkgbmFtZSBjb21wb25lbnQgKi8KIAkJCXNhd19zZXAgPSB0cnVlOwpAQCAt
OTMyMiw2ICs5MzMwLDEzIEBAIERlZmluZUN1c3RvbUVudW1WYXJpYWJsZShjb25zdCBjaGFyICpu
YW1lLAogCWRlZmluZV9jdXN0b21fdmFyaWFibGUoJnZhci0+Z2VuKTsKIH0KIAorLyoKKyAqIEZv
ciBoaXN0b3JpY2FsIGNvbnRleHQsIHRoaXMgZnVuY3Rpb24gbmFtZSBpcyBFbWl0V2FybmluZ3NP
blBsYWNlaG9sZGVycywKKyAqIGJ1dCBpdCBlbWl0cyBhbiBlcnJvciB3aGVuIGFuIGludmFsaWQg
Y3VzdG9tIEdVQyBpcyBzZXQuCisgKgorICogSWYgd2UgdXNlIERlZmluZUN1c3RvbVhYWFZhcmlh
YmxlLCBpdCBpcyByZWNvbW1lbmRlZCB0aGF0IHRoaXMgZnVuY3Rpb24gaXMKKyAqIGFkZGVkLgor
ICovCiB2b2lkCiBFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVycyhjb25zdCBjaGFyICpjbGFzc05h
bWUpCiB7CkBAIC05MzM2LDcgKzkzNTEsNyBAQCBFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVycyhj
b25zdCBjaGFyICpjbGFzc05hbWUpCiAJCQlzdHJuY21wKGNsYXNzTmFtZSwgdmFyLT5uYW1lLCBj
bGFzc0xlbikgPT0gMCAmJgogCQkJdmFyLT5uYW1lW2NsYXNzTGVuXSA9PSBHVUNfUVVBTElGSUVS
X1NFUEFSQVRPUikKIAkJewotCQkJZXJlcG9ydChXQVJOSU5HLAorCQkJZXJlcG9ydChFUlJPUiwK
IAkJCQkJKGVycmNvZGUoRVJSQ09ERV9VTkRFRklORURfT0JKRUNUKSwKIAkJCQkJIGVycm1zZygi
dW5yZWNvZ25pemVkIGNvbmZpZ3VyYXRpb24gcGFyYW1ldGVyIFwiJXNcIiIsCiAJCQkJCQkJdmFy
LT5uYW1lKSkpOwpkaWZmIC0tZ2l0IGEvc3JjL3BsL3RjbC9wbHRjbC5jIGIvc3JjL3BsL3RjbC9w
bHRjbC5jCmluZGV4IGUxMTgzNzU1OWQuLjEzODlhYTA5NDMgMTAwNjQ0Ci0tLSBhL3NyYy9wbC90
Y2wvcGx0Y2wuYworKysgYi9zcmMvcGwvdGNsL3BsdGNsLmMKQEAgLTQ3NCw2ICs0NzQsOCBAQCBf
UEdfaW5pdCh2b2lkKQogCQkJCQkJCSAgIFBHQ19TVVNFVCwgMCwKIAkJCQkJCQkgICBOVUxMLCBO
VUxMLCBOVUxMKTsKIAorCUVtaXRXYXJuaW5nc09uUGxhY2Vob2xkZXJzKCJwbHRjbHUiKTsKKwog
CXBsdGNsX3BtX2luaXRfZG9uZSA9IHRydWU7CiB9CiAKZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21v
ZHVsZXMvZGVsYXlfZXhlY3V0aW9uL2RlbGF5X2V4ZWN1dGlvbi5jIGIvc3JjL3Rlc3QvbW9kdWxl
cy9kZWxheV9leGVjdXRpb24vZGVsYXlfZXhlY3V0aW9uLmMKaW5kZXggYjNkMDg0MWJhOC4uOGVj
NjIzYWM1MiAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvbW9kdWxlcy9kZWxheV9leGVjdXRpb24vZGVs
YXlfZXhlY3V0aW9uLmMKKysrIGIvc3JjL3Rlc3QvbW9kdWxlcy9kZWxheV9leGVjdXRpb24vZGVs
YXlfZXhlY3V0aW9uLmMKQEAgLTkxLDYgKzkxLDggQEAgX1BHX2luaXQodm9pZCkKIAkJCQkJCQlO
VUxMLAogCQkJCQkJCU5VTEwpOwogCisJRW1pdFdhcm5pbmdzT25QbGFjZWhvbGRlcnMoImRlbGF5
X2V4ZWN1dGlvbiIpOworCiAJLyogSW5zdGFsbCBvdXIgaG9vayAqLwogCXByZXZfcGxhbm5lcl9o
b29rID0gcGxhbm5lcl9ob29rOwogCXBsYW5uZXJfaG9vayA9IGRlbGF5X2V4ZWN1dGlvbl9wbGFu
bmVyOwpkaWZmIC0tZ2l0IGEvc3JjL3Rlc3QvbW9kdWxlcy9zc2xfcGFzc3BocmFzZV9jYWxsYmFj
ay9zc2xfcGFzc3BocmFzZV9mdW5jLmMgYi9zcmMvdGVzdC9tb2R1bGVzL3NzbF9wYXNzcGhyYXNl
X2NhbGxiYWNrL3NzbF9wYXNzcGhyYXNlX2Z1bmMuYwppbmRleCA2YjBhM2RiMTA0Li4zYmEzM2U1
MDFjIDEwMDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVzL3NzbF9wYXNzcGhyYXNlX2NhbGxiYWNr
L3NzbF9wYXNzcGhyYXNlX2Z1bmMuYworKysgYi9zcmMvdGVzdC9tb2R1bGVzL3NzbF9wYXNzcGhy
YXNlX2NhbGxiYWNrL3NzbF9wYXNzcGhyYXNlX2Z1bmMuYwpAQCAtNDgsNiArNDgsOSBAQCBfUEdf
aW5pdCh2b2lkKQogCQkJCQkJCSAgIE5VTEwsCiAJCQkJCQkJICAgTlVMTCwKIAkJCQkJCQkgICBO
VUxMKTsKKworCUVtaXRXYXJuaW5nc09uUGxhY2Vob2xkZXJzKCJzc2xfcGFzc3BocmFzZSIpOwor
CiAJaWYgKHNzbF9wYXNzcGhyYXNlKQogCQlvcGVuc3NsX3Rsc19pbml0X2hvb2sgPSBzZXRfcm90
MTM7CiB9CmRpZmYgLS1naXQgYS9zcmMvdGVzdC9tb2R1bGVzL3dvcmtlcl9zcGkvd29ya2VyX3Nw
aS5jIGIvc3JjL3Rlc3QvbW9kdWxlcy93b3JrZXJfc3BpL3dvcmtlcl9zcGkuYwppbmRleCAwYjYy
NDY2NzZiLi5hZGIwMmQ4Y2I4IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVzL3dvcmtlcl9z
cGkvd29ya2VyX3NwaS5jCisrKyBiL3NyYy90ZXN0L21vZHVsZXMvd29ya2VyX3NwaS93b3JrZXJf
c3BpLmMKQEAgLTMyMiw2ICszMjIsOCBAQCBfUEdfaW5pdCh2b2lkKQogCQkJCQkJCSAgIDAsCiAJ
CQkJCQkJICAgTlVMTCwgTlVMTCwgTlVMTCk7CiAKKwlFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVy
cygid29ya2VyX3NwaSIpOworCiAJLyogc2V0IHVwIGNvbW1vbiBkYXRhIGZvciBhbGwgb3VyIHdv
cmtlcnMgKi8KIAltZW1zZXQoJndvcmtlciwgMCwgc2l6ZW9mKHdvcmtlcikpOwogCXdvcmtlci5i
Z3dfZmxhZ3MgPSBCR1dPUktFUl9TSE1FTV9BQ0NFU1MgfAo=
--=_3cb3cdf22c37a3ed7047b935b59884a4--