Re: Expand the use of check_canonical_path() for more GUCs

> On May 20, 2020, at 12:03 AM, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, May 19, 2020 at 09:32:15AM -0400, Tom Lane wrote:
>> Hm, I'm pretty certain that data_directory does not need this because
>> canonicalization is done elsewhere; the most that you could accomplish
>> there is to cause problems. Dunno about the rest.
>
> Hmm. I missed that this is getting done in SelectConfigFiles() first
> by the postmaster so that's not necessary, which also does the work
> for hba_file and ident_file. config_file does not need that either as
> AbsoluteConfigLocation() does the same work via ParseConfigFile(). So
> perhaps we could add a comment or such about that? Attached is an
> idea.
>
> The rest is made of PromoteTriggerFile, pg_krb_server_keyfile,
> ssl_cert_file, ssl_key_file, ssl_ca_file, ssl_crl_file and
> ssl_dh_params_file where loaded values are taken as-is, so applying
> canonicalization would be helpful there, no?

Before this patch, there are three GUCs that get check_canonical_path treatment:

log_directory
external_pid_file
stats_temp_directory

After the patch, these also get the treatment (though Peter seems to be objecting to the change):

promote_trigger_file
krb_server_keyfile
ssl_cert_file
ssl_key_file
ssl_ca_file
ssl_crl_file
ssl_dh_params_file

and these still don't, with comments about how they are already canonicalized when the config file is loaded:

data_directory
config_file
hba_file
ident_file

A little poking around shows that in SelectConfigFiles(), these four directories were set by SetConfigOption(). I don't see a problem with the code, but the way this stuff is spread around makes it easy for somebody adding a new GUC file path to do it wrong. I don't have much opinion about Peter's preference that paths be left alone, but I'd prefer some comments in guc.c explaining it all. The only cleanup that occurs to me is to reorder ConfigureNamesString[] to have all the path options back-to-back, with the four that are set by SelectConfigFiles() at the top with comments about why they are special, and the rest after that with comments about why they need or do not need canonicalization.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company