[pmg-devel] [PATCH pmg-api 2/3] add get_pg_server_version in PMG::Utils

[Dominik Csapak]

one comment inline

On 8/8/19 9:14 AM, Stoiko Ivanov wrote:
> PMG renders the postgresql.conf through its templating system (currently the
> shipped template does not use any variables). postgresql.conf (in most
> installations and in both debian and upstream packages) contains a few
> occurrences (datadir, config files, pid-file, cluster name) of the postgres
> major version number (see [0], for a description and why 9.6 and 11 are major
> version numbers). The rendered config should use the correct version number
> for the config of the currently used postgres installation (the one listening
> on the default port (5432) and socket).
> 
> This fixes a bug observed while testing the upgrade to buster and postgres 11:
> * a long running service (pmgmirror, pmgdaemon) still has the old config
>    path in memory (/etc/postgresql/9.6/)
> * while upgrading the pmg-api package the shipped template changes to one
>    with the new major number (11)
> * the next restart of the postgresql cluster fails, with an error not directly
>    related to the broken config file
> 
> By reading [1] the version number through a connection to the current
> postgresql server we rewrite the fitting configfile with the correct paths.
> 
> [0] https://www.postgresql.org/support/versioning/
> [1] https://www.postgresql.org/docs/11/runtime-config-preset.html
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
>   src/PMG/Utils.pm | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 103982e..a773f9d 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -41,6 +41,7 @@ use base 'Exporter';
>   
>   our @EXPORT_OK = qw(
>   postgres_admin_cmd,
> +get_pg_server_version,
>   );
>   
>   my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
> @@ -1401,4 +1402,20 @@ sub postgres_admin_cmd {
>   	die "setresuid back failed - $!\n";
>   }
>   
> +sub get_pg_server_version {
> +    my $major_ver;
> +    my $parser = sub {
> +	my $line = shift;
> +	$line = PVE::Tools::trim($line);
> +	# example output:
> +	# 9.6.13
> +	# 11.4 (Debian 11.4-1)
> +	($major_ver) = ($line =~ m/^([0-9]+(?:\.[0-9]+)?)\.[0-9]+/);
> +    };
> +    postgres_admin_cmd('psql', { outfunc => $parser }, '--quiet',
> +	'--tuples-only', '--no-align', '--command', 'show server_version;');
> +
> +    return $major_ver;

this returns undef if it is not able to parse the version ( for whatever 
reason)

i would prefer it to die, so we do not have to guess the version but
have either to
* handle the error explicitely in some way
* stop the execution

the second is probably better for things where we want to write the 
config, since it does not make sense to continue if we cannot get the 
version

if someone has any reason against it, i am also ok with returning undef,
but we have to carefully check it on every call

> +}
> +
>   1;
>