Skip to content

Fix CORE-5173 #9

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 28, 2016
Merged

Fix CORE-5173 #9

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99f4d16
Fix CORE-5173
aafemt Mar 26, 2016
bf87616
Additional protection from savepoint sequence anomalies
aafemt Mar 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 34 additions & 27 deletions src/dsql/StmtNodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,31 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*

if (handlers && handlers->statements.getCount() > 0)
{
// First of all rollback failed work
if (transaction != sysTransaction)
{
count = *request->getImpure<SLONG>(impureOffset);

// Since there occurred an error (req_unwind), undo all savepoints
// up to, _but not including_, the savepoint of this block.
// That's why transaction->rollbackToSavepoint() cannot be used here
// The savepoint of this block will be dealt with below.
// Do this only if error handlers exist. If not - leave rollbacking to caller node
while (transaction->tra_save_point &&
transaction->tra_save_point->sav_next &&
count < transaction->tra_save_point->sav_next->sav_number)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tra.cpp, one condition is ">" and another one is ">=". Is it correct that both conditions in this patch are strict inequalities?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. In tra.cpp rolling forward must stop immediatelly before given savepoint, here - before one savepoint before the given one because there we must rollback the savepoint, here - rollback everything but the savepoint as it must be applied afterward.

transaction->rollforwardSavepoint(tdbb);
}
// There can be no savepoints above the given one
if (transaction->tra_save_point && transaction->tra_save_point->sav_number > count)
{
transaction->rollbackSavepoint(tdbb);
}
// after that we still have to have our savepoint. If not - CORE-4424/4483 is sneaking around
fb_assert(transaction->tra_save_point && transaction->tra_save_point->sav_number == count);
}

temp = parentStmt;
bool handled = false;
const NestConst<StmtNode>* ptr = handlers->statements.begin();
Expand All @@ -575,31 +600,6 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*
temp = handlerNode->action;
exeState->errorPending = false;

if (transaction != sysTransaction)
{
count = *request->getImpure<SLONG>(impureOffset);

// Since there occurred an error (req_unwind), undo all savepoints
// up to, _but not including_, the savepoint of this block.
// That's why transaction->rollbackToSavepoint() cannot be used here
// The savepoint of this block will be dealt with below.
// Do this only if error handlers exist. If not - leave rollbacking to caller node

while (transaction->tra_save_point &&
transaction->tra_save_point->sav_next &&
count < transaction->tra_save_point->sav_next->sav_number)
{
transaction->rollforwardSavepoint(tdbb);
}
// There can be no savepoints above the given one
if (transaction->tra_save_point && transaction->tra_save_point->sav_number > count)
{
transaction->rollbackSavepoint(tdbb);
}
// after that we still have to have our savepoint. If not - CORE-4424/4483 is sneaking around
fb_assert(transaction->tra_save_point && transaction->tra_save_point->sav_number == count);
}

// On entering looper exeState->oldRequest etc. are saved.
// On recursive calling we will loose the actual old
// request for that invocation of looper. Avoid this.
Expand Down Expand Up @@ -647,9 +647,16 @@ const StmtNode* BlockNode::execute(thread_db* tdbb, jrd_req* request, ExeState*

if (handled && transaction != sysTransaction)
{
// Check that exception handlers wee executed in context of right savepoint.
// Check that exception handlers were executed in context of right savepoint.
// If not - mirror copy of CORE-4424 or CORE-4483 is around here.
fb_assert(transaction->tra_save_point && transaction->tra_save_point->sav_number == count);
// Except the case of nested exception handlers and throwing in inner one.
// In this case execution flow is like this:
// inner before (block that rollbacking savepoints above)
// outer before
// outer after (this block)
// inner after
// Because of this following assert is commentd out
//fb_assert(transaction->tra_save_point && transaction->tra_save_point->sav_number == count);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "before" and "after" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are parts of code managing savepoints that are placed before and after call of looper for exception handlers, as written in parentheses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to see the bad execution flow fixed rather than working around with a disabled assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that recursive exceptions handling is a bad thing. It can be "as designed". In any case this is a bigger work for different ticket.

for (const Savepoint* save_point = transaction->tra_save_point;
save_point && count <= save_point->sav_number;
save_point = transaction->tra_save_point)
Expand Down