-
-
Notifications
You must be signed in to change notification settings - Fork 262
Add flush sequences to replication API #8858
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
base: master
Are you sure you want to change the base?
Conversation
|
In practice this PR avoids inconsistent replica when applications use an attachment-level sequence cache (IDs are prefetched and distributed among rows being inserted). This does not allow sharing the cached IDs among different attachments, but so far we haven't seen such an use case. Custom replication plugins are free to choose between synchronous or delayed (conditional or unconditional) sequence replication. |
src/jrd/replication/Publisher.cpp
Outdated
| if (replicator) | ||
| { | ||
| FbLocalStatus status; | ||
| replicator->flushSequences(&status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no replicator on commit/rollback, it means that setSequence was never called and there is nothing to flush. Replication plugin already has an opportunity to push changed sequences to replica on rollback, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no
replicatoron commit/rollback, it means thatsetSequencewas never called and there is nothing to flush.
Yes, it can be change to attachment->att_replicator
Replication plugin already has an opportunity to push changed sequences to replica on rollback, right?
Previously, when a transaction was rollback, sequences were not transferred to the replica, only during the first commit in the current attachment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, when a transaction was rollback, sequences were not transferred to the replica, only during the first commit in the current attachment.
But it was the choice of the standard replication plugin (only). Why API changes are required if you can limit changes to plugin code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, when a transaction was rollback, sequences were not transferred to the replica, only during the first commit in the current attachment.
But it was the choice of the standard replication plugin (only). Why API changes are required if you can limit changes to plugin code?
Current API does not allow to replicate generators changed in read-only (de-facto) transactions, because the transaction replicator object is not created without I/U/D ops and thus there's no commit/rollback event at the end. Thus a need for the new API hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it was the choice of the standard replication plugin (only). Why API changes are required if you can limit changes to plugin code?
This is an optimization of the current plugin implementation, where generator values are first cached and only transmitted to the replica when commit, for example:
select gen_id(t_gen, 1) from rdb$database;
select gen_id(t_gen, 1) from rdb$database;
select gen_id(t_gen, 1) from rdb$database;
commit;
As a result, the generator will be transmitted to the replica with a value of +3, instead of three times for each operation.
Furthermore, there are situations where the generator value changes without an actual transaction, as in the example with select gen_id. In this case, ReplicatedTransaction doesn't yet exist and commit/rollback isn't called. This is why a new method call was added to Publisher.cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No select can be performed outside of transaction thus no REPL_gen_id can be called without transaction. IMHO, it is enough to change call to getReplicator inside of REPL_gen_id to let standard plugin work. Cleanup of attachment-level replicator is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do I care about is creation of plugin instance for single call that may have nothing to work with. Direct usage of att_replicator can solve it though, as you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
selectcan be performed outside of transaction thus noREPL_gen_idcan be called without transaction. IMHO, it is enough to change call togetReplicatorinside ofREPL_gen_idto let standard plugin work. Cleanup of attachment-level replicator is optional.
setSequence() is a method of IReplicatedSession, not IReplicatedTransaction. Or do you mean just creating a dummy instance of IReplicatedTransaction together with calling IReplicatedSession::setSequence()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setSequence()is a method ofIReplicatedSession, notIReplicatedTransaction.
Oops, I forgot that.
Or do you mean just creating a dummy instance of
IReplicatedTransactiontogether with callingIReplicatedSession::setSequence()?
Yes. BTW, moving of setSequence into ReplTransaction is not an option, right?..
…t ReplicatedTransaction
Add a new method to the replication API to ensure generators are always replicated (both on commit and rollback).
Previously, if execute:
The generator on the master and replica stored different values.
If decide to backport this to 5.0, might consider: