8000 [lock] Add store dedicated to postgresql by jderusse · Pull Request #38346 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[lock] Add store dedicated to postgresql #38346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2020
Merged

[lock] Add store dedicated to postgresql #38346

merged 1 commit into from
Oct 7, 2020

Conversation

jderusse
Copy link
Member
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR todo

This PR adds 2 new Stores to the Lock component:

  • PostgreSql
  • InMemory (see the WHY below)

Difference with PDO:

  • This store use the Advisory Locks provided natively by postgresql
  • Don't need to create/maintain a table
  • Native support for Blocking locks
  • Native support for Shared locks

By design the lock is linked to the connection with the database, which imply:

  • the lock can't be serialized and passed to another process (ie. store lock in session). Which is also the case for FlockStore, SemaphoreStore and ZookeeperStore
  • if the connection is cut, the process may not be aware that it loose the lock (think a very long process without performing any request)
  • the PostgreSqlStore couldn't rely on the database only to acquire a lock, because all store sharing the same connection won't be concurrent each other. That's why, I added the InMemory store that prevent concurrency within the same process.

@jderusse jderusse changed the title Add support for postgresql in lock [lock] Add store dedicated to postgresql Sep 29, 2020
stof
stof reviewed Sep 29, 2020
View reviewed changes
@nicolas-grekas nicolas-grekas added this to the next milestone Oct 1, 2020
@nicolas-grekas
Copy link
Member

the PostgreSqlStore couldn't rely on the database only to acquire a lock, because all store sharing the same connection won't be concurrent each other. That's why, I added the InMemory store that prevent concurrency within the same process.

I'm not sure I understand what you mean here. Is concurrency within the same process really an issue? E.g. flock() doesn't block when locking the same file twice in the same process.

@jderusse
Copy link
Member Author
jderusse commented Oct 1, 2020

Is concurrency within the same process really an issue? E.g. flock() doesn't block when locking the same file twice in the same process.

This is a design choice that allows concurrency within the same process.
It allows using symfony/lock in various "async" cases

  • react-php
  • guzzle/promise
  • symfony/http-client

All Store test extends an LockAbstractTest that assert this.

The php flock function does not block the same handle but can lock different handle on the same file in the same process: https://3v4l.org/69WBC

@GromNaN
Copy link
Member
GromNaN commented Oct 2, 2020

This is the kind of feature I proposed when I made a PR to add a MysqlStore that uses GET_LOCK. It's interesting to read how you changed your mind @jderusse #25578 (comment). Having a lock tied to the db connection is a good alternative to flock for distributed systems ; but does not meet the need for locks that needs to be persisted (and serialized).

@fabpot
Copy link
Member
fabpot commented Oct 3, 2020

@GromNaN I'm the one who suggested this feature to @jderusse

jderusse
jderusse commented Oct 3, 2020
View reviewed changes
src/Symfony/Component/Lock/Store/StoreFactory.php
@@ -97,8 +97,16 @@ public static function createStore($connection)
case 0 === strpos($connection, 'sqlite3://'):
return new PdoStore($connection);

case 0 === strpos($connection, 'pgsql+advisory:'):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the name of scheme.
If you have suggestion

@jderusse
Copy link
Member Author
jderusse commented Oct 3, 2020

I'm truly sorry if I offended you with my comment @GromNaN.

Time flies, and since Decembre 2017, many things changed:

  • first of all I changed. I don't stick to my ideas just to pretend I'm always right. When I was wrong, I admit it.
  • about serialization: Lock component now has several Store that don't provide serialization (zookeeper).
  • about connection-lost: all stores has drawbacks (Memcached can reboot, Redis can evict lock when clock drift). It's not a dead-end, but have to be documented.
  • Lock component now provide an adapter for MySQL that provides serialization (PDO) for people who need it.
  • pg_advisory enhance PDO by natively providing shared lock and blocking lock
  • [Lock] Add MysqlStore that use GET_LOCK #25578 didn't provide concurrency within the same process. This could be fixed with the MemoryStore from this PR

@GromNaN
Copy link
Member
GromNaN commented Oct 3, 2020

Thank you for taking time to reply. I was not offended. On the contrary I'm satisfied by the PdoStore as you made it. I like that all implementations can be provided in Symfony with their pros and cons. Again thank you and go ahead.

@fabpot fabpot changed the base branch from master to 5.x October 7, 2020 06:05
@fabpot
Copy link
Member
fabpot commented Oct 7, 2020

Thank you @jderusse.

@fabpot fabpot mentioned this pull request Oct 7, 2020
Closed
@fabpot fabpot merged commit 5cc4bc9 into symfony:5.x Oct 7, 2020
@jderusse jderusse mentioned this pull request Oct 7, 2020
Merged
fabpot added a commit that referenced this pull request Oct 7, 2020
@fabpot
bug #38453 [lock] Mark Key unserializable whith PgsqlStore (jderusse)
414a5ab 
This PR was merged into the 5.x branch.

Discussion
----------

[lock] Mark Key unserializable whith PgsqlStore

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | /
| License       | MIT
| Doc PR        | /

Marks key unserializable #38395 with the new PgsqlStore #38346

Commits
-------

eb934e9 Mark Key unserializable whith PgsqlStore
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 7, 2020
@fabpot fabpot mentioned this pull request Oct 14, 2020
Merged
@jderusse jderusse deleted the lock-psql branch October 15, 2020 10:01
@GromNaN GromNaN mentioned this pull request Feb 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.

< 3E25 div data-show-on-forbidden-error hidden>

Uh oh!

There was an error while loading. Please reload this page.

6 participants
@jderusse @nicolas-grekas @GromNaN @fabpot @stof @carsonbot
0