rollback to savepoint not handled correctly in transaction log

[Joe Daly]

Running the following will cause the value 2 to be inserted into the transaction log it should not be, below is the message

SET AUTOCOMMIT = 0;
CREATE TABLE t1(a INT NOT NULL, PRIMARY KEY(a));

START TRANSACTION;

INSERT INTO t1 VALUES (1);
SAVEPOINT `savept1`;
INSERT INTO t1 VALUES (2);

ROLLBACK TO SAVEPOINT savept1;

COMMIT;

produces a message of:

+PRINT_TRANSACTION_MESSAGE('transaction.log',(select max(entry_offset) from DATA_DICTIONARY.TRANSACTION_LOG_TRANSACTIONS))
+transaction_context {
+ server_id: 1
+ transaction_id: 3
+ START_TIMESTAMP
+ END_TIMESTAMP
+}
+statement {
+ type: INSERT
+ START_TIMESTAMP
+ END_TIMESTAMP
+ insert_header {
+ table_metadata {
+ schema_name: "test"
+ table_name: "t1"
+ }
+ field_metadata {
+ type: INTEGER
+ name: "a"
+ }
+ }
+ insert_data {
+ segment_id: 1
+ end_segment: true
+ record {
+ insert_value: "1"
+ }
+ record {
+ insert_value: "2"
+ }
+ }
+}

[Brian Aker (brianaker)]

We should take a look at this today.

Cheers,
   --Brian

On Aug 12, 2010, at 8:06 AM, Joe Daly <email address hidden> wrote:

> ** Changed in: drizzle/dexter
> Assignee: David Shrewsbury (dshrews) => Joe Daly (skinny.moey)
>
> --
> rollback to savepoint not handled correctly in transaction log
> https://bugs.launchpad.net/bugs/600032
> You received this bug notification because you are a member of Drizzle-
> developers, which is subscribed to Drizzle.
>
> Status in A Lightweight SQL Database for Cloud and Web: Confirmed
> Status in Drizzle dexter series: Confirmed
>
> Bug description:
> Running the following will cause the value 2 to be inserted into the transaction log it should not be, below is the message
>
> SET AUTOCOMMIT = 0;
> CREATE TABLE t1(a INT NOT NULL, PRIMARY KEY(a));
>
> START TRANSACTION;
>
> INSERT INTO t1 VALUES (1);
> SAVEPOINT `savept1`;
> INSERT INTO t1 VALUES (2);
>
> ROLLBACK TO SAVEPOINT savept1;
>
> COMMIT;
>
> produces a message of:
>
> +PRINT_TRANSACTION_MESSAGE('transaction.log',(select max(entry_offset) from DATA_DICTIONARY.TRANSACTION_LOG_TRANSACTIONS))
> +transaction_context {
> + server_id: 1
> + transaction_id: 3
> + START_TIMESTAMP
> + END_TIMESTAMP
> +}
> +statement {
> + type: INSERT
> + START_TIMESTAMP
> + END_TIMESTAMP
> + insert_header {
> + table_metadata {
> + schema_name: "test"
> + table_name: "t1"
> + }
> + field_metadata {
> + type: INTEGER
> + name: "a"
> + }
> + }
> + insert_data {
> + segment_id: 1
> + end_segment: true
> + record {
> + insert_value: "1"
> + }
> + record {
> + insert_value: "2"
> + }
> + }
> +}
>
>

[Joe Daly (skinny.moey)]

this code looks like it got moved around recently, as the behavior has changed the rollback now looks like its not ignored but rolling back the entire transaction rather then to the savepoint in the transaction log.

The last entry in the transaction log is now the create table statement the insert is not logged.

[Joe Daly (skinny.moey)]

The current implementation calls TransactionServices::rollbackTransactionMessage() which clears the current transaction message regardless if the rollback is to a savepoint.

I think the way to fix this would be to add to transaction.proto

For SAVEPOINT:
1. a new type (SAVEPOINT)
2. a new sub message SavepointStatement , would contain savepoint name

For ROLLBACK_TO_SAVEPOINT
1. a new type (ROLLBACK_TO_SAVEPOINT)
2. a new sub message RollbackToSavepointStatement, would contain savepoint name

Then update transaction_services to push one of these messages when a savepoint is encountered in someway.

Comments?

[David Shrewsbury (dshrews)]

I think we could handle this one of two ways:

1) Do exactly as you proposed. We should also consider making ROLLBACK and COMMIT a new type.

2) Batch up all Transaction messages for a single transaction and then modify the messages to eliminate the stuff we need to rollback before sending the messages through the replication channel.

We need to eventually do something like the batching in #2 in order to fix the intermingling of Transaction messages when we have multiple connections (think of a large LOAD DATA running that could create several Transaction messages with the same transaction_id while other clients run smaller transactions). That's going to be interesting to fix. However, I still like #1 as well. Hiding rollbacks just doesn't seem like a good idea to me (side effects?).

[Jay Pipes (jaypipes)]

Hi!

I disagree on this approach. The transaction services layer has, up to this point, followed the principle of not passing *anything* over to the replication stream if a COMMIT did not occur -- the ONLY exception to this was for bulk operations. A RollbackStatement message should be written to the replication stream if and only if:

a) a ROLLBACK SQL statement is executed, AND
b) a bulk operation has already sent a Transaction message into the replication stream

In other words, if an applier receives a RollbackStatement message in a Transaction on its end it can safely assume it may ignore any received Statement messages in that transaction and drop them all.

I would recommend keeping this approach for two reasons:

1) Don't assume an applier knows anything about savepoints. Remember that appliers can also be NoSQL stores. A Rollback should mean "discard anything you've received so far about this transaction"
2) KISS

I would do the following:

In TransactionServices::setSavepoint(), do a std::copy() of existing Transaction message to a saved location. The easiest way to do so would be to modify the drizzled::NamedSavepoint class to contain a member of type pointer to message::Transaction.

In TransactionServices::releaseSavepoint(), remember to clean up the memory allocated in the copy above for the copied Transaction message

In TransactionServices::rollbackToSavepoint(), simply discard the Session's existing Transaction message and set the pointer to the NamedSavepoint's copy of the Transaction message at savepoint time.

Don't send anything over the replication stream in the event of a ROLLBACK TO SAVEPOINT. Just copy/modify the Transaction message for that Session. This will keep the replication stream simple and straightforward, which is A Good Thing.

Cheers!
jay