Skip to content

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Noremos
Copy link
Contributor

@Noremos Noremos commented Jul 17, 2025

No description provided.

@@ -341,6 +341,8 @@ namespace Replication
unsigned charset,
const char* schemaSearchPath,
const char* sql);

void storeBlobs(Transaction* transaction, Firebird::IReplicatedRecord* record);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to keep declaration of all overloads in one place so they can be seen with one glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I didn't quite understand the remark. The method does not overload anything

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, my bad. storeBlobs and storeBlob are different names. Sorry.

@hvlad
Copy link
Member

hvlad commented Jul 17, 2025

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
Copy link
Contributor Author

Noremos commented Jul 17, 2025

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
Copy link
Contributor

aafemt commented Jul 17, 2025

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
Copy link
Member

hvlad commented Jul 17, 2025

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
Copy link
Member

hvlad commented Jul 17, 2025

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.

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.

3 participants