Add a fact for installed postgres version

[maggu]

Summary

There is sometimes a need to handle versions differently. (The last exampel for us being deciding whether to monitor the stats collector process.) This fact provides a basis for writing such code.

Checklist

• 🟢 pdk validate
• 🟢 pdk test unit

[CLAassistant]

All committers have signed the CLA.

[kenyon]

At least on Debian family, postgres is not in PATH, so this wouldn't work. Maybe it'd be better to get the version from psql, which is in the PATH?

[maggu]

The problem with that is that psqlmight be a different version, and I would like to ensure that it's actually the database server version that gets reported. I guess this needs a bit more thought.

[maggu]

As stated below, perhaps pg_config --version could be used.

[kenyon]

This regex won't match the output of Debian family packages:

kenyon@lithium ~ % /usr/lib/postgresql/14/bin/postgres -V
postgres (PostgreSQL) 14.13 (Debian 14.13-1.pgdg110+1)

[maggu]

Okay. Guess the regex will have to be adjusted. This is RHEL 9 by the way, and I believe also vanilla PostgreSQL:

% /bin/postgres -V
postgres (PostgreSQL) 13.14
%

[deric]

There might be multiple PostgreSQL clusters installed with different versions. Debian has pg_lsclusters script.

$ pg_lsclusters 
Ver Cluster Port Status Owner    Data directory              Log file
13  main    5432 online postgres /var/lib/postgresql/13/main /var/log/postgresql/postgresql-13-main.log
16  main    5433 online postgres /var/lib/postgresql/16/main /var/log/postgresql/postgresql-16-main.log

the fact should be hierarchical, including cluster name, e.g.:

postgres:
  '13/main':
    version: 13.14
    data: '/var/lib/postgresql/13/main'
 '16/main':
     version: 16.2
     data: '/var/lib/postgresql/16/main'

[maggu]

There might be multiple PostgreSQL clusters installed with different versions. Debian has pg_lsclusters script.

RHEL doesn't unfortunately, so we can't depend on it for this. A quick Google search indicates that pg_lsclusters is a Debian specific thing.

[deric]

@maggu Yes, it's Debian specific. It's fairly simple Perl script. AFAIK postgresql doesn't ship any tool to manage/list clusters. You could try to parsing $confdir but that can be in very different locations. You could export clusters managed by Pupppet to a text/JSON file and then load it as a fact.

[bastelfreak]

I think in this case it's fine to only have a 80% solution. If it works for the common setups and the edge cases are documented, that is fine in my opinion.

[maggu]

I think in this case it's fine to only have a 80% solution. If it works for the common setups and the edge cases are documented, that is fine in my opinion.

I agree. If there's a need for the rest, a second hierarchical fact confined by and using pg_lsclusters could always be added in the future.

[ekohl]

Fedora 40 has already moved to parallel installable packages for PostgreSQL (by creating postgresqlXX packages) and I expect future RHEL versions to follow. It sounds to me that tooling like pg_lsclusters should be included.

There is sometimes a need to handle versions differently. (The last exampel for us being deciding whether to monitor the stats collector process.) This fact provides a basis for writing such code.

The version is already available in the globals as $postgresql::globals::version. By default detected rather statically but if you need a non-standard version then you modify it. The further code sets up the right repositories or in case of RHEL 8 & 9 OS packages the right DNF module. I don't see a need for a fact here.

[maggu]

The version is already available in the globals as $postgresql::globals::version. By default detected rather statically but if you need a non-standard version then you modify it. The further code sets up the right repositories or in case of RHEL 8 & 9 OS packages the right DNF module. I don't see a need for a fact here.

We started out by using that, with code like this:

lookup('postgresql::globals::version', undef, undef, pick(getvar('postgresql::globals::version'), '15.0'))

It has worked reasonably well sometimes. Other times though, when the version value is undef and the fallback used, the result has turned out to not be what we wanted. I would be interested to hear how this approach has worked out for others though. Switching to using a fact to determine the actual version so far at least seems to be more consistent over time.

I don't see us going back to using the globals parameter, even though I suppose to could be made to work with enough effort. We have a lot of people working with Puppet code in multiple departments and can't depend on it being explicitly set everywhere. Perhaps we're special enough though, that it's better that we just use a local fact instead of trying to merge this upstream.

[maggu]

Debian appears to have pg_config --version as part of the standard installation. I believe that could perhaps solve the problem of making this work on the major Linux platforms. I don't currently plan to put more work into it though unless the consensus is that a version fact would indeed be useful.

[ekohl]

It has worked reasonably well sometimes. Other times though, when the version value is undef and the fallback used, the result has turned out to not be what we wanted. I would be interested to hear how this approach has worked out for others though. Switching to using a fact to determine the actual version so far at least seems to be more consistent over time.

I think it should be the other way around: you specify the version you want and the module makes it so.

Fundamentally the problem is that until you install the package, you don't have the version. So it can never be idempotent.

• Run 1
  • Install package with some configuration
• Run 2
  • Fact is available
  • Modifications may be made

In addition to that, we have multi instance mode where I think even one instance may run a different version than another and I question how this fact is going to be useful in the generic case.

I don't see us going back to using the globals parameter, even though I suppose to could be made to work with enough effort. We have a lot of people working with Puppet code in multiple departments and can't depend on it being explicitly set everywhere. Perhaps we're special enough though, that it's better that we just use a local fact instead of trying to merge this upstream.

Perhaps other maintainers have a different opinion, but as a maintainer of this module I'd say that you're not using the module correctly. It is designed that you pass in the version you want.

[maggu]

Fundamentally the problem is that until you install the package, you don't have the version. So it can never be idempotent.

It would indeed require two runs to reach a final state. Isn't that the case for any fact except for the core facts? While I agree that in general fewer runs are better than more, there are also other aspects to consider. In this case, first and foremost the amount of work required to keep explicit versions updated.

In addition to that, we have multi instance mode where I think even one instance may run a different version than another and I question how this fact is going to be useful in the generic case.

That would be a limitiation to this PR, yes (as discussed above). Unless the scope is changed, and that is of course a possibility.

Perhaps other maintainers have a different opinion, but as a maintainer of this module I'd say that you're not using the module correctly. It is designed that you pass in the version you want.

I personally don't think the documentation currently reflects that intention very well, and perhaps ought to be updated. (Also, for reasons stated above I don't believe this approach would work very well for us.)

[ekohl]

It would indeed require two runs to reach a final state. Isn't that the case for any fact except for the core facts? While I agree that in general fewer runs are better than more, there are also other aspects to consider. In this case, first and foremost the amount of work required to keep explicit versions updated.

That is why I think detecting the version via a fact is considered unreliable. You can query the repositories for the available version and use that, but what if the manifests set up the repositories?

We have this in https://github.com/voxpupuli/puppet-nginx and there it's only causing problems.

I personally don't think the documentation currently reflects that intention very well, and perhaps ought to be updated.

That may be true. I see https://github.com/puppetlabs/puppetlabs-postgresql?tab=readme-ov-file#override-defaults is there, but it could be promoted to its own chapter so I opened #1610

(Also, for reasons stated above I don't believe this approach would work very well for us.)

Could you elaborate on that? Because I'm inclined to consider it unsupported. This module takes great care of minimizing the amount of changes to a database server because restarting a database server is considered undesirable. Anything that can risk that should be treated with great caution.