New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong transaction management #50

Open
david-garcia-garcia opened this Issue May 15, 2015 · 19 comments

Comments

6 participants
@david-garcia-garcia

david-garcia-garcia commented May 15, 2015

Still need to confirm and create test script, but looks like if during a PDO transaction if a PDO exception is thrown (even if the exception is catched by the user) the transaction will automatically rollback.

In pseudocode:

$connection->startTransaction();

$connection->query('DO SOMETHING');

try {
$connection->query('DO SOMETHING ELSE');
} catch() {

}

$connection->commitTransaction();

If the 'DO SOMETHING ELSE' throws ann exception, the transaction is rollbacked.

This was not happening without buffered queries.

@meet-bhagdev

This comment has been minimized.

Show comment
Hide comment
@meet-bhagdev

meet-bhagdev May 22, 2015

Contributor

Could you give us the exact query/script you are using. I tried a simple query against my database and got a the expected behavior.
I am using the php_pdo_sqlsrv_56_nts driver.

Contributor

meet-bhagdev commented May 22, 2015

Could you give us the exact query/script you are using. I tried a simple query against my database and got a the expected behavior.
I am using the php_pdo_sqlsrv_56_nts driver.

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Sep 4, 2015

Here you go. Simple repro script! The driver is automatically doing rollbacks when a PDO exception is thrown, even if the exception is catched by the user.

http://pastebin.com/EbCXt1YU

It would be great if you could look at this ASAP and at least confirm that it can be reproduced,

david-garcia-garcia commented Sep 4, 2015

Here you go. Simple repro script! The driver is automatically doing rollbacks when a PDO exception is thrown, even if the exception is catched by the user.

http://pastebin.com/EbCXt1YU

It would be great if you could look at this ASAP and at least confirm that it can be reproduced,

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Sep 12, 2015

Can you confirm if the repro script worked?

I'm insisting on this because it is potentially affecting the only maintained Drupal-MSSQL integration with loss of data.

https://www.drupal.org/project/sqlsrv

Thanks for your time.

david-garcia-garcia commented Sep 12, 2015

Can you confirm if the repro script worked?

I'm insisting on this because it is potentially affecting the only maintained Drupal-MSSQL integration with loss of data.

https://www.drupal.org/project/sqlsrv

Thanks for your time.

@Lfsantos

This comment has been minimized.

Show comment
Hide comment
@Lfsantos

Lfsantos Sep 13, 2015

Jason, can you answer Davvid's question?

From: Davvid <notifications@github.commailto:notifications@github.com>
Reply-To: Azure/msphpsql <reply@reply.github.commailto:reply@reply.github.com>
Date: Saturday, September 12, 2015 at 7:40 AM
To: Azure/msphpsql <msphpsql@noreply.github.commailto:msphpsql@noreply.github.com>
Subject: Re: [msphpsql] Wrong transaction management (#50)

Can you confirm if the repro script worked?

I'm insisting on this because it is potentially affecting the only maintained Drupal-MSSQL integration with loss of data.

https://www.drupal.org/project/sqlsrvhttps://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fwww.drupal.org%2fproject%2fsqlsrv&data=01%7c01%7clfsantos%40microsoft.com%7c225b65cccadc47d9707d08d2bb8014bd%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=OzDFHPSYbWaVNTlTqrK1rfX13X0IB7VKRSfyGEVgoYY%3d

Thanks for your time.

Reply to this email directly or view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fgithub.com%2fAzure%2fmsphpsql%2fissues%2f50%23issuecomment-139776709&data=01%7c01%7clfsantos%40microsoft.com%7c225b65cccadc47d9707d08d2bb8014bd%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=R2TiAV2LL8g8UE%2fVGSX3JGqMXpRUo9HBqJ98%2bLdmr9M%3d.

Lfsantos commented Sep 13, 2015

Jason, can you answer Davvid's question?

From: Davvid <notifications@github.commailto:notifications@github.com>
Reply-To: Azure/msphpsql <reply@reply.github.commailto:reply@reply.github.com>
Date: Saturday, September 12, 2015 at 7:40 AM
To: Azure/msphpsql <msphpsql@noreply.github.commailto:msphpsql@noreply.github.com>
Subject: Re: [msphpsql] Wrong transaction management (#50)

Can you confirm if the repro script worked?

I'm insisting on this because it is potentially affecting the only maintained Drupal-MSSQL integration with loss of data.

https://www.drupal.org/project/sqlsrvhttps://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fwww.drupal.org%2fproject%2fsqlsrv&data=01%7c01%7clfsantos%40microsoft.com%7c225b65cccadc47d9707d08d2bb8014bd%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=OzDFHPSYbWaVNTlTqrK1rfX13X0IB7VKRSfyGEVgoYY%3d

Thanks for your time.

Reply to this email directly or view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3a%2f%2fgithub.com%2fAzure%2fmsphpsql%2fissues%2f50%23issuecomment-139776709&data=01%7c01%7clfsantos%40microsoft.com%7c225b65cccadc47d9707d08d2bb8014bd%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=R2TiAV2LL8g8UE%2fVGSX3JGqMXpRUo9HBqJ98%2bLdmr9M%3d.

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Sep 15, 2015

Please, feedback on this. I've double checked the repro script and unless there is a flaw that I cannot see in it, this a very nasty PDO driver bug.

Just noticed that the script fails if the table 'PEOPLE' does not exist in the database when it tries to DROP and RECREATE.

Just comment the DROP statement on the first run of the test, or put the DROP inside a try/catch.

david-garcia-garcia commented Sep 15, 2015

Please, feedback on this. I've double checked the repro script and unless there is a flaw that I cannot see in it, this a very nasty PDO driver bug.

Just noticed that the script fails if the table 'PEOPLE' does not exist in the database when it tries to DROP and RECREATE.

Just comment the DROP statement on the first run of the test, or put the DROP inside a try/catch.

@meet-bhagdev

This comment has been minimized.

Show comment
Hide comment
@meet-bhagdev

meet-bhagdev Sep 15, 2015

Contributor

We are aggressively working with the experts on the PDO driver. Really sorry for the delay. Hang in there and you will hear from us.

Contributor

meet-bhagdev commented Sep 15, 2015

We are aggressively working with the experts on the PDO driver. Really sorry for the delay. Hang in there and you will hear from us.

@meet-bhagdev

This comment has been minimized.

Show comment
Hide comment
@meet-bhagdev

meet-bhagdev Sep 17, 2015

Contributor

Hi @david-garcia-garcia
We were able to reproduce it. We are currently trying to understand what caused this issue. Will keep you posted.

Thanks,
Meet

Contributor

meet-bhagdev commented Sep 17, 2015

Hi @david-garcia-garcia
We were able to reproduce it. We are currently trying to understand what caused this issue. Will keep you posted.

Thanks,
Meet

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Sep 18, 2015

@meet-bhagdev

Thanks! Hope it does not take long to resolve, this is having very bad (and difficult to detect and diagnose) consecuences on the Drupal-MSSQL integration.

david-garcia-garcia commented Sep 18, 2015

@meet-bhagdev

Thanks! Hope it does not take long to resolve, this is having very bad (and difficult to detect and diagnose) consecuences on the Drupal-MSSQL integration.

@meet-bhagdev

This comment has been minimized.

Show comment
Hide comment
@meet-bhagdev

meet-bhagdev Sep 25, 2015

Contributor

@david-garcia-garcia I got an answer from the driver authors. Let me know if this helps:

According to the experts, from the PHP script perspective, it may SEEM like this is a loss of data issue, and, depending on how the transaction is treated, it could lead to data processing errors, PDO and our driver underneath are acting exactly as they should/are designed to.

In the event of an error, uncommitted batched data is automatically rolled back. This behavior is described here: http://php.net/manual/en/pdo.transactions.php, and is congruous with the way that SQL Server works. The following SQL script also demonstrates the behavior in a less abstract form than the PHP script:

USE test;
GO
UPDATE PEOPLE SET age = 18 WHERE name = 'David';
GO
BEGIN TRANSACTION
SELECT TOP 10 * FROM PEOPLE;
UPDATE PEOPLE SET age = 16 WHERE name = 'David';
SELECT TOP 10 * FROM PEOPLE;
UPDATE PEOPLE SET age = 'A' WHERE name = 'David';
SELECT TOP 10 * FROM PEOPLE;
COMMIT TRANSACTION
GO
SELECT TOP 10 * FROM PEOPLE;
GO

The results for the above queries are as follows:
18
16
18

Please let me know if this clarifies anything, if not we will further try to figure out how we can make it better. Additionally, would it be possible for you to get on a call with us? We want to get a better understanding of some of the issues you are facing and help you out.

Thanks,
Meet

Contributor

meet-bhagdev commented Sep 25, 2015

@david-garcia-garcia I got an answer from the driver authors. Let me know if this helps:

According to the experts, from the PHP script perspective, it may SEEM like this is a loss of data issue, and, depending on how the transaction is treated, it could lead to data processing errors, PDO and our driver underneath are acting exactly as they should/are designed to.

In the event of an error, uncommitted batched data is automatically rolled back. This behavior is described here: http://php.net/manual/en/pdo.transactions.php, and is congruous with the way that SQL Server works. The following SQL script also demonstrates the behavior in a less abstract form than the PHP script:

USE test;
GO
UPDATE PEOPLE SET age = 18 WHERE name = 'David';
GO
BEGIN TRANSACTION
SELECT TOP 10 * FROM PEOPLE;
UPDATE PEOPLE SET age = 16 WHERE name = 'David';
SELECT TOP 10 * FROM PEOPLE;
UPDATE PEOPLE SET age = 'A' WHERE name = 'David';
SELECT TOP 10 * FROM PEOPLE;
COMMIT TRANSACTION
GO
SELECT TOP 10 * FROM PEOPLE;
GO

The results for the above queries are as follows:
18
16
18

Please let me know if this clarifies anything, if not we will further try to figure out how we can make it better. Additionally, would it be possible for you to get on a call with us? We want to get a better understanding of some of the issues you are facing and help you out.

Thanks,
Meet

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Sep 25, 2015

@meet-bhagdev Thanks for looking into the issue!

Amazingly, I was not expecting this:

http://blogs.msdn.com/b/sqlserverfaq/archive/2011/05/11/errors-raised-with-severity-level-16-may-cause-transactions-into-doomed-state.aspx

http://stackoverflow.com/questions/18787678/sql-transaction-uncommittable-while-using-try-catch-why

There is a fundamental and critical difference between my sample script and the SQL script you posted. Your SQL script is never reaching the COMMIT TRANSACTION because an exception is thrown on the previous SQL statement. If you enclose the failing statement inside a TRY CATCH block, when calling COMMIT TRANSACTION you at least get an error:

Msg 3930, Level 16, State 1, Line 21
The current transaction cannot be committed and cannot support operations that write to the log file. Roll back the transaction.
Msg 3998, Level 16, State 1, Line 4
Uncommittable transaction is detected at the end of the batch. The transaction is rolled back.

But on the PHP side (my sample script) you can call commit and anything done after the first PDOException is actually commited.

Even though this might be an expected behaviour or MSSQL limitation, the driver SHOULD be complaining with a PDOException when $connection->commit() is being called with a message such as this one:

Uncommittable transaction is detected at the end of the batch. The transaction is rolled back.

The fact that it silently swallows the $connection->commit() makes this a very dangerous situation. It is not only swallowing the commit() statement but it actually does commit any further operations done inside the scope of the original transaction as if nothing had happened!

I did some tests and it looks like internally the driver is rolling back the transaction and starting a new one. This is unreliable, confusing and error prompt.

Assuming this is an expected behaviour (some PDOExceptions will DOOM the current transaction even if inside a TRY-CATCH block), some other things are wrong:

  1. Why is the user allowed to commit the doomed transaction?
  2. Why are further operations after the "doom" happens treated as transactional?

And still, after reading the PDO documentation link you provided:

http://php.net/manual/en/pdo.transactions.php

I believe it does not match the driver behaviour (but I understand this might be a MSSQL limitation).

This is what it says:

When the script ends or when a connection is about to be closed, if you have an outstanding transaction, PDO will automatically roll it back.

In the provided sample script the connection is not closed netiher the script has ended.

Unluckily I can simply not deal with this behaviour and I am sure (not tested though) this is not happening on MySQL. I maintain the Drupal SQL Server integration, but not 3d party code such as core or contrib that might rely on PDO Exceptions thrown inside a TRY-CATCH block not to doom the whole transaction.

As I see this there are 2 things that need to be done here:

1 - Make sure that the PDO driver will not allow calls to $connection->commit() after the transaction is doomed. The provided sample script should be throwing a PDO exception when commit() is called. Se the proposed error message (that is already used by SQL Server).

Indeed, I would not even wait for the call to commit(). Any further attempt to issue an SQL Statement after the transaction is doomed (something that the user is of course not aware of) should throw an exception so that the user inmediately knows that something went wrong.

Fixing this will, at least, make the driver behave in a consistent, safe and understandable manner.

This is extremely important and high priority because, right now, I don't know how many 3d party code is relying on PDOExceptions inside Try-Catch blocks expecting this not to doom the whole transaction. If the driver threw an exception on such a situation, at least I will not get inconsistent data (or loss of data).

2 - WIth more time, see if it will be possible to make PDOExceptions inside Try-Catch blocks not to completely mess up the enclosing transaction. After all, that is why the user is using a Try-Catch block....

would it be possible for you to get on a call with us?

As my english permits :) I barely have 2 hour time difference from Redmond, just drop me a private message on my Drupal contact form if you feel this is really necessary (https://www.drupal.org/u/david_garcia). I already have direct e-mail contact with other MS-PHP related teams.

david-garcia-garcia commented Sep 25, 2015

@meet-bhagdev Thanks for looking into the issue!

Amazingly, I was not expecting this:

http://blogs.msdn.com/b/sqlserverfaq/archive/2011/05/11/errors-raised-with-severity-level-16-may-cause-transactions-into-doomed-state.aspx

http://stackoverflow.com/questions/18787678/sql-transaction-uncommittable-while-using-try-catch-why

There is a fundamental and critical difference between my sample script and the SQL script you posted. Your SQL script is never reaching the COMMIT TRANSACTION because an exception is thrown on the previous SQL statement. If you enclose the failing statement inside a TRY CATCH block, when calling COMMIT TRANSACTION you at least get an error:

Msg 3930, Level 16, State 1, Line 21
The current transaction cannot be committed and cannot support operations that write to the log file. Roll back the transaction.
Msg 3998, Level 16, State 1, Line 4
Uncommittable transaction is detected at the end of the batch. The transaction is rolled back.

But on the PHP side (my sample script) you can call commit and anything done after the first PDOException is actually commited.

Even though this might be an expected behaviour or MSSQL limitation, the driver SHOULD be complaining with a PDOException when $connection->commit() is being called with a message such as this one:

Uncommittable transaction is detected at the end of the batch. The transaction is rolled back.

The fact that it silently swallows the $connection->commit() makes this a very dangerous situation. It is not only swallowing the commit() statement but it actually does commit any further operations done inside the scope of the original transaction as if nothing had happened!

I did some tests and it looks like internally the driver is rolling back the transaction and starting a new one. This is unreliable, confusing and error prompt.

Assuming this is an expected behaviour (some PDOExceptions will DOOM the current transaction even if inside a TRY-CATCH block), some other things are wrong:

  1. Why is the user allowed to commit the doomed transaction?
  2. Why are further operations after the "doom" happens treated as transactional?

And still, after reading the PDO documentation link you provided:

http://php.net/manual/en/pdo.transactions.php

I believe it does not match the driver behaviour (but I understand this might be a MSSQL limitation).

This is what it says:

When the script ends or when a connection is about to be closed, if you have an outstanding transaction, PDO will automatically roll it back.

In the provided sample script the connection is not closed netiher the script has ended.

Unluckily I can simply not deal with this behaviour and I am sure (not tested though) this is not happening on MySQL. I maintain the Drupal SQL Server integration, but not 3d party code such as core or contrib that might rely on PDO Exceptions thrown inside a TRY-CATCH block not to doom the whole transaction.

As I see this there are 2 things that need to be done here:

1 - Make sure that the PDO driver will not allow calls to $connection->commit() after the transaction is doomed. The provided sample script should be throwing a PDO exception when commit() is called. Se the proposed error message (that is already used by SQL Server).

Indeed, I would not even wait for the call to commit(). Any further attempt to issue an SQL Statement after the transaction is doomed (something that the user is of course not aware of) should throw an exception so that the user inmediately knows that something went wrong.

Fixing this will, at least, make the driver behave in a consistent, safe and understandable manner.

This is extremely important and high priority because, right now, I don't know how many 3d party code is relying on PDOExceptions inside Try-Catch blocks expecting this not to doom the whole transaction. If the driver threw an exception on such a situation, at least I will not get inconsistent data (or loss of data).

2 - WIth more time, see if it will be possible to make PDOExceptions inside Try-Catch blocks not to completely mess up the enclosing transaction. After all, that is why the user is using a Try-Catch block....

would it be possible for you to get on a call with us?

As my english permits :) I barely have 2 hour time difference from Redmond, just drop me a private message on my Drupal contact form if you feel this is really necessary (https://www.drupal.org/u/david_garcia). I already have direct e-mail contact with other MS-PHP related teams.

@meet-bhagdev

This comment has been minimized.

Show comment
Hide comment
@meet-bhagdev

meet-bhagdev Sep 28, 2015

Contributor

Hi @david-garcia-garcia ,
Thanks for such a thorough response. It helps us further investigate this issue. We apologize for the inconvenience this is causing and hope that we can work through it.

Is this the contact form I should use to schedule a call? http://www.drupalonwindows.com/en/content/contact

I am also adding @Lfsantos as he may have something to add here.

Thanks,
Meet

Contributor

meet-bhagdev commented Sep 28, 2015

Hi @david-garcia-garcia ,
Thanks for such a thorough response. It helps us further investigate this issue. We apologize for the inconvenience this is causing and hope that we can work through it.

Is this the contact form I should use to schedule a call? http://www.drupalonwindows.com/en/content/contact

I am also adding @Lfsantos as he may have something to add here.

Thanks,
Meet

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Sep 29, 2015

@meet-bhagdev No that's my hobby site :)

This is the Drupal.org profile:

https://www.drupal.org/u/david_garcia

You need to be registered in Drupal.org to see the contact form and be able to contact another Drupal.org member.

But... use this instead:

deivid dot garcia dot garcia at gmail dot com

Greetings,

david-garcia-garcia commented Sep 29, 2015

@meet-bhagdev No that's my hobby site :)

This is the Drupal.org profile:

https://www.drupal.org/u/david_garcia

You need to be registered in Drupal.org to see the contact form and be able to contact another Drupal.org member.

But... use this instead:

deivid dot garcia dot garcia at gmail dot com

Greetings,

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Nov 4, 2015

I am able to "workaround" the issue by proxying all over both PDOStatement and PDO class and detecting when this messy situation takes place.

All PDO Exceptions are equal, but some are more equal than others :)

Basically, not every kind of PDO Exception will trigger the issue (automatic rollback and issue of new transaction).

Is there a consistent way (such as a specific code list) of knowing which PDO Exceptions trigger the issue?

david-garcia-garcia commented Nov 4, 2015

I am able to "workaround" the issue by proxying all over both PDOStatement and PDO class and detecting when this messy situation takes place.

All PDO Exceptions are equal, but some are more equal than others :)

Basically, not every kind of PDO Exception will trigger the issue (automatic rollback and issue of new transaction).

Is there a consistent way (such as a specific code list) of knowing which PDO Exceptions trigger the issue?

@yitam

This comment has been minimized.

Show comment
Hide comment
@yitam

yitam Mar 9, 2017

Member

Hi @david-garcia-garcia the repro script is no longer available. Since you mentioned that not all PDO Exceptions would trigger automatic rollback, it will help us if you can provide a repro scenario. Looking forward to hearing from you.

Member

yitam commented Mar 9, 2017

Hi @david-garcia-garcia the repro script is no longer available. Since you mentioned that not all PDO Exceptions would trigger automatic rollback, it will help us if you can provide a repro scenario. Looking forward to hearing from you.

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Mar 10, 2017

repro script is no longer available.

Will have to build a new one. Just hang on for a while. I have that covered in a test-script, but the script is built on top of a framework. Will have to distill it back to plain and basic php.

david-garcia-garcia commented Mar 10, 2017

repro script is no longer available.

Will have to build a new one. Just hang on for a while. I have that covered in a test-script, but the script is built on top of a framework. Will have to distill it back to plain and basic php.

@Hadis-Fard

This comment has been minimized.

Show comment
Hide comment
@Hadis-Fard

Hadis-Fard Mar 10, 2017

Member

@david-garcia-garcia Thanks, keep us posted.

Member

Hadis-Fard commented Mar 10, 2017

@david-garcia-garcia Thanks, keep us posted.

@david-garcia-garcia

This comment has been minimized.

Show comment
Hide comment
@david-garcia-garcia

david-garcia-garcia Mar 11, 2017

http://pastebin.com/nYpeTydh

That's it. Hope it helps. This is the kind of bug that is extremely hard to track down (when you don't know what is going on).

david-garcia-garcia commented Mar 11, 2017

http://pastebin.com/nYpeTydh

That's it. Hope it helps. This is the kind of bug that is extremely hard to track down (when you don't know what is going on).

@dsgrillo

This comment has been minimized.

Show comment
Hide comment
@dsgrillo

dsgrillo Dec 18, 2017

Any news on this?
I'm facing a similar issue.
In my case, the transaction is rolled back by the driver (I suppose) even though the PDOException is catched (in my case, I was trying to insert an invalid date - [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Conversion failed when converting date and/or time from character string.

dsgrillo commented Dec 18, 2017

Any news on this?
I'm facing a similar issue.
In my case, the transaction is rolled back by the driver (I suppose) even though the PDOException is catched (in my case, I was trying to insert an invalid date - [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Conversion failed when converting date and/or time from character string.

@yitam

This comment has been minimized.

Show comment
Hide comment
@yitam

yitam Dec 18, 2017

Member

@dsgrillo could you please create a new issue with details (versions, env, etc.) and if you like, you can reference this issue as related. Please also list your repro steps or even better, provide a repro script.

Member

yitam commented Dec 18, 2017

@dsgrillo could you please create a new issue with details (versions, env, etc.) and if you like, you can reference this issue as related. Please also list your repro steps or even better, provide a repro script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment