MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="=_216654aee1cad53d7b68c21999a1d8c6"
Content-Transfer-Encoding: 7bit
Date: Tue, 16 Nov 2021 18:20:33 +0900
From: Shinya Kato <Shinya11.Kato@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: 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: 
 <CALj2ACX9eDm9bPmcZDTGazEVrH+p7jUXfN0miBc_VVze_mjw9A@mail.gmail.com>
References: <524fa2c0a34f34b68fbfa90d0760d515@oss.nttdata.com>
 <681A95E0-0CCA-4C66-A12B-CE0D8D5076D7@yesql.se>
 <25643eaa81a69416317e85c2fd7d4c0a@oss.nttdata.com>
 <CALj2ACX9eDm9bPmcZDTGazEVrH+p7jUXfN0miBc_VVze_mjw9A@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <98d354d3b099d8e24c6b653336bd29d7@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/98d354d3b099d8e24c6b653336bd29d7%40oss.nttdata.com>
Precedence: bulk

--=_216654aee1cad53d7b68c21999a1d8c6
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=US-ASCII;
 format=flowed

On 2021-11-15 15:14, Bharath Rupireddy wrote:

> Do we need to add them in the following too?
> 
> delay_execution/delay_execution.c
> ssl_passphrase_func.c
> worker_spi.c
> 
> Especially, worker_spi.c is important as it works as a template for
> the bg worker code.

Thank you for pointing this out! I added it.

> 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.

I plan to change to emit an error when an invalid custom GUC is set in 
the SET or ALTER SYSTEM SET commands, but I haven't tackled this yet.
The patch as of now is attached.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
--=_216654aee1cad53d7b68c21999a1d8c6
Content-Transfer-Encoding: base64
Content-Type: text/x-diff;
 name=v3_Add_EmitWarningsOnPlaceholders.patch
Content-Disposition: attachment;
 filename=v3_Add_EmitWarningsOnPlaceholders.patch;
 size=4133

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
ZGV4IGU5MWQ1YTNjZmQuLjA3NjQ3YTBkODQgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxz
L21pc2MvZ3VjLmMKKysrIGIvc3JjL2JhY2tlbmQvdXRpbHMvbWlzYy9ndWMuYwpAQCAtOTMyMiw2
ICs5MzIyLDEzIEBAIERlZmluZUN1c3RvbUVudW1WYXJpYWJsZShjb25zdCBjaGFyICpuYW1lLAog
CWRlZmluZV9jdXN0b21fdmFyaWFibGUoJnZhci0+Z2VuKTsKIH0KIAorLyoKKyAqIEZvciBoaXN0
b3JpY2FsIGNvbnRleHQsIHRoaXMgZnVuY3Rpb24gbmFtZSBpcyBFbWl0V2FybmluZ3NPblBsYWNl
aG9sZGVycywKKyAqIGJ1dCBpdCBlbWl0cyBhbiBlcnJvciB3aGVuIGFuIGludmFsaWQgY3VzdG9t
IEdVQyBpcyBzZXQuCisgKgorICogSWYgd2UgdXNlIERlZmluZUN1c3RvbVhYWFZhcmlhYmxlLCBp
dCBpcyByZWNvbW1lbmRlZCB0aGF0IHRoaXMgZnVuY3Rpb24gaXMKKyAqIGFkZGVkLgorICovCiB2
b2lkCiBFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVycyhjb25zdCBjaGFyICpjbGFzc05hbWUpCiB7
CkBAIC05MzM2LDcgKzkzNDMsNyBAQCBFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVycyhjb25zdCBj
aGFyICpjbGFzc05hbWUpCiAJCQlzdHJuY21wKGNsYXNzTmFtZSwgdmFyLT5uYW1lLCBjbGFzc0xl
bikgPT0gMCAmJgogCQkJdmFyLT5uYW1lW2NsYXNzTGVuXSA9PSBHVUNfUVVBTElGSUVSX1NFUEFS
QVRPUikKIAkJewotCQkJZXJlcG9ydChXQVJOSU5HLAorCQkJZXJlcG9ydChFUlJPUiwKIAkJCQkJ
KGVycmNvZGUoRVJSQ09ERV9VTkRFRklORURfT0JKRUNUKSwKIAkJCQkJIGVycm1zZygidW5yZWNv
Z25pemVkIGNvbmZpZ3VyYXRpb24gcGFyYW1ldGVyIFwiJXNcIiIsCiAJCQkJCQkJdmFyLT5uYW1l
KSkpOwpkaWZmIC0tZ2l0IGEvc3JjL3BsL3RjbC9wbHRjbC5jIGIvc3JjL3BsL3RjbC9wbHRjbC5j
CmluZGV4IGUxMTgzNzU1OWQuLjEzODlhYTA5NDMgMTAwNjQ0Ci0tLSBhL3NyYy9wbC90Y2wvcGx0
Y2wuYworKysgYi9zcmMvcGwvdGNsL3BsdGNsLmMKQEAgLTQ3NCw2ICs0NzQsOCBAQCBfUEdfaW5p
dCh2b2lkKQogCQkJCQkJCSAgIFBHQ19TVVNFVCwgMCwKIAkJCQkJCQkgICBOVUxMLCBOVUxMLCBO
VUxMKTsKIAorCUVtaXRXYXJuaW5nc09uUGxhY2Vob2xkZXJzKCJwbHRjbHUiKTsKKwogCXBsdGNs
X3BtX2luaXRfZG9uZSA9IHRydWU7CiB9CiAKZGlmZiAtLWdpdCBhL3NyYy90ZXN0L21vZHVsZXMv
ZGVsYXlfZXhlY3V0aW9uL2RlbGF5X2V4ZWN1dGlvbi5jIGIvc3JjL3Rlc3QvbW9kdWxlcy9kZWxh
eV9leGVjdXRpb24vZGVsYXlfZXhlY3V0aW9uLmMKaW5kZXggYjNkMDg0MWJhOC4uOGVjNjIzYWM1
MiAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvbW9kdWxlcy9kZWxheV9leGVjdXRpb24vZGVsYXlfZXhl
Y3V0aW9uLmMKKysrIGIvc3JjL3Rlc3QvbW9kdWxlcy9kZWxheV9leGVjdXRpb24vZGVsYXlfZXhl
Y3V0aW9uLmMKQEAgLTkxLDYgKzkxLDggQEAgX1BHX2luaXQodm9pZCkKIAkJCQkJCQlOVUxMLAog
CQkJCQkJCU5VTEwpOwogCisJRW1pdFdhcm5pbmdzT25QbGFjZWhvbGRlcnMoImRlbGF5X2V4ZWN1
dGlvbiIpOworCiAJLyogSW5zdGFsbCBvdXIgaG9vayAqLwogCXByZXZfcGxhbm5lcl9ob29rID0g
cGxhbm5lcl9ob29rOwogCXBsYW5uZXJfaG9vayA9IGRlbGF5X2V4ZWN1dGlvbl9wbGFubmVyOwpk
aWZmIC0tZ2l0IGEvc3JjL3Rlc3QvbW9kdWxlcy9zc2xfcGFzc3BocmFzZV9jYWxsYmFjay9zc2xf
cGFzc3BocmFzZV9mdW5jLmMgYi9zcmMvdGVzdC9tb2R1bGVzL3NzbF9wYXNzcGhyYXNlX2NhbGxi
YWNrL3NzbF9wYXNzcGhyYXNlX2Z1bmMuYwppbmRleCA2YjBhM2RiMTA0Li4zYmEzM2U1MDFjIDEw
MDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVzL3NzbF9wYXNzcGhyYXNlX2NhbGxiYWNrL3NzbF9w
YXNzcGhyYXNlX2Z1bmMuYworKysgYi9zcmMvdGVzdC9tb2R1bGVzL3NzbF9wYXNzcGhyYXNlX2Nh
bGxiYWNrL3NzbF9wYXNzcGhyYXNlX2Z1bmMuYwpAQCAtNDgsNiArNDgsOSBAQCBfUEdfaW5pdCh2
b2lkKQogCQkJCQkJCSAgIE5VTEwsCiAJCQkJCQkJICAgTlVMTCwKIAkJCQkJCQkgICBOVUxMKTsK
KworCUVtaXRXYXJuaW5nc09uUGxhY2Vob2xkZXJzKCJzc2xfcGFzc3BocmFzZSIpOworCiAJaWYg
KHNzbF9wYXNzcGhyYXNlKQogCQlvcGVuc3NsX3Rsc19pbml0X2hvb2sgPSBzZXRfcm90MTM7CiB9
CmRpZmYgLS1naXQgYS9zcmMvdGVzdC9tb2R1bGVzL3dvcmtlcl9zcGkvd29ya2VyX3NwaS5jIGIv
c3JjL3Rlc3QvbW9kdWxlcy93b3JrZXJfc3BpL3dvcmtlcl9zcGkuYwppbmRleCAwYjYyNDY2NzZi
Li5hZGIwMmQ4Y2I4IDEwMDY0NAotLS0gYS9zcmMvdGVzdC9tb2R1bGVzL3dvcmtlcl9zcGkvd29y
a2VyX3NwaS5jCisrKyBiL3NyYy90ZXN0L21vZHVsZXMvd29ya2VyX3NwaS93b3JrZXJfc3BpLmMK
QEAgLTMyMiw2ICszMjIsOCBAQCBfUEdfaW5pdCh2b2lkKQogCQkJCQkJCSAgIDAsCiAJCQkJCQkJ
ICAgTlVMTCwgTlVMTCwgTlVMTCk7CiAKKwlFbWl0V2FybmluZ3NPblBsYWNlaG9sZGVycygid29y
a2VyX3NwaSIpOworCiAJLyogc2V0IHVwIGNvbW1vbiBkYXRhIGZvciBhbGwgb3VyIHdvcmtlcnMg
Ki8KIAltZW1zZXQoJndvcmtlciwgMCwgc2l6ZW9mKHdvcmtlcikpOwogCXdvcmtlci5iZ3dfZmxh
Z3MgPSBCR1dPUktFUl9TSE1FTV9BQ0NFU1MgfAo=
--=_216654aee1cad53d7b68c21999a1d8c6--