Fixes #37680 - Stop installing postgresql_evr extension

[sjha4]

Ref: Katello/katello#11087 which removes postgresql_evr from katello and adds the migration to create the type and trigger needed.

Goal of this PR is to uninstall postgresql_evr on existing setups and to stop installing it on new ones.

[ekohl]

I think this one needs some careful consideration for the ordering because I suspect you first need to migrate the DB before you drop the extension. Otherwise the DB depends on something that doesn't exist anymore.

That can easily lead to some cyclic dependencies:

• db:migrate depends on postgresql
• postgresql needs to be restarted to unload the old evr extension

If you do that restart, it causes downtime.

For simplicity, I'd be tempted to just stop managing it and leave it on the system for now. Then in a few releases we can actually ensure it's absent automatically. Document that in the release notes so users who want to can do so.

[ekohl]

Also, a Redmine issue is appreciated.

[sjha4]

If I understood it correctly, we'd remove the below section in the first version:

if $manage_db {
    package { $postgresql_evr_package:
      ensure => installed,
    }

and then in a future release add:

if $manage_db {
    package { $postgresql_evr_package:
      ensure => absent,
    }

I'll make a push with this.

[ekohl]

That is indeed what I was suggesting.

[sjha4]

That can easily lead to some cyclic dependencies:

• db:migrate depends on postgresql
• postgresql needs to be restarted to unload the old evr extension

So the db:migrate isn't needed for this on upgrades since we are updating an old migration file. On existing installs, the type, functions and triggers created as part of the loaded extension already exist and continue to work as normal when the extension is deleted. For new setups, the migration will take care of running the sql to create the same types, functions and triggers.

[ekohl]

I thought about testing that theory:

# dnf install postgresql-server postgresql-evr -y -q

Installed:
  postgresql-13.14-1.el9.x86_64    postgresql-evr-0.0.2-3.el9.noarch    postgresql-private-libs-13.14-1.el9.x86_64    postgresql-server-13.14-1.el9.x86_64   

# postgresql-setup --initdb
 * Initializing database in '/var/lib/pgsql/data'
 * Initialized, logs are in /var/lib/pgsql/initdb_postgresql.log
# systemctl enable --now postgresql
Created symlink /etc/systemd/system/multi-user.target.wants/postgresql.service → /usr/lib/systemd/system/postgresql.service.
# sudo -u postgres createdb foreman
# sudo -u postgres psql foreman
psql (13.14)
Type "help" for help.

foreman=# CREATE EXTENSION evr;
CREATE EXTENSION
foreman=# 
\q
# dnf remove postgresql-evr -y

Removed:
  postgresql-evr-0.0.2-3.el9.noarch                                                                                                                            

Complete!
# systemctl restart postgresql
# sudo -u postgres psql foreman
psql (13.14)
Type "help" for help.

foreman=# CREATE EXTENSION evr;
ERROR:  extension "evr" already exists

This surprises me because I'd have thought that it used the extension code from postgresql-evr even at runtime.

Now looking at a database dump you can see it still tries to use the extension:

# sudo -u postgres pg_dump foreman
--
-- PostgreSQL database dump
--

-- Dumped from database version 13.14
-- Dumped by pg_dump version 13.14

SET statement_timeout = 0;
SET lock_timeout = 0;
SET idle_in_transaction_session_timeout = 0;
SET client_encoding = 'UTF8';
SET standard_conforming_strings = on;
SELECT pg_catalog.set_config('search_path', '', false);
SET check_function_bodies = false;
SET xmloption = content;
SET client_min_messages = warning;
SET row_security = off;

--
-- Name: evr; Type: EXTENSION; Schema: -; Owner: -
--

CREATE EXTENSION IF NOT EXISTS evr WITH SCHEMA public;


--
-- Name: EXTENSION evr; Type: COMMENT; Schema: -; Owner: 
--

COMMENT ON EXTENSION evr IS 'evr datatype';


--
-- PostgreSQL database dump complete
--

You can then see it can't restore:

# sudo -u postgres pg_dump foreman > foreman.sql
# sudo -u postgres dropdb foreman
# sudo -u postgres createdb foreman
# sudo -u postgres postgres
postgres                     postgresql-new-systemd-unit  postgresql-setup             postgresql-upgrade           
# sudo -u postgres pgforeman < foreman.sql
pg_basebackup    pg_controldata   pg_dump          pg_isready       pg_recvlogical   pg_resetwal      pg_rewind        pg_verifybackup  
pg_checksums     pg_ctl           pg_dumpall       pg_receivewal    pgrep            pg_restore       pg_upgrade       
# sudo -u postgres psql foreman < foreman.sql
SET
SET
SET
SET
SET
 set_config 
------------
 
(1 row)

SET
SET
SET
SET
ERROR:  could not open extension control file "/usr/share/pgsql/extension/evr.control": No such file or directory
ERROR:  extension "evr" does not exist

So I didn't test using the extension at runtime, but at the very least you're breaking backups.

[sjha4]

Hmmm..We'll probably need to delete the extension itself while keeping all the functions/triggers/types on existing systems before we can remove the package.

For new installs the extension shouldn't be needed because we are not loading the extension which was happening here:
https://github.com/Katello/katello/pull/11087/files#diff-c1f0631894eedd49dbaed09b65310e6f689ef0339b4e3a0011b5ba9ad6cd7145L7

[ekohl]

For new installs the extension shouldn't be needed because we are not loading the extension which was happening here:

Except if a user does a fresh installation and wants to do a database restore.

I think you need some sort of DROP EXTENSION IF EXISTS evr in a migration that always runs, but also not break existing structures.

[sjha4]

Playing with that. A blind drop extension drops the evr type columns with existing data in it..

katello=# DROP EXTENSION evr CASCADE;
NOTICE:  drop cascades to 6 other objects
DETAIL:  drop cascades to column evr of table katello_rpms
drop cascades to column evr of table katello_installed_packages
drop cascades to trigger evr_insert_trigger_katello_rpms on table katello_rpms
drop cascades to trigger evr_update_trigger_katello_rpms on table katello_rpms
drop cascades to trigger evr_insert_trigger_katello_installed_packages on table katello_installed_packages
drop cascades to trigger evr_update_trigger_katello_installed_packages on table katello_installed_packages

That would require us to rerun the migration and then the applicability tasks etc to repopulate that data. Will have to think more about how to handle that.

[ekohl]

That would require us to rerun the migration and then the applicability tasks etc to repopulate that data.

You could introduce a new type (evr_tmp_t), alter the tables to that new type, drop the extension and rename the type to evr_t. And something similar for the functions. You can also detect if the extension was installed according to https://stackoverflow.com/questions/21799956/using-psql-how-do-i-list-extensions-installed-in-a-database. I just don't know if that works with limited permissions.

[sjha4]

Added a migration to run only on existing systems with evr extension enabled, which drops the extension and then recreates the constructs and data.
Katello/katello@ba94aef

Note that recreating the data after the evr field is dropped uses the exact same logic as the existing migration did when we first created the evr_t fields: https://github.com/Katello/katello/blob/8fed606fdc2bcd783a8e875aef43273e686c80e5/db/migrate/20200213184848_create_evr_type.rb#L17

I was running into trouble casting type evr_t to evr_t_temp and then realized it would still have to go through each row and update the data to evr_t_temp and then back to evr_t.

Note: db:rollback is a no-op in that migration.

[ekohl]

I'd still feel more comfortable if we ensured the migration ran before removing the extension.

@evgeni any thoughts?

[sjha4]

We have an ack on the katello migration Katello/katello#11087 which drops the extension from DB and removes all dependency on this package including migrating evr_t from extension to katello. We have also stopped enabling the extension on new installations as part of that PR.

Theoretically the katello PR is independent of what we do with the package. Unless anyone has objections, we can merge that and have QE get their hands on it while we figure out the best way to handle the package?

[ekohl]

Theoretically the katello PR is independent of what we do with the package. Unless anyone has objections, we can merge that and have QE get their hands on it while we figure out the best way to handle the package?

I still think this PR should simply remove the code from this module instead of ensuring it as absent, but I think that PR can indeed be merged without this.

[ekohl]

Unless anyone has objections, we can merge that and have QE get their hands on it while we figure out the best way to handle the package?

Also, QE can simply take the packit RPM to test it. Please don't merge things just so QE can test it: they don't need to. We should aim for merged code to always be shippable.

[sjha4]

Ack..Updated this to stop installing the postgresql-evr package for and keep it around for a release on existing setups in case anything comes up. Better to have it and not need it.

[ekohl]

I started the tests, but then quickly realized: you really need to drop the related code. Just grep for evr in manifests and spec`.

[sjha4]

@ekohl : Might have deleted more than required. Could you trigger the tests again?

[ianballou]

Just for an FYI, I'm dropping the postgresql-evr from the docs here: theforeman/foreman-documentation#3167

[sjha4]

This error is probably because the test is not running against katello master where we have removed the call to the extension?

2024-07-25T19:57:54.2310165Z �[00;00m�[00;33m  Notice: /Stage[main]/Foreman::Database/Foreman::Rake[db:migrate]/Exec[foreman-rake-db:migrate]/returns: /usr/share/gems/gems/katello-4.14.0.pre.master/db/migrate/20200213184848_create_evr_type.rb:7:in `up'

[ekohl]

I'd assume so. Let's wait till the migration is merged and makes it into nightly.

[sjha4]

@ekohl : Could you approve the test run on this?

Rebased and pushed. The last nightly was green following other evr merges so hopefully this will be green now..

[evgeni]

The last nightly was green following other evr merges so hopefully this will be green now..

The "problem" with the nightly testing right now is: it doesn't start on PostgreSQL 12, so the "bad owner" case is not covered.
(Not saying it should block, just that this aspect will require manual testing)

[ekohl]

CI failures are unrelated. Like @evgeni said: our CI won't cover the edge cases and manual testing is needed but that doesn't have to stop this.