Skip to content

Add local copy of CommitWriteBuffer with rollback in WriteProcessor - #544

Open
cyrusknopf wants to merge 1 commit into
mainfrom
bug/writeproc-triage
Open

Add local copy of CommitWriteBuffer with rollback in WriteProcessor#544
cyrusknopf wants to merge 1 commit into
mainfrom
bug/writeproc-triage

Conversation

@cyrusknopf

@cyrusknopf cyrusknopf commented Mar 25, 2026

Copy link
Copy Markdown
Contributor

This is more a proof of concept of a fix that needs to be added.

It may be possible that WriteProcessor::execute modifies the CommitWriteBuffer of the current change, and errors in the same cycle. This violates the atomicity of a single query. This PR adds a naïve fix of keeping a copy of the CommitWriteBuffer on prepare, and rolling back to the local copy before throwing any exceptions to be caught by the QueryInterpreter.

Implemented in this PR by using Result types in the WriteProcessor instead of exceptions. This means we have minimal try ... catch blocks, and that errors can be propagated up to a single point, where the rollback occurs if it needs to, and then finally an exception can be thrown to be caught by QueryInterpreter.

@cyrusknopf cyrusknopf self-assigned this Mar 25, 2026
@cyrusknopf
cyrusknopf marked this pull request as ready for review March 26, 2026 11:21
@cyrusknopf
cyrusknopf requested a review from rjb32 as a code owner March 26, 2026 11:21
Comment on lines +186 to +192
try {
Dispatcher::dispatch(valueColumn, toVec);
} catch (const std::exception& e) {
return WriteError {e.what()};
} catch (...) {
return WriteError {"Unknown error."};
}

@cyrusknopf cyrusknopf Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I think this try ... catch might be unavoidable, without adding a result type to the functor passed to the dispatcher

throw FatalException(fmt::format("Attempted to create edge {} with "
"target node with no such column: {}",
edge._name, edge._tgtTag.getValue()));
return WriteError {fmt::format("Attempted to create edge {} with "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use the construction style with parenthesis

}
} else { // Column(Opt)Vector -> unique property for each node
getUntypedProperties(valueColumn, propsBuffer, pid);
WriteResult res = getUntypedProperties(valueColumn, propsBuffer, pid);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can res be const?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants