Skip to content
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

Fix CORE-5252 #29

Merged
merged 2 commits into from
Jun 29, 2016
Merged

Fix CORE-5252 #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d042f60
Fix CORE-5252
aafemt May 26, 2016
2b72824
Postfix for DY refactoring that leaded to loop in savepoint chain
aafemt Jun 14, 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
6 changes: 4 additions & 2 deletions src/jrd/Savepoint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,6 @@ Savepoint* Savepoint::rollforward(thread_db* tdbb, Savepoint* prior)
fb_assert(!m_next->m_next); // check that transaction savepoint is the last in list
// get rid of tx-level savepoint
m_next->rollforward(tdbb);
m_next->release();
m_next = NULL;
}

Expand Down Expand Up @@ -506,7 +505,6 @@ Savepoint* Savepoint::rollforward(thread_db* tdbb, Savepoint* prior)
fb_assert(!m_next->m_next); // check that transaction savepoint is the last in list
// get rid of tx-level savepoint
m_next->rollforward(tdbb);
m_next->release();
m_next = NULL;
}

Expand Down Expand Up @@ -561,10 +559,14 @@ Savepoint* Savepoint::release(Savepoint* prior)
Savepoint* const next = m_next;

if (prior)
{
fb_assert(prior != next);
prior->m_next = next;
}

m_next = m_transaction->tra_save_free;
m_transaction->tra_save_free = this;
fb_assert(m_next != this);

return next;
}
Expand Down
4 changes: 3 additions & 1 deletion src/jrd/Savepoint.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ namespace Jrd
m_number = number;
m_flags |= root ? SAV_root : 0;
m_next = next;
fb_assert(m_next != this);
}

VerbAction* getAction(const jrd_rel* relation) const
Expand Down Expand Up @@ -210,6 +211,7 @@ namespace Jrd

Savepoint* const next = m_next;
m_next = target;
fb_assert(m_next != this);
target = this;
return next;
}
Expand Down Expand Up @@ -298,7 +300,7 @@ namespace Jrd
bool isLarge() const;
Savepoint* release(Savepoint* prior = NULL);

jrd_tra* m_transaction; // transaction this savepoint belongs to
jrd_tra* const m_transaction; // transaction this savepoint belongs to
SavNumber m_number; // savepoint number
USHORT m_flags; // misc flags
USHORT m_count; // active verb count
Expand Down
14 changes: 13 additions & 1 deletion src/jrd/exe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,14 @@ static void execute_looper(thread_db* tdbb,
if (!(request->req_flags & req_proc_fetch) && request->req_transaction)
{
if (transaction && !(transaction->tra_flags & TRA_system))
transaction->startSavepoint();
{
if (request->req_savepoints)
{
request->req_savepoints = request->req_savepoints->moveToStack(transaction->tra_save_point);
}
else
transaction->startSavepoint();
}
}

request->req_flags &= ~req_stall;
Expand All @@ -1033,8 +1040,13 @@ static void execute_looper(thread_db* tdbb,
transaction->tra_save_point->isSystem() &&
!transaction->tra_save_point->isChanging())
{
Savepoint* savepoint = transaction->tra_save_point;
// Forget about any undo for this verb
transaction->rollforwardSavepoint(tdbb);
// Preserve savepoint for reuse
fb_assert(savepoint == transaction->tra_save_free);
transaction->tra_save_free = savepoint->moveToStack(request->req_savepoints);
fb_assert(savepoint != transaction->tra_save_free);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/jrd/req.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ class jrd_req : public pool_alloc<type_req>
ExtEngineManager::ResultSet* req_ext_resultset; // external result set
USHORT req_label; // label for leave
ULONG req_flags; // misc request flags
Savepoint* req_savepoints; // Looper savepoint list
Savepoint* req_proc_sav_point; // procedure savepoint list
Firebird::TimeStamp req_timestamp; // Start time of request

Expand Down
6 changes: 6 additions & 0 deletions src/jrd/tra.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,13 @@ void TRA_attach_request(Jrd::jrd_tra* transaction, Jrd::jrd_req* request)
void TRA_detach_request(Jrd::jrd_req* request)
{
if (!request->req_transaction)
{
fb_assert(!request->req_savepoints);
return;
}

// Release stored looper savepoints
Savepoint::destroy(request->req_savepoints);
Copy link
Contributor Author

@aafemt aafemt Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This line causes engine to crash on test bugs.core_0214 as if savepoint was released from non-existing pool, but I cannot understand how it can be: pointers to transaction and pool are valid.
Changing it to Savepoint::mergeStacks(request->req_transaction->tra_save_free, request->req_savepoints); helps, but I'm afraid that it only put problem off, not fixing it.

Copy link
Member

Choose a reason for hiding this comment

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

On 06/14/2016 01:13 PM, Dimitry Sibiryakov wrote:

     return;
  • }
  • // Release stored looper savepoints
  • Savepoint::destroy(request->req_savepoints);
    This line causes engine to crash on test bugs.core_0214 as if savepoint was released from non-existing pool, but I cannot understand how it can be: pointers to transaction and pool are valid.

Always good idea to provide stack trace in such cases.

Copy link
Member

Choose a reason for hiding this comment

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

On 06/14/2016 01:58 PM, Dimitry Sibiryakov wrote:

engine13.dll!Firebird::MemPool::release(void * object, bool flagDecr) Line 2125 C++

at this frame please print *block and *pool

Copy link
Contributor Author

@aafemt aafemt Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Now I see that block's pool doesn't match savepoint's transaction pool. But savepoint's transaction match request transaction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this block 'pool' pointer is invalid, 'next' pointer is valid, but code handle it as a pointer to pool, that's a reason for crash. Perhaps, single savepoints are not supposed to be freed at all, they will die at once with transaction pool...

Copy link
Member

Choose a reason for hiding this comment

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

On 06/14/2016 04:47 PM, Dimitry Sibiryakov wrote:

     return;
  • }
  • // Release stored looper savepoints
  • Savepoint::destroy(request->req_savepoints);
    For this block 'pool' pointer is invalid, 'next' pointer is valid, but code handle it as a pointer to pool, that's a reason for crash.

That means that code tries to free some block once again.

Perhaps, single savepoints are not supposed to be freed at all, they will die at once with transaction pool...

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/FirebirdSQL/firebird/pull/29/files/057fc3a9ee0a851a39de07ad6497b28ac48ce931#r66972581


// Remove request from the doubly linked list
if (request->req_tra_next)
Expand Down