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

Fix IsolationLevel.read_comitted and introduce IsolationLevel.default #770

Merged
merged 10 commits into from
Dec 21, 2020

Conversation

Pliner
Copy link
Member

@Pliner Pliner commented Dec 20, 2020

PR introduces IsolationLevel.default and fixes IsolationLevel.read_comitted as it was mentioned by @dvarrazzo.

Are there changes in behavior for the user?

Instead of hacking

        with (await pool.cursor()) as cur:
            # Set the connection in read committed isolation level
            # to avoid serialization failures on file lock
            # https://github.com/aio-libs/aiopg/issues/699
            # You can unlock the aiopg version from setup.py when this hack
            # is no more needed.
            cur._transaction = aiopg.Transaction(
                cur, aiopg.IsolationLevel.read_committed
            )
            yield cur

PR allows to pass IsolationLevel

        with (await pool.cursor(isolation_level=aiopg.IsolationLevel.read_committed)) as cur:
            yield cur

Related issue number

Fixes #699

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Pliner Pliner changed the title Slightly reworked IsolationCompiler and IsolationLevel.default Fix IsolationLevel.read_comitted and introduce IsolationLevel.default Dec 20, 2020
@codecov
Copy link

codecov bot commented Dec 20, 2020

Codecov Report

Merging #770 (a65e032) into master (52532fb) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #770      +/-   ##
==========================================
+ Coverage   93.08%   93.11%   +0.03%     
==========================================
  Files          13       13              
  Lines        1532     1539       +7     
  Branches      172      172              
==========================================
+ Hits         1426     1433       +7     
  Misses         78       78              
  Partials       28       28              
Impacted Files Coverage Δ
aiopg/connection.py 91.86% <100.00%> (ø)
aiopg/cursor.py 100.00% <100.00%> (ø)
aiopg/transaction.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52532fb...a65e032. Read the comment docs.

@Pliner Pliner marked this pull request as ready for review December 20, 2020 13:21

def __init__(self, readonly, deferrable):
if readonly or deferrable:
raise ValueError("Readonly or deferrable are not supported")

Choose a reason for hiding this comment

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

Why readonly and deferrable are not supported? As this object is running BEGIN it should be able to support it, they are independent from the isolation level.

https://www.postgresql.org/docs/current/sql-begin.html

Choose a reason for hiding this comment

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

I think it was right that __slots__ was on the base class and that these properties are available to all the hierarchy

Copy link
Member Author

@Pliner Pliner Dec 20, 2020

Choose a reason for hiding this comment

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

image

I've only refactored current implementation 🙈

Copy link
Member Author

@Pliner Pliner Dec 20, 2020

Choose a reason for hiding this comment

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

Why readonly and deferrable are not supported? As this object is running BEGIN it should be able to support it, they are independent from the isolation level.

I dunno why it was unsupported, let me check

Copy link
Member Author

Choose a reason for hiding this comment

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

image

https://www.postgresql.org/docs/13/sql-set-transaction.html

It looks like readonly supported for any isolation level, but deferred - only for serialisable with readonly.

But I'm a but unsure about current behaviour with raise of an exception.

Choose a reason for hiding this comment

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

You shouldn't impose a rule on top of what postgres implements.

If it's a no-op it may be a no-op only for the current implementation and change in the future. If the database accepts it, it is not the driver's business to decide to raise an exception.

This seems to work: the database doesn't raise exceptions or even warnings:

piro=# begin isolation level read uncommitted deferrable;
BEGIN
*piro=#

so I don't see a reason to special-case it and raise an exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, thanks 🎉

@Pliner Pliner merged commit 72d889b into master Dec 21, 2020
@Pliner Pliner deleted the fix-699 branch December 21, 2020 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The default read committed isolation level is the wrong choice
2 participants