Not possible to "upgrade" PostgreSQL database [PATCH]

[sergey]

trac-admin <path> upgrade for a project that uses PostgreSQL database always results in an error:

Command failed: Can only backup sqlite databases

[Matthew Good]

This is a duplicate of #1741

You need to turn off the database backup as noted in the upgrade documentation.

[Martin <martin@…>]

I know this ticket is closed, but wouldn't it be *smarter* to look at the type of database been used in conf/trac.ini and then decide whether or not a backup is needed?

It looks like backup is only done with SQLite, and not with PostgreSQL or MySQL.

I can try to make a patch.

[shanec@…]

patch to use postgres scheme for backup

[anonymous]

I just added a patch I created for postgres backups. It creates a new schema, using the current schema name, then copies the tables/data into the new schema. This can latter be processed via cron job or something to do a pg_dump of the schema and get rid of it. This at least allows for a backup of data during an upgrade. Not sure what kind of performance issues this may have with large databases.

[shanec@…]

IBackupProvider interface

[shanec@…]

The second patch, just added, implements the use of an extension interface, allowing different systems to implement a backup that is appropriate for them. I used the postgres code from the first patch in an implementation of the interface to provide a simple backup for postgres during upgrades.

[Shane Caraveo <shanec@…>]

Implementation of IBackupProvier

[Shane Caraveo <shanec@…>]

The third patch is much more interesting for Trac IMHO. It creates an IBackupProvider interface for the trac backup. This allows me to create custom backup handlers for any database. The first provider I created simply backed up to a new postgres schema, then I later created one that calls out to pg_dump to dump the database into a backup directory in the trac environment.

[Remy Blank]

Looks very interesting.

[Shane Caraveo <shanec@…>]

general backup implementation

[Shane Caraveo <shanec@…>]

revised backup patch with test case

[shanec@…]

now including mysql backup

[Shane Caraveo <shanec@…>]

fixed full patch with new backup

[Christian Boos]

The approach looks good! A few initial remarks:

• please be more ​PEP:0008 compliant, long lines will be wrapped which makes the reading through Trac less convenient
• why do you need to wrap the call to the backup program with a call to bash -c or cmd /c on Windows?
• the backup_compression level stuff doesn't seem to be used

[anonymous]

Replying to cboos:

The approach looks good! A few initial remarks:

• please be more ​PEP:0008 compliant, long lines will be wrapped which makes the reading through Trac less convenient

shall do in the future

• why do you need to wrap the call to the backup program with a call to bash -c or cmd /c on Windows?

It was the only way I could get this to work. Giving the args directly to subprocess didn't for whatever reason.

• the backup_compression level stuff doesn't seem to be used

It is passed through with -Z for pg_dump.

This is in the rework-testing sandbox, where more work has been done on it.

[Remy Blank]

I have a few comments as well:

• We have enscript_path and php_path. Can we have mysqldump_path and pg_dump_path rather than *_bin?

• Does the compression really need to be configurable? Can't we just select the most likely candidate (the current default value, probably) and hardcode it?

• Could you replace the redirection using > (which requires a shell) with passing an open file as stdout to Popen()? This would remove the special case for Windows, and allow calling the command directly, without needing the shell.

• Instead of checking if the destination file exists after the call, I would check the call's return value. Indeed, the file may have been created, but the backup failed anyway.

• You don't seem to use the output on stderr, so instead of recording it, it could be dropped into a "null" stream.

I'll attach a patch with the proposed changes.

[Remy Blank]

Proposed changes against [8038] as per comment:12.

[Remy Blank]

The patch above implements the changes proposed in comment:12. It hasn't been tested, as I don't have PostgreSQL or MySQL readily available.

[Christian Boos]

Replying to rblank:

• You don't seem to use the output on stderr, so instead of recording it, it could be dropped into a "null" stream.

I don't want to nitpick or ruin your nice Null class… but wouldn't it be more interesting to actually collect the stderr output and show it to the user? The backup failed, so the question is "what really happened?" (this is assuming those binaries show something useful on their stderr).

Otherwise, I also see the other proposed changes as an improvement.

[Remy Blank]

Replying to cboos:

I don't want to nitpick or ruin your nice Null class…

Heh, a bit on the overengineered side, isn't it? At first, I just wanted to use a null stream like:

class NullStream(object):
    def write(data):
        pass

but I have always found the Null pattern quite handy, so I implemented the full Monty. Feel free to drop it, I won't shout.

but wouldn't it be more interesting to actually collect the stderr output and show it to the user? The backup failed, so the question is "what really happened?" (this is assuming those binaries show something useful on their stderr).

Yes, that's a nice idea, too ;-) It could be used as part of the exception text in case the command fails.

[Remy Blank]

I see the rework-testing branch has been merged to trunk. Will 2304-proposed-changes-r8038.patch​ be integrated as well? I'd do it myself, but unfortunately I have neither PostgreSQL nor MySQL at hand, so I can't test the DB backup functionality.

[Christian Boos]

Yes, that was the idea, I just got out of time for today, but I'll do it tomorrow. This and backporting to 0.11-stable, as the changes will be useful there as well (i.e. allow me to run the functional tests on windows without svn).

I have no idea if there was more work left to be done on the branch, but the current status was already useful to be backported. The remaining work if any could happen on the branch or on trunk directly, as the "functional test guys" see fit ;-)

[Christian Boos]

Ok, so I've been able to get the functional tests in the rework-testing branch working with the other db backends. I did minimal changes in order to get them working on Windows (r8135 for MySQL and r8137 for PostgreSQL). It would be interesting to check they're still working on Linux…

Now I'll move on to integrate attachment:2304-proposed-changes-r8038.patch​.

[Christian Boos]

Patch attachment:2304-proposed-changes-r8038.patch​ applied in r8138, with the modifications discussed above (comment:14 / comment:15).

The above changes were tested on Windows only, so far. Please test on Linux as well, and refine further if needed.

[Remy Blank]

A comment about [8138]: when spawning the dump processes with subprocess, if you do a wait() followed by a communicate() only if the return value is not 0, you risk having the process block if it outputs too much on stderr, as the pipe will be filled. It would be better to use communicate() to wait for process termination, and to check the .returncode on the Popen object afterwards.

[Christian Boos]

Thanks for the hint, I modified the methods accordingly in [8142].

[Christian Boos]

Landed on 0.11-stable in r8152.