D21154: Automatically run pg_upgrade when major PostgreSQL upgrade is detected

[Sandro Knauß]

knauss added a comment.


  overall I like this patch - that is great an brings postgresql more to a usable db interface.

INLINE COMMENTS

> dbconfigpostgresql.cpp:98
>                  // Don't break once PostgreSQL 10 is released, but something more future-proof will be needed
>                  if (path.fileName().startsWith(QLatin1String("10."))) {
>                      postgresVersionedSearchPaths.prepend(path.absoluteFilePath() + QStringLiteral("/bin"));

We do another if clause for version 11?

> dbconfigpostgresql.cpp:209
> +    }
> +    // Looks like "pg_ctl (PostgreSQL) 11.2"
> +    const auto output = QString::fromUtf8(pgctl.readAll());

For Debian it looks like:

pg_ctl (PostgreSQL) 11.3 (Debian 11.3-1)

I would make the regex more explicit, to match to the correct version:

re(QStringLiteral("\(PostgreSQL\)  ([0-9]+).[0-9]+"))

> dbconfigpostgresql.cpp:254
> +        // TODO: Check Debian-based distros, they might install previous PSQL versions into a
> +        // different directory.
> +    };

for Debian it is just:

/usr/lib/postgresql/%1/bin/

You should use the same logic like in postgresSearchPath from DbConfigPostgresql::init to find the old bin dir. I can't see why the old version should be located differently.

> dbconfigpostgresql.cpp:260
> +
> +    if (baseDir.exists(QStringLiteral("old_db_data"))) {
> +        qCInfo(AKONADISERVER_LOG) << "Postgres cluster update: old_db_data cluster already exists, trying to remove it first";

please change order to:

1. search for oldBinPath (line 268)
2. test if oldBinPath.isEmpty(line 275)
3. remove old_db_data (l260)

This is more in the sense, that we do not touch anything, if the old Postgres is not installed, as we can't migrate at all.

> dbconfigpostgresql.cpp:282
> +
> +    // Next, initialize a new cluster
> +    const QString newDbData = baseDir.path() + QStringLiteral("/new_db_data");

shouldn't we check if newDbData alread exists before runInitDb? because this is properly an error sate, as newDbData should not exist at this state.

REPOSITORY
  R165 Akonadi

REVISION DETAIL
  https://phabricator.kde.org/D21154

To: dvratil, #kde_pim
Cc: knauss, kde-pim, dvasin, rodsevich, winterz, vkrause, mlaurent, dvratil
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20190513/2554cd3d/attachment.html>