Subject: Re: [PATCH] Make configuration file "include" directive handling more
 robust
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, pgsql-hackers@postgresql.org
References: <8c8bcbca-3bd9-dc6e-8986-04a5abdef142@2ndquadrant.com>
 <20190717.173445.172079490.horikyota.ntt@gmail.com>
 <05b22e26-adb5-8193-cec5-a8ae1de0b8dc@2ndquadrant.com>
 <6760.1566675571@sss.pgh.pa.us>
From: Ian Barwick <ian.barwick@2ndquadrant.com>
Message-ID: <58456b7c-ff81-d020-0e9b-a64189620d5d@2ndquadrant.com>
Date: Wed, 28 Aug 2019 10:57:24 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <6760.1566675571@sss.pgh.pa.us>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/25/19 4:39 AM, Tom Lane wrote:
> Ian Barwick <ian.barwick@2ndquadrant.com> writes:
>> On 7/17/19 5:34 PM, Kyotaro Horiguchi wrote:> Hello.
>>>> I don't think this is new to 12.
> 
>> No, though I'm not sure how much this would be seen as a bugfix
>> and how far back it would be sensible to patch.
> 
> I think this is worth considering as a bugfix; although I'm afraid
> we can't change the signature of ParseConfigFile/ParseConfigFp in
> released branches, since extensions could possibly be using those.
> That limits what we can do --- but it's still possible to detect
> direct recursion, which seems like enough to produce a nice error
> message in typical cases.

Makes sense.

> I concur with Kyotaro-san that disallow-empty-include-directives.v1.patch
> seems a bit brute-force, but where I would put the checks is in
> ParseConfigFile and ParseConfigDirectory.
> 
> Also, I don't agree with the goals of prevent-disallowed-includes.patch.
> I'm utterly not on board with breaking use of "include" in extension
> files, for instance; while that may not be documented, it works fine,
> and maybe somebody out there is relying on it.

I couldn't for the life of me think of any reason for using it.
But if there's undocumented functionality we think someone might
be using, shouldn't that be documented somewhere, if only as a note
in the code to prevent its accidental removal at a later date?

> Likewise, while "include"
> in pg.auto.conf is not really considered supported, I don't see the
> point of going out of our way to break the historical behavior.

The amusing thing about that of course is that the include directive
will disappear the next time ALTER SYSTEM is run and the values from
the included file will appear in pg.auto.conf, which may cause some
headscratching. But I guess hasn't been an actual real-world
issue so far.

> That leads me to propose the attached simplified patch.  While I haven't
> actually tried, I'm pretty sure this should back-patch without trouble.

Ah, I see it's been applied already, thanks!


Regards

Ian Barwick

-- 
  Ian Barwick                   https://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services