-
Notifications
You must be signed in to change notification settings - Fork 614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a fact for installed postgres version #1609
base: main
Are you sure you want to change the base?
Conversation
There is sometimes a need to handle versions differently. This fact provides a basis for writing such code.
# frozen_string_literal: true | ||
|
||
Facter.add('postgres_version') do | ||
confine { Facter::Core::Execution.which('postgres') } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that is that psql
might 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated below, perhaps pg_config --version
could be used.
confine { Facter::Core::Execution.which('postgres') } | ||
setcode do | ||
version = Facter::Core::Execution.execute('postgres -V 2>/dev/null') | ||
version.match(%r{\d+\.\d+$})[0] if version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
%
There might be multiple PostgreSQL clusters installed with different versions. Debian has
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' |
RHEL doesn't unfortunately, so we can't depend on it for this. A quick Google search indicates that |
@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 |
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 |
Fedora 40 has already moved to parallel installable packages for PostgreSQL (by creating
The version is already available in the globals as |
We started out by using that, with code like this:
It has worked reasonably well sometimes. Other times though, when the 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. |
Debian appears to have |
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.
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.
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. |
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 would be a limitiation to this PR, yes (as discussed above). Unless the scope is changed, and that is of course a possibility.
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.) |
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.
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. |
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