0004715: Inconsistent use of transactions in migration scripts may cause unexpected problems

View Issue Details [ Jump to Notes ]

[ Issue History ] [ Print ]

Project

Category

View Status

Date Submitted

Last Update

0004715

opensim

[GRID] Asset Service

public

2010-05-19 12:50

2011-08-27 04:19

Reporter

AlexRa

Assigned To

Priority

normal

Severity

feature

Reproducibility

N/A

Status

acknowledged

Resolution

open

Platform

OS Version

Product Version

master (dev code)

Target Version

Fixed in Version

Summary

0004715: Inconsistent use of transactions in migration scripts may cause unexpected problems

Description

The code in Migrations.cs has the following catch block after executing a command:

catch (Exception e)
{
    m_log.DebugFormat("[MIGRATIONS] Cmd was {0}", e.Message.Replace("\n", " "));
    m_log.Debug("[MIGRATIONS]: An error has occurred...");
    ExecuteScript("ROLLBACK;");
}

The block is supposed to log the error and continue with the migration.

However, the ROLLBACK at the end might itself throw an exception when there is
no active transaction. This means that absolutely any portion of the migration script must be surrounded by BEGIN .. COMMIT. However, the rules of what kinds of SQL can or can not be executed within a transaction vary for various databases. For example, in MySQL schema changes (add column, etc) are not rolled back. Also a single SQL statement is rolled back automatically and doesn't require an explicit transaction.

It seems unwise to have a hardcoded "ROLLBACK" for scripts that are written at different times, by different people and may or may not run within a transaction.

As it is, there are migration sections for various stores/DBs that are not surrounded by a transaction. If any of that fails, the migration will be aborted with an exception rather than continued.

I'm not sure what a perfect solution would be, but one possibility may be this:

* make the Migration class recognize some kind of ":TRANS" marker in the script,
starting an explicit transaction from that point on to the next ":TRANS" or to the end of the script section. Fully manage these transactions within the Migration code.

* remove all explicit BEGINs/COMMITs from all migrations for all DBs. Instead, use the ":TRANS" marker and only when it is necessary (multiple commands, actually transactional).

This looks like something for me to fix, but I'd like this report to be
reviewed and confirmed first.

(Just in case: this issue is NOT a result of my rewriting of the Migration
class in the latest commits. The ROLLBACK has been there for a long time!)

Tags

No tags attached.

Git Revision or version number

12808

Run Mode

Standalone (1 Region)

Physics Engine

BasicPhysics

Environment

.NET / Windows32

Mono Version

None

Viewer

Attached Files

Relationships

Notes
(0015478) melanie (administrator) 2010-05-19 18:24	The old code didn't check for errors thrown by that rollback, so if it wasn't proper, it would be ignored. Also, there is a contract: Any migration MUST be enclosed in an EXPLICIT transaction. No migration can include both schema changes and data manipulation So, the rollback will roll back the data manipulation (insert into xxx select fff from yyy) types of migration. it will not do anything for the schema alterations. However, since the data manipulation migrations, which are expected to fail in certain cases, can never be combined with schema manipulation, this is acceptable. Schema manipulation is not expected to fail. If it does, it means the user has manually modified their database, which means they're on their own. That is how it was designed. How to change it, is up for discussion.

(0019745) makopoppo (manager) 2011-08-27 04:19	Probably it is still in open discussion for architecture proposal.

Issue History
Date Modified	Username	Field	Change
2010-05-19 12:50	AlexRa	New Issue
2010-05-19 12:50	AlexRa	Git Revision	=> 12808
2010-05-19 12:50	AlexRa	SVN Revision	=> 0
2010-05-19 12:50	AlexRa	Run Mode	=> Standalone (1 Region)
2010-05-19 12:50	AlexRa	Physics Engine	=> BasicPhysics
2010-05-19 12:50	AlexRa	Environment	=> .NET / Windows32
2010-05-19 12:50	AlexRa	Mono Version	=> None
2010-05-19 18:24	melanie	Note Added: 0015478
2011-08-27 04:19	makopoppo	Note Added: 0019745
2011-08-27 04:19	makopoppo	Severity	minor => feature
2011-08-27 04:19	makopoppo	Reproducibility	sometimes => N/A
2011-08-27 04:19	makopoppo	Status	new => acknowledged