search

cycle / orm

PHP DataMapper, ORM
https://cycle-orm.dev
MIT License
1.23k stars 71 forks source link

Пулинг соединений для долгоживущих асинхронных приложений #115

Open codercms opened 4 years ago

codercms commented 4 years ago

Привет! Это прекрасная ORM, но немного не понятно, как ее использовать в контексте асинхронных приложений. Речь идет про Swoole/AMPHP/ReactPHP. Хотя речь идет скорее про Swoole, про корутиновую однопоточную модель исполнения.

Асинхронные приложения работают не совсем в констекте 1 запрос = 1 ответ, во время выполнения запроса может понадобиться выполнить какие-нибудь I/O операции, например пойти в БД за какими-нибудь данными. И заместо того, чтобы "зависнуть" на получении данных от БД - асинхронное приложение начинает обрабатывать следующий запрос.

PDO с таким (асинхронным) подходом конечно же несовместим (Swoole умеет делать PDO MySQL неблокирующим, но не PDO PGSQL). Поэтому я написал свой драйвер - https://github.com/makise-co/postgres (вдохновившись драйвером от AMPHP, что является максимально близким по концепции к Swoole) И написал адаптер этого драйвера для пакета spiral/database - https://github.com/makise-co/postgres-spiral-driver

У Postgres есть ограничение, что с 1 соединения нельзя выполнять более 1 запроса одновременно, поэтому встает вопрос о пулинге соединений.

Я не могу понять, как правильно реализовать пулинг в рамках ORM. С одной стороны задача пулить соединения лежит на DBAL, а с другой стороны ORM может хранить у себя какие-то внутренние состояния, которые завязаны на конкретное соединение, которое в асинхронном приложение уже может быть занято обработкой других задач. Плюс, пулить нужно грамотно, например при выполнении транзакций соединение не должно возвращаться в пул, а должно оставаться занятым до тех пор, пока транзакция не будет закоммичена или роллбекнута или возникнет ошибка соединения при работе с транзакцией.

Поэтому у меня возникает вопрос - Где и как правильно реализовать пулинг соединений, чтобы не столкнуться с багами и неправильным поведением?

Вне ORM и database/spiral я использую подход, который именую - LazyConnection. Как и в database/spiral (DriverInterface) у меня есть ConnectionInterface. Структура получается такая:

LazyConnection скрывает под собой работу с пулом соединений (пример можно увидеть тут - https://github.com/makise-co/framework/blob/master/src/Database/Connection/LazyConnection.php)

А сами репозитории, дабы исключить проблему конкурентности запросов каждый раз обращаются к DBAL с просьбой выдать им новый LazyConnection (https://github.com/makise-co/framework/blob/master/src/Database/DatabaseManager.php#L48).

wolfy-j commented 4 years ago

Привет, ответим на след неделе. Извиняюсь за задержку.

codercms commented 4 years ago

@wolfy-j привет! Есть новости?

wolfy-j commented 4 years ago

Привет, из-за событий в стране Тикет потерялся. Внесём на след неделю, тут все сложно.

codercms commented 4 years ago

@wolfy-j удачи вам!

wolfy-j commented 4 years ago

Привет, грамотным местом будет использовать DatabaseInterface и реализовать свой способ работы с драйверами.

https://github.com/spiral/database/blob/master/src/Database.php#L98

Придется заэкспозить transactionLevel из драйвера - https://github.com/spiral/database/blob/master/src/Driver/Driver.php#L79 (можешь сделать ПР для get метода).

И не возращать драйвер в пул пока соединение не закрыто. Возможно, придется также сделать свой TransactionRunner - https://github.com/cycle/orm/blob/master/src/Transaction/Runner.php так как он работает с драйверами напрямую.

codercms commented 3 years ago

@wolfy-j привет! У меня получилось реализовать несколько иначе, без кастомных раннеров и т.д. Запуск транзакции - https://github.com/makise-co/postgres-spiral-driver/blob/master/src/Driver/MakisePostgres/PooledMakisePostgresDriver.php#L261 С ограничением, что транзакции привязаны к конкрутным корутинам, т.е. одна корутина может иметь не более одной активной транзакции. Пример определения - нужно выполнять запрос в транзакции, или на другом коннекте из пула - https://github.com/makise-co/postgres-spiral-driver/blob/master/src/Driver/MakisePostgres/PooledMakisePostgresDriver.php#L535

И у меня возникло несколько вопросов (предложений):

  1. Контракт DriverInterface имеет следующую сигнатуру: public function beginTransaction(string $isolationLevel = null): bool

    Но данный подход вносит ограничения в приложение, т.е. то, с чем я столкнулся выше, я вынужден хранить состояние, т.к. ORM не умеет выполнять запросы на транзакции самостоятельно. Однака, данная ORM позиционируется как подходящая для долгоживущих приложений (designed to work in long-running applications: immutable service core, disposable UoW)

    Так вот, предложение состоит в том - Почему бы не изменить сигнатуру контракта на public function beginTransaction(string $isolationLevel = null): Transaction?

    Тогда бы действительно можно было бы использовать данную ORM в Long-Running Applications без боли и необходимости вручную хранить "транзакции" и писать подобную точку централизации. Так например сделано в GoLang драйверах, а также NodeJS - т.е. везде, где есть пулинг коннектов. Т.е. ORM вызывает beginTransaction и все последующие запросы в рамках данной транзакции выполняет на объекте транзакции.

  2. Для Postgres драйвера используется некий механизм эмуляции lastInsertId. Однако по запросам, которые строит ORM видно, что используется правильный подход с RETURNING id; . И если в драйвере удалить механизм для эмуляции "lastInsertId", то запросы к ORM по типу insert начинают заместо идентификатора возвращать NULL. Можно ли как-то избавиться от эмуляции, чтобы не было такого сайд эффекта?

wolfy-j commented 3 years ago

Привет, мы используем одну корутину в долгоживущих приложениях (смотри модель РР), но идея очень хорошая.

1) Можешь создать тикет в spiral/database, я думаю что это будет новый метод (createTransaction) и его нужно будет хорошо продумать. Подход мне нравится.

2) Можешь описать более подробно? Там все находится в пределах одного запроса и, в теории, не должно ломаться. Единственный момент - PG ищет какой же ключ является первичным (хотя мы сделали чтобы ОРМ сама об этом говорила).

codercms commented 3 years ago

@wolfy-j

  1. Завёл тикет
  2. Если из драйвера убрать функционал связанный с: 
    /**
 * Cached list of primary keys associated with their table names. Used by InsertBuilder to
 * emulate last insert id.
 *
 * @var array
 */
private array $primaryKeys = [];

    То вот этот тест упадет: https://github.com/spiral/database/blob/master/tests/Database/TransactionsTest.php#L204 , так как заместо идентификатора вернется NULL.

codercms commented 3 years ago

@wolfy-j привет! Можешь сказать что-нибудь про п.2 https://github.com/cycle/orm/issues/115#issuecomment-715508476 ?

wolfy-j commented 3 years ago

Привет, сорри заняты новым компонентом очень сильно.

Если это убрать то само собой last insert id будет неопределенным в некоторых случаях. Надо копать, я пока не понимаю как это мешает пуллингу.

codercms commented 3 years ago

@wolfy-j это не особо мешает пулингу, проблема в том, то что на Last insert ID не стоит завязываться. Т.е. вроде как ОРМка строит запрос с RETURNING, а по факту получается, что какой-то кусок кода тянет из lastInsertId IDШник. И если в своем драйвере как бы "выпилить" этот кусок с Last insert ID ломается ORMка, т.к. у сущности заместо ID становится NULL.

wolfy-j commented 3 years ago

В PG драйвере lastInsertID эмулируется через RETURNING, этот список нужен чтобы знать что возвращать. ОРМ же не знает она работает с PG или не с PG.

codercms commented 3 years ago

@wolfy-j ага, теперь понял. Спасибо! А если я захочу запрос где будет не RETURNING id, а RETURNING *, как ORMке объяснить, что нужно подставить все поля, которые вернулись результатом запроса?

wolfy-j commented 3 years ago

Сдалать свою команду, returning не должен переписывать ваши значения.

codercms commented 3 years ago

@wolfy-j все же касательно этого поста https://github.com/cycle/orm/issues/115#issuecomment-728868257 , на самом деле это мешает пулингу из-за конкурентного доступа к данным. Пока одна корутина может читать $this->primaryKeys, другая может вызвать resetPrimarykeys. Я бы все же как-то пересмотрел этот дизайн, т.к. каждый раз это плодит кучу SQL запросов, что безусловно будет дико аффектить производительность приложений. Чтобы хоть как-то стабилизировать поведение и снизить импакт на производительность, мне пришлось воткнуть лок в getPrimaryKey - https://github.com/makise-co/postgres-spiral-driver/blob/master/src/Driver/MakisePostgres/PooledMakisePostgresDriver.php#L453 и лок в resetPrimaryKeys (https://github.com/makise-co/postgres-spiral-driver/blob/master/src/Driver/MakisePostgres/PooledMakisePostgresDriver.php#L490)

wolfy-j commented 3 years ago

Врядли в момент работы базы данные PK изменятся, так что лок это нормальное решение. Я правда не копал как они работают в PHP, но в другом языке я бы сделал так же.

codercms commented 3 years ago

@wolfy-j чтоб не плодить топики, хочу спросить кое-что про ConstrainException, это вроде круто, что можно отловить такое исключение, но само по себе оно ничего не дает. И код обрабатывающий это исключение, должен будет работать c "предыдущим" исключением, т.е. он должен знать конкретную реализацию драйвера, с которой работает. Получается такая некрасивая завязка, т.к. вроде работаешь с ORM, но при этом должен работать с низкоуровневыми исключениями от реализации конкретного драйвера. В большинстве юзкейсов там будет PDOException, но не в моем случае.

Как ты считаешь, а не будет ли хорошей идеей, в ConstrainException собственно добавить такие параметры как SQLSTATE и т.д.? Тогда коду не нужно будет полагаться на более низлежащий слой реализаций конкретных драйверов.

P.S. возможно также стоит рассмотреть вариант не сырым SQLSTATE, а что-то более высокоуровневое, например какой-нибудь enum, а-ля UNIQUE_VIOLATION, FOREIGN_KEY_VIOLATION и т.д.

codercms commented 3 years ago

@wolfy-j чтоб было понятно, мне приходится делать вот так:

$t = new Transaction($this->orm);
$t->persist($entity);

try {
    $t->run();
} catch (ConstrainException $e) {
    $prev = $e->getPrevious();
    if ($prev instanceof QueryExecutionError) {
        $sqlState = $prev->getDiagnostics()['sqlstate'] ?? '';
        if ('23505' === $sqlState) {
            throw new SynonymAlreadyExist($synonym);
        }

        if ('23503' === $sqlState) {
            throw new CompanyNotFoundException($companyId);
        }
    }

    throw $e;
}

Думаю этот пример отлично иллюстрирует проблему

wolfy-j commented 3 years ago

Там уже заложен механизм типизации ошибок, возможно стоит его расширить. Я не против, чем более очевидные ошибки - тем лучше.

wolfy-j commented 3 years ago

@SerafimArts что думаешь?

SerafimArts commented 3 years ago

@wolfy-j у меня недостаточно опыта что бы давать конструктивные комментарии в рамках Цикла.

А на счёт расширения ошибок - вполне разумно, т.к. в PHP try/catch завязан на типы, а не на коды и тем более sqlstate. Так что удобный механизм матчинга их по кодам БД был бы вполне юзабелен. Отличить Not Found от Syntax Error - важный кейс.