db icon indicating copy to clipboard operation
db copied to clipboard

Split connection into master and slave

Open SamMousa opened this issue 5 years ago • 24 comments

Currently both master and slave connections use the same class.

This violates the SRP. We should have a class that only implements the slave connection part and one class that only implements the master connection.

SamMousa avatar Nov 06 '18 15:11 SamMousa

Hi @SamMousa, if you have time you could develop the idea to implement it.

terabytesoftw avatar Nov 06 '20 17:11 terabytesoftw

And don't forget to use primary/replica.

Mister-42 avatar Nov 06 '20 18:11 Mister-42

That's a big no. I'm not contributing to anything that things inclusion means replacing random words because people get offended.

SamMousa avatar Nov 06 '20 18:11 SamMousa

@SamMousa talking seriously, I've attempted it already and it did't went well so far. Deleted everything. Going to try again later but wouldn't mind you trying. You're way better in designing things than I am.

samdark avatar Nov 06 '20 22:11 samdark

I'll try giving it a go, hopefully within next 2 weeks

SamMousa avatar Nov 06 '20 22:11 SamMousa

Thank you!

samdark avatar Nov 06 '20 22:11 samdark

Btw, I thought Yii3 would use CycleORM? Is that no longer the case?

Also, are we committed to the Command object as it is now? It is terrible from a SRP point of view:

  • It is a value object for the query
  • It is mutable, via mutator functions like batchInsert() that are named as verbs
  • It contains execution logic in execute()
  • It contains public functions like prepare() that I think should be private, since execute() always calls it.

I think it would make more sense to implement a builder like QueryBuilder, but then for commands: CommandBuilder.

What about ConnectionInterface? It contains a lot of getters that indicate the design could be improved. Also, at the heart of the Connection should be some way to execute queries; Schema uses Connection to throw some queries at it to get information about what the database looks like.

Furthermore, there are a lot of references to SQL, but these interfaces were, in Yii2, also used for non-sql. I propose to just use query instead.

Note these are just some random things I noticed and since I haven't been too active I'm not sure what has already been discussed / decided :-)

SamMousa avatar Nov 08 '20 15:11 SamMousa

Btw, I thought Yii3 would use CycleORM? Is that no longer the case?

It's going to be not tied to any DB. Initially we were planning to drop DB overall but then @terabytesoftw convinced us that it's possible to port it and then enhance. Then he did the porting. Likely we'll not tag a stable version of DB anytime soon but eventually we want it released.

Also, are we committed to the Command object as it is now?

Not really.

I think it would make more sense to implement a builder like QueryBuilder, but then for commands: CommandBuilder.

Likely yes. We love Yii 2 query builder syntax. It is super-convenient.

What about ConnectionInterface? It contains a lot of getters that indicate the design could be improved.

Yes. Most of these getters could go into one getDsn(): DsnInterface (or two if we want primary/replica connection info separately).

Furthermore, there are a lot of references to SQL, but these interfaces were, in Yii2, also used for non-sql. I propose to just use query instead.

Agree.

samdark avatar Nov 08 '20 17:11 samdark

Here's some related work:

  • https://github.com/yiisoft/db/pull/196
  • https://github.com/yiisoft/db/pull/195

samdark avatar Nov 08 '20 17:11 samdark

It's going to be not tied to any DB. Initially we were planning to drop DB overall but then @terabytesoftw convinced us that it's possible to port it and then enhance. Then he did the porting. Likely we'll not tag a stable version of DB anytime soon but eventually we want it released.

Ah, in that case I apologize, but reinventing the wheel doesn't make sense to me. I did a lot of research into Cycle after it was chosen and was am really impressed with it. I'll not spend time on a library that I think has no right to exist..

If something is missing from Cycle (or any other abstraction layer) wouldn't it make more sense to contribute there?

I'm sorry; I don't have much time for any type of contributions to open source, so when I do I want to make sure it is something that I feel provides value ;-)

SamMousa avatar Nov 09 '20 08:11 SamMousa

Ah, in that case I apologize, but reinventing the wheel doesn't make sense to me. I did a lot of research into Cycle after it was chosen and was am really impressed with it. I'll not spend time on a library that I think has no right to exist..

It is incorrect to compare Connection with Cycle ORM. In this case, it will be correct to compare Connection with Spiral Database, and Active Record with Cycle ORM. I really like the Spiral Database architecture, and I think it makes more sense to switch to using it rather than rewrite Connection from Yii2.

mj4444ru avatar Nov 12 '20 09:11 mj4444ru

Architecturally it's alright but is missing way too many things:

  1. Oracle support.
  2. Good docs.
  3. Likely named placeholder support (there are tests only for positional ones so I'm not sure named are alright).
  4. Convenient way to fetch scalars.
  5. Batch selection.
  6. Cross-db quoting syntax.
  7. Nested transactions.
  8. R/W split.
  9. JSON and Array.
  10. Independency from commercial projects schedule. For example, right now, Spiral Scout are busy with commercial stuff and won't be able to check PRs till ~January.

samdark avatar Nov 12 '20 10:11 samdark

  1. Sometimes you have to sacrifice something.
  2. This does not prevent the use of Cycle ORM in Yii3.
  3. Named parameters are not compatible with some functions. Perhaps this can be fixed. Cycle ORM does not need it, I think this is the only reason for the absence of named parameters. In general, named parameters in collected queries are a bad approach and a place for errors.
  4. These are easily fixable little things.
  5. I don't understand what it is.
  6. This is a separate feature of the Yii2 framework. It can be ported (https://github.com/spiral/database/blob/33e6db63b86238fedeb697bffb28489f0d0dc2d5/src/Driver/Compiler.php#L40).
  7. As I wrote earlier, nested transactions are supported.
  8. The R/W split implementation in Yii2 has the wrong architecture. The implementation of R/W split in spiral/database is more correct and should be guided by.
  9. JSON in Yii2 doesn't work fine. Array is what?
  10. I think yiisoft/db won't be normal in January 2022 either :)

@terabytesoftw - I understand that the yiisoft / db port is your brainchild, but I would be ashamed of such a brainchild.

mj4444ru avatar Nov 13 '20 09:11 mj4444ru

  1. Sometimes you have to sacrifice something.
  2. This does not prevent the use of Cycle ORM in Yii3.
  3. Named parameters are not compatible with some functions. Perhaps this can be fixed. Cycle ORM does not need it, I think this is the only reason for the absence of named parameters.
  4. These are easily fixable little things.
  5. I don't understand what it is.
  6. This is a separate feature of the Yii2 framework. It can be ported (https://github.com/spiral/database/blob/33e6db63b86238fedeb697bffb28489f0d0dc2d5/src/Driver/Compiler.php#L40).
  7. As I wrote earlier, nested transactions are supported.
  8. The R/W split implementation in Yii2 has the wrong architecture. The implementation of R/W split in spiral/database is more correct and should be guided by.
  9. JSON in Yii2 doesn't work fine. Array is what?
  10. I think yiisoft/db won't be normal in January 2022 either :)

@terabytesoftw - I understand that the yiisoft / db port is your brainchild, but I would be ashamed of such a brainchild.

I think your words are very misplaced, firstly I just transferred the code from yii2 to yii3, which you mean then that the yii2 code does not work and will be used by the new WC3 website, I think it is already taking what personally, which leaves him very bad.

By the way feel free to use any orm, that's your choice.

terabytesoftw avatar Nov 13 '20 09:11 terabytesoftw

I think your words are very misplaced, firstly I just transferred the code from yii2 to yii3, which you mean then that the yii2 code does not work and will be used by the new WC3 website, I think it is already taking what personally, which leaves him very bad.

I write based on my own experience. I think the current architecture is wrong. But even to tidy up the currently working code, it takes a huge effort, which seems to me a waste of time, since the architecture will still be wrong. For example MariaDB is very popular, but Yii2 does not know how to work with its JSON. ENUM and SET are still not supported by Yii2.

mj4444ru avatar Nov 13 '20 09:11 mj4444ru

Actually I have a library for that, I use mariadb and json extensively sam-it/yii2-mariadb

On Fri, 13 Nov 2020, 10:55 mj4444, [email protected] wrote:

I think your words are very misplaced, firstly I just transferred the code from yii2 to yii3, which you mean then that the yii2 code does not work and will be used by the new WC3 website, I think it is already taking what personally, which leaves him very bad.

I write based on my own experience. I think the current architecture is wrong. But even to tidy up the currently working code, it takes a huge effort, which seems to me a waste of time, since the architecture will still be wrong. For example MariaDB is very popular, but Yii2 does not know how to work with its JSON. ENUM and SET are still not supported by Yii2.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/yiisoft/db/issues/24#issuecomment-726666627, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEFRTPBZECBHT5JQ7XE2FTSPT65ZANCNFSM4GCCVPIA .

SamMousa avatar Nov 13 '20 10:11 SamMousa

  1. I don't understand what it is.
  • https://www.yiiframework.com/doc/api/2.0/yii-db-query#batch()-detail
  • https://www.yiiframework.com/doc/api/2.0/yii-db-query#each()-detail

samdark avatar Nov 13 '20 19:11 samdark

The batch selection is actually very suboptimal since the driver actually loads all data in memory. Instead automatic paging on primary keys is faster and more efficient for batch iterating.

SamMousa avatar Nov 13 '20 19:11 SamMousa

For memory efficiency using pure objects is also way more efficient due to the fact that php arrays are very inefficient. Pure objects with properties that get hydrated use significantly less memory

SamMousa avatar Nov 13 '20 19:11 SamMousa

For memory efficiency using pure objects is also way more efficient due to the fact that php arrays are very inefficient. Pure objects with properties that get hydrated use significantly less memory

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = new stdClass();
    for($n = 0; $n < 10; $n++) {
        $el->{"param$n"} = new stdClass();
    }
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

159 Mb / 162 Mb

class Test
{
    public $param0;
    public $param1;
    public $param2;
    public $param3;
    public $param4;
    public $param5;
    public $param6;
    public $param7;
    public $param8;
    public $param9;
}

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = new Test();
    for($n = 0; $n < 10; $n++) {
        $el->{"param$n"} = new stdClass();
    }
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

80 Mb / 86 Mb

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = [];
    for($n = 0; $n < 10; $n++) {
        $el["param$n"] = [];
    }
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

101 Mb / 104 Mb

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = [];
    $el["param0"] = [];
    $el["param1"] = [];
    $el["param2"] = [];
    $el["param3"] = [];
    $el["param4"] = [];
    $el["param5"] = [];
    $el["param6"] = [];
    $el["param7"] = [];
    $el["param8"] = [];
    $el["param9"] = [];
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

71 Mb / 72 Mb

mj4444ru avatar Nov 13 '20 20:11 mj4444ru

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = new stdClass();
    for($n = 0; $n < 10; $n++) {
        $el->{"param$n"} = 1;
    }
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

106 Mb / 108 Mb

class Test
{
    public int $param0;
    public int $param1;
    public int $param2;
    public int $param3;
    public int $param4;
    public int $param5;
    public int $param6;
    public int $param7;
    public int $param8;
    public int $param9;
}

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = new Test();
    for($n = 0; $n < 10; $n++) {
        $el->{"param$n"} = 1;
    }
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

27 Mb / 28 Mb

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = [];
    for($n = 0; $n < 10; $n++) {
        $el["param$n"] = 1;
    }
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

101 Mb / 104 Mb

$array = [];
$elCount = 100000;
for ($i = 0; $i < $elCount; $i++) {
    $el = [];
    $el["param0"] = 1;
    $el["param1"] = 1;
    $el["param2"] = 1;
    $el["param3"] = 1;
    $el["param4"] = 1;
    $el["param5"] = 1;
    $el["param6"] = 1;
    $el["param7"] = 1;
    $el["param8"] = 1;
    $el["param9"] = 1;
    $array[] = $el;
}
echo round(memory_get_peak_usage() / 1024 / 1024) . " Mb / " . round(memory_get_peak_usage(true) / 1024 / 1024) . " Mb\n";

71 Mb / 72 Mb

mj4444ru avatar Nov 13 '20 20:11 mj4444ru

Mostly the key side should be more efficient for objects because keys in arrays are stored as hashes. I imagine that something similar happens for stdclass but not for objects with explicit properties.. I think that aligns with your test results, right?

SamMousa avatar Nov 13 '20 20:11 SamMousa

Also for these tests use SplFixedArray to store your test objects would make the differences more pronounced

SamMousa avatar Nov 13 '20 20:11 SamMousa

Mostly the key side should be more efficient for objects because keys in arrays are stored as hashes. I imagine that something similar happens for stdclass but not for objects with explicit properties.. I think that aligns with your test results, right?

This is probably the case. Here's another interesting result:

    $el = new class() {
        public int $param0;
        public int $param1;
        public int $param2;
        public int $param3;
        public int $param4;
        public int $param5;
        public int $param6;
        public int $param7;
        public int $param8;
        public int $param9;
    };

27 Mb / 28 Mb

for ($i = 0; $i < $elCount; $i++) {
    $el = new SplFixedArray(10);
    for($n = 0; $n < 10; $n++) {
        $el[$n] = 1;
    }
    $array[] = $el;
}

37 Mb / 38 Mb

mj4444ru avatar Nov 13 '20 21:11 mj4444ru