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

Replication support #182

Merged
merged 14 commits into from Jan 25, 2019

Conversation

Projects
None yet
6 participants
@dyemanov
Copy link
Member

dyemanov commented Jan 8, 2019

Still work in progress, but suitable as feature preview for Beta 1. Discussion will follow in fb-devel, questions may be raised here as well.

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Jan 8, 2019


## Setting up the master side

Replication is configured using a single configuration file: replication.conf. It allows to define global settings as well as per-database settings. All the possible options are listed inside replication.conf, descriptions are provided as comments there. For per-database configuration, full database name must be specified \(aliases or wildcards are not allowed\) inside the {database} section.

This comment has been minimized.

@aafemt

aafemt Jan 8, 2019

Contributor

Why separate config file with the same syntax as databases.conf instead of databases.conf itself? It has no sense to distribute database's parameters into several files.
I would understand if it was plugin's config file, but now it AFAIU is read by engine itself.

This comment has been minimized.

@dyemanov

dyemanov Jan 8, 2019

Author Member

The reasons are partially historical (initial implementation was for v2.5 without per-database configuration) and partially intentional - to separate purely optional feature from the "core" configuration. But it can be changed if your suggestion is supported by others.

This comment has been minimized.

@aafemt

aafemt Jan 8, 2019

Contributor

I think that for users would be more convenient to have all database-related settings in one place.


// Replication interfaces

interface ReplicatedRecord : Versioned

This comment has been minimized.

@aafemt

aafemt Jan 8, 2019

Contributor

I see no method to get record's format or any other way to get separate fields values. Does it exist?

This comment has been minimized.

@dyemanov

dyemanov Jan 8, 2019

Author Member

It doesn't exist yet, but I plan to add it after Beta. Your suggestions are appreciated.

This comment has been minimized.

@aafemt

aafemt Jan 8, 2019

Contributor

Avalerion uses this:

class IField
{
public:
	virtual bool hasData() = 0;
	virtual const char* getName() = 0;
	virtual int getType() = 0;
	virtual int getScale() = 0;
	virtual int getLength() = 0;
	virtual int getCharset() = 0;
	virtual bool isNull() = 0;
	virtual void* getData() = 0;
	virtual bool next() = 0;
};

class IRecord
{
public:
	virtual const char* getSchemaName() = 0;
	virtual const char* getTableName() = 0;
	virtual long long getNumber() = 0;
	virtual IField* getField() = 0;
};

Where IField is actually enumerator. I would like to have also "bool isKey()" in it.

Status getStatus();

ReplicatedTransaction startTransaction(int64 number);
boolean cleanupTransaction(int64 number);

This comment has been minimized.

@aafemt

aafemt Jan 10, 2019

Contributor

Some comments about "what this method is supposed to do and when is called" would be very appreciated.

@aafemt

This comment has been minimized.

Copy link
Contributor

aafemt commented Jan 10, 2019

I see interfaces for CDC, but I don't see a new plugin type for CDC. It was supposed to be a plugin, wasn't it?


boolean storeBlob(ISC_QUAD blobId, ReplicatedBlob blob);

boolean executeSql(const string sql);

This comment has been minimized.

@aafemt

aafemt Jan 10, 2019

Contributor

What is character set of this string? Could it be fixed UTF-8, please?..

This comment has been minimized.

@dyemanov

dyemanov Jan 13, 2019

Author Member

It's fixed UTF-8.

@aafemt

This comment has been minimized.

Copy link
Contributor

aafemt commented Jan 10, 2019

A lot of changes seems to be completely unrelated to replication. Could they go to a separate PR?

@dyemanov

This comment has been minimized.

Copy link
Member Author

dyemanov commented Jan 13, 2019

I see interfaces for CDC, but I don't see a new plugin type for CDC. It was supposed to be a plugin, wasn't it?

Yes, it's expected after Beta 1. You may provide a pull request.

@dyemanov

This comment has been minimized.

Copy link
Member Author

dyemanov commented Jan 13, 2019

A lot of changes seems to be completely unrelated to replication. Could they go to a separate PR?

They're all related, IIRC (even if indirectly).

@aafemt

This comment has been minimized.

Copy link
Contributor

aafemt commented Jan 13, 2019

They're all related, IIRC (even if indirectly).

I really cannot see relation between replication and renaming of Win32DirItr into Win32DirIterator for example.

@dyemanov

This comment has been minimized.

Copy link
Member Author

dyemanov commented Jan 13, 2019

They're all related, IIRC (even if indirectly).

I really cannot see relation between replication and renaming of Win32DirItr into Win32DirIterator for example.

Yep, sorry for that. I used to code a different class in other forks and discovered Win32DirItr only during backporting. As it wasn't used in our codebase before replication, I've made some adjustments (including better readability). I understand they may complicate the review process, but I'd rather prefer to keep them together with replication.

@aafemt

This comment has been minimized.

Copy link
Contributor

aafemt commented Jan 13, 2019

@dyemanov

This comment has been minimized.

Copy link
Member Author

dyemanov commented Jan 13, 2019

It wasn't used because the codebase contains TWO classes for directory iteration.

The one inside PathUtils was created first (circa FB 1.5), then Jim ignored it and created ScanDir, then I ignored both and created DirectoryWalker ;-) I've fixed my mistake now, but I'm not going to fix the remaining duplication (at least not in this PR).

@AlexPeshkoff

This comment has been minimized.

Copy link
Member

AlexPeshkoff commented Jan 13, 2019

@aafemt

This comment has been minimized.

Copy link
Contributor

aafemt commented Jan 13, 2019

@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Jan 14, 2019

# Connection string to the replica database (used for synchronous replication only).
# Expected format:
#
# [<login>:<password>@]<database connection string>

This comment has been minimized.

@asfernandes

asfernandes Jan 24, 2019

Member

What about login and password with "special" characters as : and @?

This comment has been minimized.

@dyemanov

dyemanov Jan 24, 2019

Author Member

Currently, you may omit both and use ISC_USER/ISC_PASSWORD instead. I can also add support for the quotes, i.e. "myn@me":"my:pwd". But speaking honestly, I'm not satisfied with the current solution, maybe you guys will raise some clever idea.

This comment has been minimized.

@hvlad

hvlad Jan 26, 2019

Member

Hmm... is it setting with value only, no name ?

This comment has been minimized.

@hvlad

hvlad Jan 26, 2019

Member

Ah, i see it below. It is "sync_replica".
Why not break it into 3 parts: login\password\connection string ?

This comment has been minimized.

@dyemanov

dyemanov Jan 27, 2019

Author Member

@hvlad, do you mean three different settings? How to define multiple replicas in this case? Now you can specify:
sync_replica user1:pwd1@/my/first/replica
sync_replica user2:pwd2@/my/second/replica
sync_replica user3:pwd3@/my/third/replica
This may be not very elegant, but with three settings per each replica it's going to be a mess.

This comment has been minimized.

@dyemanov

dyemanov Jan 27, 2019

Author Member

Just as idea, maybe we could use nested sections:
database = /my/db
{
sync_replica = /my/first/replica
{
user = user1
password = pwd1
## may be other connection options
}
}
but I don't know whether such nesting is supported by our config classes.

This comment has been minimized.

@hvlad

hvlad Jan 27, 2019

Member

Exactly, nested sections should work

dyemanov added some commits Jan 24, 2019

@dyemanov dyemanov merged commit 932ca51 into FirebirdSQL:master Jan 25, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@dyemanov

This comment has been minimized.

Copy link
Member Author

dyemanov commented Jan 25, 2019

PR was merged but feel free to post any further reviews here, they will be noted/answered.

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