Skip to content
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

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

Open
codercms opened this issue Aug 8, 2020 · 21 comments
Labels
type:enhancement Enhancement. type:question Further information is requested

Comments

@codercms
Copy link

codercms commented Aug 8, 2020

Привет! Это прекрасная 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.
Структура получается такая:

  • ConnectionInterface
    • PostgresConnection
    • AnyOtherDatabaseConnection
    • LazyConnection

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

wolfy-j commented Aug 14, 2020

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

@codercms
Copy link
Author

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

@wolfy-j
Copy link
Contributor

wolfy-j commented Aug 21, 2020

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

@codercms
Copy link
Author

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

@wolfy-j
Copy link
Contributor

wolfy-j commented Aug 26, 2020

Привет, грамотным местом будет использовать 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 так как он работает с драйверами напрямую.

@wolfy-j wolfy-j added the type:question Further information is requested label Aug 28, 2020
@codercms
Copy link
Author

codercms commented Oct 21, 2020

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

wolfy-j commented Oct 22, 2020

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

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

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

@codercms
Copy link
Author

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

@wolfy-j привет! Можешь сказать что-нибудь про п.2 #115 (comment) ?

@wolfy-j
Copy link
Contributor

wolfy-j commented Nov 17, 2020

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

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

@codercms
Copy link
Author

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

@wolfy-j
Copy link
Contributor

wolfy-j commented Nov 17, 2020

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

@codercms
Copy link
Author

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

@wolfy-j
Copy link
Contributor

wolfy-j commented Nov 17, 2020

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

@codercms
Copy link
Author

@wolfy-j все же касательно этого поста #115 (comment) , на самом деле это мешает пулингу из-за конкурентного доступа к данным. Пока одна корутина может читать $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
Copy link
Contributor

wolfy-j commented Nov 19, 2020

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

@codercms
Copy link
Author

codercms commented Nov 19, 2020

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

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

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

@codercms
Copy link
Author

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

wolfy-j commented Nov 23, 2020

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

@wolfy-j
Copy link
Contributor

wolfy-j commented Nov 23, 2020

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

@SerafimArts
Copy link
Contributor

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement. type:question Further information is requested
Projects
Status: Backlog
Development

No branches or pull requests

4 participants