MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 22 Dec 2021 16:50:34 +0900
From: Shinya Kato <Shinya11.Kato@oss.nttdata.com>
To: Tom Lane <tgl@sss.pgh.pa.us>, Kyotaro Horiguchi
 <horikyota.ntt@gmail.com>
Cc: 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: <57009.1640107405@sss.pgh.pa.us>
References: <9f5e96ca44891915f1d2736bea9fdf45@oss.nttdata.com>
 <83b33009-3a1b-9230-8f07-2115763c1853@enterprisedb.com>
 <c5660e836dd511549810fd35b12edc3c@oss.nttdata.com>
 <20211221.100834.1297468238807011575.horikyota.ntt@gmail.com>
 <57009.1640107405@sss.pgh.pa.us>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <f055f52eeedba4a4fc69a3e0dc5dc176@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/f055f52eeedba4a4fc69a3e0dc5dc176%40oss.nttdata.com>
Precedence: bulk

On 2021-12-22 02:23, Tom Lane wrote:
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>> At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato 
>> <Shinya11.Kato@oss.nttdata.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

Thank you for pushing!
Thank you all for the reviews, I think I will take down 0002 and 0003.

-- 
Regards,

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