Pg schema support #159

ghettovoice · 2018-05-25T06:50:55Z

This pull request adds support of multi schema events stores and projection for PostresSQL implementation #157 .
Summary (only Pg related classes):

Event streams table and projections table name can be defined with custom DB schema.
String before first dot in stream name treated as DB schema, so stream auto generated table will be created in custom schema.
I added additional tests to clarify the new behaviour

prolic

I really like this idea. Can we have this for mysql / mariadb as well?

prolic · 2018-05-25T09:41:52Z

src/Util/PostgresHelper.php

+     */
+    private function quoteIdent(string $tableName): string
+    {
+        return array_reduce(explode('.', $tableName), function ($result, $part) {


that's really hard to read, can we simplify that?

prolic · 2018-05-25T09:42:00Z

src/Util/PostgresHelper.php

+     */
+    private function extractSchema(string $identifier): ?string
+    {
+        $parts = array_map(function ($part) {


prolic · 2018-05-25T09:42:44Z

tests/Assets/scripts/postgres/01_custom_event_streams_table.sql

@@ -0,0 +1,12 @@
+CREATE SCHEMA IF NOT EXISTS custom;


I don't know if "custom" is a good default, why not "prooph" 😄

ghettovoice · 2018-05-25T09:53:26Z

Thank you for review! I'll fix it.
As I know in MySQL/MariaDB schema represents another database and probably with another connection configuration. Currently I don't know how to deal with this. May be connection pool or something else..

prolic · 2018-05-25T11:43:24Z

You're right, thanks. So no worries. Can you also please make sure that existing app configurations are not broken and the use of additional schema is optional?

…

On Fri, May 25, 2018, 17:53 Vladimir Vershinin ***@***.***> wrote: Thank you for review! I'll fix it. As I know in MySQL/MariaDB schema represents another database and probably with another connection configuration. Currently I don't know how to deal with this — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvIx7me5fkeHKNDIn1Mc5-LWltpazks5t19SYgaJpZM4UNdJd> .

ghettovoice · 2018-05-25T12:38:16Z

Can you also please make sure that existing app configurations are not
broken and the use of additional schema is optional?

Tables for event streams and projections will be created in custom schema only if $eventStreamsTable, $projectionsTable and stream name will contain dot and only for Postgres driver. I highly doubt anyone is using a point in these variables intentionally right now, so there should't be problems.

prolic · 2018-05-25T15:01:18Z

May I asked why it's a requirement to have schemas in your project?

ghettovoice · 2018-05-25T15:06:09Z

Sure, it just an our internal convention: not to create all at public schema (pg extensions usually creates here functions, tables and etc.), each module adds tables, functions and etc in it's own schema, so it easy to search in relatively large db

prolic · 2018-05-25T20:24:36Z

👍 I'll recheck this PR this weekend.

ghettovoice · 2018-05-25T21:20:21Z

Ok , thank you

coveralls · 2018-05-26T00:05:42Z

Coverage increased (+0.2%) to 90.4% when pulling c3e925c on ghettovoice:pg-schema-support into af6d698 on prooph:master.

prolic · 2018-05-28T18:05:13Z

I'll have to make some more testing, I don't like this quoting method, I hope to come up with a faster solution. Currently really busy.

prolic · 2018-05-30T08:28:42Z

@ghettovoice I think I found a much faster quoting-function, see https://3v4l.org/7bCYi

ghettovoice · 2018-05-30T08:46:46Z

@prolic I see 👍
My implementation uses two functions because of needless schema extraction in generateTableName
method e1b9a7b#diff-6895a5b565f06c33a1404ad8257df13e.
May be here should be another solution for persistence strategy?

prolic · 2018-05-30T08:50:35Z

Can you try to work this out yourself? I have time to check this maybe the weekend. Sascha-Oliver Prolic 2018-05-30 16:46 GMT+08:00 Vladimir Vershinin <[email protected]>:

…

@prolic <https://github.com/prolic> I see 👍 My implementation uses two functions because of needless schema extraction in generateTableName method e1b9a7b#diff-6895a5b565f06c33a1404ad8257df13e <e1b9a7b#diff-6895a5b565f06c33a1404ad8257df13e> . May be here should be another solution for persistence strategy? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#159 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYEvNmnOHiZ60l-JoBwwpzKH3s7TrNsks5t3lx4gaJpZM4UNdJd> .

ghettovoice · 2018-05-30T09:10:53Z

Ok, I'll try to make this before weekends

re-implement `quoteIdent` and `splitIdent` according to https://3v4l.org/7bCYi and https://3v4l.org/QcCCl to increase quoting performance.

prolic · 2018-06-03T20:13:42Z

src/PersistenceStrategy/PostgresAggregateStreamStrategy.php

-            '_' . sha1($streamName->toString()),
-        ]));
+        if ($schema) {
+            $schema . '.' . $table;


That doesn't make any sense

prolic · 2018-06-03T20:13:55Z

src/PersistenceStrategy/PostgresSimpleStreamStrategy.php

-            '_' . sha1($streamName->toString()),
-        ]));
+        if ($schema) {
+            $schema . '.' . $table;


prolic · 2018-06-03T20:14:18Z

src/PersistenceStrategy/PostgresSingleStreamStrategy.php

-            '_' . sha1($streamName->toString()),
-        ]));
+        if ($schema) {
+            $schema . '.' . $table;


and again, maybe you should add a unit test for this method

Oh, sorry! You are right. I thought that the test already exists

- fix generateTableName method in Postgres related persistence strategy classes - add tests for generateTableName method

ghettovoice · 2018-06-04T07:59:40Z

Now it's done. Yesterday I made it late at night

prolic · 2018-06-04T14:31:47Z

src/Util/PostgresHelper.php

+     * @param string $ident
+     * @return string[]
+     */
+    private function splitIdent(string $ident): array


On reviewing I found that the returned table name is not used at all. So why not change the signature then? Something like private function schema(string $ident): ?string. It either returns the schema or null

There was private function extractSchema(string $ident): ?string in the very first version.
I'll fix this

… - an simplified extraction of pg schema from string

prolic

GOOD WORK! thanks you

ghettovoice · 2018-06-04T16:42:29Z

Thank you for your time!

Vladimir Vershinin and others added 6 commits May 24, 2018 18:10

add support of custom schema tables in Pg Event Store

52faed2

add support of custom schema tables in Pg Projections

5326e19

fix broken tests with MySQL/MariaDB, incorrect table name quoting

59954bf

fix broken tests

1bceb67

fix broken test: correct custom schema removing

7b90c70

apply prooph code style

f1ed945

ghettovoice force-pushed the pg-schema-support branch from c82a3c0 to f1ed945 Compare May 25, 2018 09:41

prolic requested changes May 25, 2018

View reviewed changes

review fixes: rename custom to prooph

dfe3f78

simplify PostgresHelper::quoteIdent helper

e1b9a7b

prolic self-assigned this May 25, 2018

prolic added the enhancement label May 25, 2018

PostgresHelper::quoteIdent optimization

e26a2be

re-implement `quoteIdent` and `splitIdent` according to https://3v4l.org/7bCYi and https://3v4l.org/QcCCl to increase quoting performance.

prolic reviewed Jun 3, 2018

View reviewed changes

Postgres persistence strategies

9611fff

- fix generateTableName method in Postgres related persistence strategy classes - add tests for generateTableName method

prolic reviewed Jun 4, 2018

View reviewed changes

refactore PostgresHelper::splitIdent to PostgresHelper::extractSchema…

c3e925c

… - an simplified extraction of pg schema from string

prolic approved these changes Jun 4, 2018

View reviewed changes

prolic merged commit 2eb70f0 into prooph:master Jun 4, 2018

ghettovoice deleted the pg-schema-support branch June 5, 2018 18:22

jsor mentioned this pull request Oct 11, 2019

Column length of event_streams.stream_name might be too short #215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pg schema support #159

Pg schema support #159

ghettovoice commented May 25, 2018

prolic left a comment

prolic May 25, 2018

ghettovoice May 25, 2018

prolic May 25, 2018

ghettovoice May 25, 2018

prolic May 25, 2018

ghettovoice May 25, 2018

ghettovoice commented May 25, 2018 •

edited

Loading

prolic commented May 25, 2018 via email

ghettovoice commented May 25, 2018

prolic commented May 25, 2018

ghettovoice commented May 25, 2018 •

edited

Loading

prolic commented May 25, 2018

ghettovoice commented May 25, 2018

coveralls commented May 26, 2018 •

edited

Loading

prolic commented May 28, 2018

prolic commented May 30, 2018

ghettovoice commented May 30, 2018

prolic commented May 30, 2018 via email

ghettovoice commented May 30, 2018

prolic Jun 3, 2018

prolic Jun 3, 2018

prolic Jun 3, 2018

ghettovoice Jun 4, 2018

ghettovoice commented Jun 4, 2018

prolic Jun 4, 2018

ghettovoice Jun 4, 2018

prolic left a comment

ghettovoice commented Jun 4, 2018

Pg schema support #159

Pg schema support #159

Conversation

ghettovoice commented May 25, 2018

prolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghettovoice commented May 25, 2018 • edited Loading

prolic commented May 25, 2018 via email

ghettovoice commented May 25, 2018

prolic commented May 25, 2018

ghettovoice commented May 25, 2018 • edited Loading

prolic commented May 25, 2018

ghettovoice commented May 25, 2018

coveralls commented May 26, 2018 • edited Loading

prolic commented May 28, 2018

prolic commented May 30, 2018

ghettovoice commented May 30, 2018

prolic commented May 30, 2018 via email

ghettovoice commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghettovoice commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic left a comment

Choose a reason for hiding this comment

ghettovoice commented Jun 4, 2018

ghettovoice commented May 25, 2018 •

edited

Loading

ghettovoice commented May 25, 2018 •

edited

Loading

coveralls commented May 26, 2018 •

edited

Loading