Move blob search logic in Replicator into a new method to prevent code duplication

[Noremos]

[hvlad]

Note, updateRecord() should not put blob into log if it was not changed. Thus it should have a bit different code. I can prepare a patch with such improvement (I already done it sometime ago but did not commit yet)

[Noremos]

Note, updateRecord() should not put blob into log if it was not changed. Thus it should have a bit different code. I can prepare a patch with such improvement (I already done it sometime ago but did not commit yet)

I hope the template argument with if constexpr will help split the check without sacrificing performance.

[aafemt]

Note, updateRecord() should not put blob into log if it was not changed.

What it will do if the record is not found on the target side and update to be converted into insert?

[hvlad]

Note, updateRecord() should not put blob into log if it was not changed.

What it will do if the record is not found on the target side and update to be converted into insert?

Good question.

So we have a choice - raise error on replica in (should be very) rare cases or continue to pollute repl logs with duplicated blobs in (should be much more) often cases.

[hvlad]

Note, updateRecord() should not put blob into log if it was not changed. Thus it should have a bit different code. I can prepare a patch with such improvement (I already done it sometime ago but did not commit yet)

I hope the template argument with if constexpr will help split the check without sacrificing performance.

Only if it will not harm code readability. Don't use all and every feature at all possible places just because it exists.

[hvlad]

Note, updateRecord() should not put blob into log if it was not changed.

What it will do if the record is not found on the target side and update to be converted into insert?

Good question.

So we have a choice - raise error on replica in (should be very) rare cases or continue to pollute repl logs with duplicated blobs in (should be much more) often cases.

Or it could be warning, not error, with inserting a NULL into missing blob field - in a hope that next UPDATE will fix it.

@dyemanov, what do you think ?

[aafemt]

Do you have on mind a next update with still unchanged BLOB value?

[hvlad]

Do you have on mind a next update with still unchanged BLOB value?

Dumb question, isn't is ?

[aafemt]

Yes, it is a dumb question with obvious answer. That's why the optimization of unchanged BLOB transfer is attractive but cannot work in multimaster setting when update conflicts are possible.

[dyemanov]

In the initial implementation (RDB v2.6 / v3.0) blob was replicated always if master_priority setting was enabled, otherwise only if blob was changed. This setting was not migrated to FB v4/v5 because it was always turned on in practice for automatic conflict resolution. Thus the current behavior is correct, even if sub-optimal. Or we may return back to the configuration setting (but IIRC it has to be mirrored in the replication protocol).