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

Context manager to acquire Postgres advisory locks #138

Merged
merged 1 commit into from Nov 2, 2015

Conversation

Projects
None yet
4 participants
@guewen
Member

guewen commented Nov 2, 2015

This Postgres feature is invaluable when dealing with synchronisations,
especially when importing concurrently records from a system.

When we export a record, we are able to acquire a lock on the exported record
to prevent 2 jobs to export it at the same time. This is different when we
import a record for the first time and with several jobs running in parallel,
chances are high that 2 jobs will import the same record at the same moment.

The Postgres advisory lock comes handy there for they allow to acquire an
application lock. Usually we'll acquire the lock at the beginning of an import
(beginning of Importer.run()) and we'll throw a RetryableJobError if
the lock cannot be acquired so the job is retried later. The lock will remain
in place until the end of the transaction.

Example:

  • Job 1 imports Partner A
  • Job 2 imports Partner B
  • Partner A has a category X which happens not to exist yet
  • Partner B has a category X which happens not to exist yet
  • Job 1 import category X as a dependency
  • Job 2 import category X as a dependency

Since both jobs are executed concurrently, they both create a record for category X.
With this lock:

  • Job 1 imports Partner A, it puts a lock for this partner
  • Job 2 imports Partner B, it puts a lock for this partner
  • Partner A has a category X which happens not to exist yet
  • Partner B has a category X which happens not to exist yet
  • Job 1 import category X as a dependency, it puts a lock for this category
  • Job 2 import category X as a dependency, try to put a lock but can't, Job 2
    is retried later, and when it is retried, it sees the category X created by
    Job 1

See http://topopps.com/implementing-postgres-advisory-locks/ for the article
where I learned about the computation of the hash for this purpose.

Context manager to acquire Postgres advisory locks
This Postgres feature is invaluable when dealing with synchronisations,
especially when importing concurrently records from a system.

When we export a record, we are able to acquire a lock on the exported record
to prevent 2 jobs to export it at the same time. This is different when we
import a record for the first time and with several jobs running in parallel,
chances are high that 2 jobs will import the same record at the same moment.

The Postgres advisory lock comes handy there for they allow to acquire an
application lock.  Usually we'll acquire the lock at the beginning of an import
(beginning of ``Importer.run()``) and we'll throw a ``RetryableJobError`` if
the lock cannot be acquired so the job is retried later. The lock will remain
in place until the end of the transaction.

Example:
 - Job 1 imports Partner A
 - Job 2 imports Partner B
 - Partner A has a category X which happens not to exist yet
 - Partner B has a category X which happens not to exist yet
 - Job 1 import category X as a dependency
 - Job 2 import category X as a dependency

Since both jobs are executed concurrently, they both create a record for category X.
With this lock:

 - Job 1 imports Partner A, it puts a lock for this partner
 - Job 2 imports Partner B, it puts a lock for this partner
 - Partner A has a category X which happens not to exist yet
 - Partner B has a category X which happens not to exist yet
 - Job 1 import category X as a dependency, it puts a lock for this category
 - Job 2 import category X as a dependency, try to put a lock but can't, Job 2
   is retried later, and when it is retried, it sees the category X created by
   Job 1

See http://topopps.com/implementing-postgres-advisory-locks/ for the article
where I learned about the computation of the hash for this purpose.
@damdam-s

This comment has been minimized.

Show comment
Hide comment
@damdam-s

damdam-s commented Nov 2, 2015

👍

@lmignon

This comment has been minimized.

Show comment
Hide comment
@lmignon

lmignon Nov 2, 2015

Contributor

@guewen This new functionality looks really great! I already see a lot of cases where I need it. As usual, the code looks fine and comes with tests and documentation.
👍 (Code review)

Contributor

lmignon commented Nov 2, 2015

@guewen This new functionality looks really great! I already see a lot of cases where I need it. As usual, the code looks fine and comes with tests and documentation.
👍 (Code review)

@pedrobaeza

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Nov 2, 2015

Contributor

Another contribution to make the framework more robust

👍

Contributor

pedrobaeza commented Nov 2, 2015

Another contribution to make the framework more robust

👍

pedrobaeza added a commit that referenced this pull request Nov 2, 2015

Merge pull request #138 from guewen/advisory-lock-common
Context manager to acquire Postgres advisory locks

@pedrobaeza pedrobaeza merged commit f68890c into OCA:8.0 Nov 2, 2015

2 checks passed

ci/runbot runbot build 3118818-138-6e2377 (runtime 31s)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
be retried when the lock cannot be acquired.
"""
if pg_try_advisory_lock(self.env, lock):
yield

This comment has been minimized.

@guewen

guewen Nov 2, 2015

Member

retrospectively I wonder if the context manager is a good choice as the lock is acquired until the end of the transaction anyway... Maybe the following is more clear?

def try_advisory_lock(self, lock, retry_seconds=1):
    if not pg_try_advisory_lock(self.env, lock):
        raise RetryableJobError('Could not acquire advisory lock',...)
@guewen

guewen Nov 2, 2015

Member

retrospectively I wonder if the context manager is a good choice as the lock is acquired until the end of the transaction anyway... Maybe the following is more clear?

def try_advisory_lock(self, lock, retry_seconds=1):
    if not pg_try_advisory_lock(self.env, lock):
        raise RetryableJobError('Could not acquire advisory lock',...)

This comment has been minimized.

@guewen

guewen Nov 2, 2015

Member

retrospectively I wonder if the context manager is a good choice as the lock is acquired until the end of the transaction anyway... Maybe the following is more clear?

@lmignon @damdam-s @pedrobaeza opinions?

@guewen

guewen Nov 2, 2015

Member

retrospectively I wonder if the context manager is a good choice as the lock is acquired until the end of the transaction anyway... Maybe the following is more clear?

@lmignon @damdam-s @pedrobaeza opinions?

This comment has been minimized.

@pedrobaeza

pedrobaeza Nov 2, 2015

Contributor

Yeah, it can be clearer, but it isn't also very confused this way. What you prefer.

@pedrobaeza

pedrobaeza Nov 2, 2015

Contributor

Yeah, it can be clearer, but it isn't also very confused this way. What you prefer.

This comment has been minimized.

@lmignon

lmignon Nov 2, 2015

Contributor

Good catch @guewen ! The expected behavior of the contextmanager is to 'scope' the changes to the context to the code enclosed by the with statement. In this case the lock is acquired until the end of the transaction not the end of the enclosed lines.

@lmignon

lmignon Nov 2, 2015

Contributor

Good catch @guewen ! The expected behavior of the contextmanager is to 'scope' the changes to the context to the code enclosed by the with statement. In this case the lock is acquired until the end of the transaction not the end of the enclosed lines.

This comment has been minimized.

@pedrobaeza

pedrobaeza Nov 2, 2015

Contributor

But does it matter? They both end at the same more or less, isn't it?

@pedrobaeza

pedrobaeza Nov 2, 2015

Contributor

But does it matter? They both end at the same more or less, isn't it?

This comment has been minimized.

@guewen

guewen Nov 2, 2015

Member

Exactly what I thought. I'll make a PR tomorrow.

@guewen

guewen Nov 2, 2015

Member

Exactly what I thought. I'll make a PR tomorrow.

@lmignon

This comment has been minimized.

Show comment
Hide comment
@lmignon

lmignon Nov 2, 2015

Contributor

@pedrobaeza IMO yes. With the 'with' statement I expect that the lock is released at the end of the block and it's not the case.

Contributor

lmignon commented Nov 2, 2015

@pedrobaeza IMO yes. With the 'with' statement I expect that the lock is released at the end of the block and it's not the case.

@pedrobaeza

This comment has been minimized.

Show comment
Hide comment
@pedrobaeza

pedrobaeza Nov 2, 2015

Contributor

OK, I understand. @guewen, please go ahead with the PR, please.

Contributor

pedrobaeza commented Nov 2, 2015

OK, I understand. @guewen, please go ahead with the PR, please.

guewen added a commit to guewen/connector-magento that referenced this pull request Nov 3, 2015

guewen added a commit to guewen/connector-magento that referenced this pull request Nov 3, 2015

atchuthan added a commit to sodexis/connector-magento that referenced this pull request May 19, 2016

atchuthan added a commit to sodexis/connector-magento that referenced this pull request Oct 18, 2016

guewen added a commit to guewen/connector-magento that referenced this pull request Jun 20, 2017

blaggacao pushed a commit to xoes-oca/magentoerpconnect that referenced this pull request Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment