Conversation
2. Completed table schema restore
… recovery is broken)
Conflicts: src/codegen/updater.cpp src/concurrency/timestamp_ordering_transaction_manager.cpp src/storage/data_table.cpp test/brain/query_logger_test.cpp
| case LogRecordType::TRANSACTION_BEGIN:{ | ||
| if(all_txns_.find(txn_id) != all_txns_.end()){ | ||
| LOG_ERROR("Duplicate transaction"); | ||
| PELOTON_ASSERT(false); |
There was a problem hiding this comment.
In the beginning, upon log file reading failure, the StartRecovery writes a LOG_ERROR and returns. I am wondering whether it should behave the same here - return after writing LOG_ERROR instead of using PELOTON_ASSERT(false) to abort?
| buf_curr += (record_len + sizeof(record_len)); | ||
| buf_rem -= (record_len + sizeof(record_len)); | ||
| } else { | ||
| break; |
There was a problem hiding this comment.
Might directly log an error and abort/return instead of using break.
| LOG_INFO("Replaying TXN_COMMIT"); | ||
| } | ||
|
|
||
| else if(record_type==LogRecordType::TRANSACTION_ABORT) { |
There was a problem hiding this comment.
This check might need to be moved to ParseFromDisk and files an error there.
If a TRANSACTION_ABORT record were found at then at this time, then the previous changes in this transaction would have been made (which should not be, if I got this correctly).
On the other hand, if the transaction could reach here, then it should also have a TRANSACTION_COMMIT record. I wonder if it is possible for a transaction to have both records. If not, then such check can be redundant.
| } | ||
|
|
||
| // Pass 2 | ||
| log_buffer_ = new char[log_buffer_size_]; |
There was a problem hiding this comment.
I wonder if the recovery needs to read the log file twice from the disk. Since log_buffer_ is constructed here after the first pass.
If this is the case, will it be better to populate the log_buffer_ in the first phase and save the second disk read in the second phase?
nwang57
left a comment
There was a problem hiding this comment.
Logging looks good. recovery seems not finished yet. Add some tests for the recovery part so that correctness can be easily verified.
| void PostgresProtocolHandler::GetResult() { | ||
| traffic_cop_->ExecuteStatementPlanGetResult(); | ||
|
|
||
| // traffic_cop_->ExecuteStatementPlanGetResult(); |
| current_txn->GetEpochId(), current_txn->GetTransactionId(), | ||
| current_txn->GetCommitId(), schema_oid); | ||
|
|
||
| record.SetOldItemPointer(location); |
There was a problem hiding this comment.
You already set the old item pointer once during the log record constructor why do you need to set it explicitly?
| LogRecordType::TUPLE_UPDATE, location, new_location, current_txn->GetEpochId(), | ||
| current_txn->GetTransactionId(), current_txn->GetCommitId(), schema_oid); | ||
|
|
||
| record.SetOldItemPointer(location); |
There was a problem hiding this comment.
You already set the old item pointer once during the log record constructor why do you need to set it explicitly?
| LogRecordType::TRANSACTION_COMMIT, current_txn->GetEpochId(), | ||
| current_txn->GetTransactionId(), current_txn->GetCommitId()); | ||
|
|
||
| current_txn->GetLogBuffer()->WriteRecord(record); |
There was a problem hiding this comment.
Will the log_buffer exceeds its threshold after this write?
| LogRecordType::TRANSACTION_ABORT, current_txn->GetEpochId(), | ||
| current_txn->GetTransactionId(), current_txn->GetCommitId()); | ||
|
|
||
| current_txn->GetLogBuffer()->WriteRecord(record); |
There was a problem hiding this comment.
Will the log_buffer exceeds its threshold after this write?
| stream->flush(); | ||
|
|
||
| if(stream->fail()){ | ||
| PELOTON_ASSERT(false); |
There was a problem hiding this comment.
why not PELOTON_ASSERT(!stream->fail())? Do we really want to crash the system if the stream fails or is there any mechanism to rewrite the log to file?
| } | ||
|
|
||
| // Pass 2 | ||
| log_buffer_ = new char[log_buffer_size_]; |
There was a problem hiding this comment.
This memory may not be freed
| if(it->second.first != LogRecordType::TRANSACTION_COMMIT) | ||
| continue; | ||
|
|
||
| auto offset_pair = std::make_pair(curr_txn_offset, curr_txn_offset); |
There was a problem hiding this comment.
Why making a pair of two identical size_t values?
This PR implements logging and recovery for robustness to crashes.
Logging:
TimestampOrderingTransactionManageras executing transactions. Submit the buffer when it is full and then acquire a new one for the current transaction.QUEUINGinCommitTransactionandAbortTransaction.values_buf,values_sizeandoffsetsall the way down toPerformUpdate.Recovery:
txn_id. After two passes, all the transactions are replayed.Create Table,Create Database,InsertandDeletecurrently.