From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
cc: Shinya11.Kato@oss.nttdata.com, peter.eisentraut@enterprisedb.com,
        masao.fujii@oss.nttdata.com, bharath.rupireddyforpostgres@gmail.com,
        daniel@yesql.se, pgsql-hackers@postgresql.org
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
In-reply-to: <20211221.100834.1297468238807011575.horikyota.ntt@gmail.com>
References: <9f5e96ca44891915f1d2736bea9fdf45@oss.nttdata.com>
 <83b33009-3a1b-9230-8f07-2115763c1853@enterprisedb.com>
 <c5660e836dd511549810fd35b12edc3c@oss.nttdata.com>
 <20211221.100834.1297468238807011575.horikyota.ntt@gmail.com>
Comments: In-reply-to Kyotaro Horiguchi <horikyota.ntt@gmail.com>
	message dated "Tue, 21 Dec 2021 10:08:34 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <57008.1640107405.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 21 Dec 2021 12:23:25 -0500
Message-ID: <57009.1640107405@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/57009.1640107405%40sss.pgh.pa.us>
Precedence: bulk

Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato <Shinya11.Kato@oss.nttda=
ta.com> wrote in =

>> We should use EmitWarningsOnPlaceholders when we use
>> DefineCustomXXXVariable.
>> I don't think there is any room for debate.

> Unfortunately, pltcl.c defines variables both in pltcl and pltclu but
> the patch adds the warning only for pltclu.

Right.  The rest of 0001 looks fine, so pushed with that correction.

I concur that 0002 is unacceptable.  The result of it will be that
a freshly-loaded extension will fail to hook into the system as it
should, because it will fail to complete its initialization.
That will cause user frustration and bug reports.

(BTW, I wonder if EmitWarningsOnPlaceholders should be using LOG
rather than WARNING when it's running in the postmaster.  Use of
WARNING is moderately likely to result in nothing getting printed
at all.)

0003 looks to me like it was superseded by 75d22069e.  I do not
particularly like that patch though; it seems both inefficient
and ill-designed.  0003 is adding a check in an equally bizarre
place.  Why isn't add_placeholder_variable the place to deal
with this?  Also, once we know that a prefix is reserved,
why don't we throw an error not just a warning for attempts to
set some unknown variable under that prefix?  We don't allow
you to set unknown non-prefixed variables.

			regards, tom lane