Edgewall Software
Modify

Opened 14 years ago

Closed 10 years ago

Last modified 4 years ago

#2304 closed defect (fixed)

Not possible to "upgrade" PostgreSQL database [PATCH]

Reported by: sergey Owned by: Remy Blank
Priority: normal Milestone: 0.11.5
Component: admin/console Version: 0.11.2.1
Severity: major Keywords: postgresql, mysql, install
Cc: shanec@… Branch:
Release Notes:
API Changes:

Description

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

Command failed: Can only backup sqlite databases

Attachments (8)

trac_pg_backup.patch (1.7 KB ) - added by shanec@… 11 years ago.
patch to use postgres scheme for backup
trac_pg_backup.2.patch (3.6 KB ) - added by shanec@… 11 years ago.
IBackupProvider interface
trac_pg_backup.3.patch (3.6 KB ) - added by Shane Caraveo <shanec@…> 11 years ago.
Implementation of IBackupProvier
backup.patch (8.5 KB ) - added by Shane Caraveo <shanec@…> 10 years ago.
general backup implementation
backup.2.patch (10.4 KB ) - added by Shane Caraveo <shanec@…> 10 years ago.
revised backup patch with test case
backup.3.patch (14.1 KB ) - added by shanec@… 10 years ago.
now including mysql backup
backup.3.2.patch (13.7 KB ) - added by Shane Caraveo <shanec@…> 10 years ago.
fixed full patch with new backup
2304-proposed-changes-r8038.patch (7.5 KB ) - added by Remy Blank 10 years ago.
Proposed changes against [8038] as per comment:12.

Download all attachments as: .zip

Change History (31)

comment:1 by Matthew Good, 14 years ago

Resolution: duplicate
Status: newclosed

This is a duplicate of #1741

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

comment:2 by Martin <martin@…>, 12 years ago

Resolution: duplicate
Status: closedreopened

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.

comment:3 by Piotr Kuczynski <piotr.kuczynski@…>, 11 years ago

Component: generaladmin/console
Keywords: postgresql install added
Milestone: 0.12

by shanec@…, 11 years ago

Attachment: trac_pg_backup.patch added

patch to use postgres scheme for backup

comment:4 by anonymous, 11 years ago

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.

by shanec@…, 11 years ago

Attachment: trac_pg_backup.2.patch added

IBackupProvider interface

comment:5 by shanec@…, 11 years ago

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.

comment:6 by shanec@…, 11 years ago

Summary: Not possible to "upgrade" PostgreSQL databaseNot possible to "upgrade" PostgreSQL database [PATCH]

by Shane Caraveo <shanec@…>, 11 years ago

Attachment: trac_pg_backup.3.patch added

Implementation of IBackupProvier

comment:7 by Shane Caraveo <shanec@…>, 11 years ago

Cc: shanec@… added

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.

comment:8 by Shane Caraveo <shanec@…>, 11 years ago

Version: 0.90.11.2.1

comment:9 by Remy Blank, 11 years ago

Milestone: 0.130.12
Owner: changed from Jonas Borgström to Remy Blank
Status: reopenednew

Looks very interesting.

by Shane Caraveo <shanec@…>, 10 years ago

Attachment: backup.patch added

general backup implementation

by Shane Caraveo <shanec@…>, 10 years ago

Attachment: backup.2.patch added

revised backup patch with test case

by shanec@…, 10 years ago

Attachment: backup.3.patch added

now including mysql backup

by Shane Caraveo <shanec@…>, 10 years ago

Attachment: backup.3.2.patch added

fixed full patch with new backup

comment:10 by Christian Boos, 10 years ago

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

in reply to:  10 comment:11 by anonymous, 10 years ago

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.

comment:12 by Remy Blank, 10 years ago

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.

by Remy Blank, 10 years ago

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

comment:13 by Remy Blank, 10 years ago

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.

in reply to:  12 ; comment:14 by Christian Boos, 10 years ago

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.

in reply to:  14 comment:15 by Remy Blank, 10 years ago

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.

comment:16 by Remy Blank, 10 years ago

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.

comment:17 by Christian Boos, 10 years ago

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 ;-)

comment:18 by Christian Boos, 10 years ago

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.

comment:19 by Christian Boos, 10 years ago

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.

comment:20 by Remy Blank, 10 years ago

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.

comment:21 by Christian Boos, 10 years ago

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

comment:22 by Christian Boos, 10 years ago

Keywords: mysql added
Milestone: 0.120.11.5
Resolution: fixed
Status: newclosed

Landed on 0.11-stable in r8152.

comment:23 by Ryan J Ollos, 4 years ago

Keywords: postgresql mysql install → postgresql, mysql, install

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.