- Notifications
You must be signed in to change notification settings - Fork35
Logical replication support#42
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedMar 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #42 +/- ##==========================================- Coverage 97.01% 96.99% -0.02%========================================== Files 16 17 +1 Lines 1405 1529 +124 ==========================================+ Hits 1363 1483 +120- Misses 42 46 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The code looks good, but there's a few things that definitely could be improved.
testgres/node.py Outdated
| @@ -836,7 +847,8 @@ def poll_query_until(self, | |||
| expected=True, | |||
| commit=True, | |||
| raise_programming_error=True, | |||
| raise_internal_error=True): | |||
| raise_internal_error=True, | |||
| zero_rows_is_ok=False): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Isn'tzero_rows_is_ok=True effectively equal toexpected=None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
InSubscription.catchup() we are expecting True and at the same time it may happen that there is no rows at the moment (until statistics collected).
testgres/node.py Outdated
| @@ -987,6 +1004,41 @@ def catchup(self, dbname=None, username=None): | |||
| except Exception as e: | |||
| raise_from(CatchUpException("Failed to catch up", poll_lsn), e) | |||
| def publish(self, | |||
| pubname, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe changepubname toname?
testgres/node.py Outdated
| dbname: database name where objects or interest are located | ||
| username: replication username | ||
| """ | ||
| return Publication(pubname, self, tables, dbname, username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you please use keyword args instead of positional args?
testgres/pubsub.py Outdated
| # connection info | ||
| conninfo = ( | ||
| u"dbname={} user={} host={} port={}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we could extract commonconninfo-related code from_create_recovery_conf() into a function and add it toutils.py. What do you think?
testgres/pubsub.py Outdated
| query=query, | ||
| dbname=self.pub.dbname, | ||
| username=username or self.pub.username, | ||
| max_attempts=60, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO it's better to replacemax_attempts=60 andzero_rows_is_ok=True with kwargs.
testgres/pubsub.py Outdated
| dbname=dbname, | ||
| username=username) | ||
| def close(self, dbname=None, username=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't it be calleddrop instead?
tests/test_simple.py Outdated
| @@ -462,6 +532,10 @@ def test_poll_query_until(self): | |||
| node.poll_query_until( | |||
| query='create table def()', expected=None) # returns nothing | |||
| # check 0 rows equivalent to expected=None | |||
| node.poll_query_until( | |||
| query='select * from pg_class where true = false', expected=None) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
pg_class should be schema-qualified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
… the Sphinx templates and module header was added as well.
testgres/pubsub.py Outdated
| # create publication in database | ||
| t = "table " + ", ".join(tables) if tables else "all tables" | ||
| query = "create publication {} for {}" | ||
| node.safe_psql(query.format(name, t), dbname=dbname, username=username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
All occurrences ofsafe_psql should be replaced withexecute for better performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done. However I had to refactorPostgresNode.execute() method a little bit. Some statements (such asCREATE/DROP SUBSCRIPTION) require non-transactional environment butexecute would start transaction automatically. So I usedautocommit option which allows to run that kind of queries.
testgres/pubsub.py Outdated
| class Subscription(object): | ||
| def __init__(self, | ||
| name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's better to placename right after the mandatory args. Chances are we'll come up with some name generation facility, but we won't be able to makename optional without moving it, which is something I'd like to avoid.
testgres/pubsub.py Outdated
| constructing subscription objects. | ||
| Args: | ||
| name: subscription name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've also noticed that some doc strings end with commas while others don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed
testgres/pubsub.py Outdated
| query=query, | ||
| dbname=self.pub.dbname, | ||
| username=username or self.pub.username, | ||
| max_attempts=60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Personally, I don't like the hard-coded number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it should be a parameter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, addedLOGICAL_REPL_MAX_CATCHUP_ATTEMPTS parameter
testgres/pubsub.py Outdated
| After that :meth:`~.PostgresNode.publish()` and :meth:`~.PostgresNode.subscribe()` | ||
| methods may be used to setup replication. Example: | ||
| >>> from .api import get_new_node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be changed tofrom testgres import get_new_node. Documentation should contain working code, even it would require remove doctest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree
testgres/api.py Outdated
| @@ -28,7 +28,7 @@ | |||
| ... replica.catchup() # wait until changes are visible | |||
| ... print(replica.execute('postgres', 'select count(*) from test')) | |||
| PostgresNode(name='...', port=..., base_dir='...') | |||
| [(3,)] | |||
| [(3L,)] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it was when I tested with doctest. But now when I ran doctest it showed[(3,)] so I am confused :) Will change it back then as it shouldn't be part of this pull request anyway.
| @@ -413,6 +417,7 @@ def default_conf(self, | |||
| fsync=False, | |||
| unix_sockets=True, | |||
| allow_streaming=True, | |||
| allow_logical=False, | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why don't we enable this by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because it is not supported on postgres versions below 10 and there is specific message when someone's trying to enable this feature on those versions. Besides it produces extra WAL data and hence could work slightly slower.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, i see.
testgres/pubsub.py Outdated
| """ | ||
| Drop publication | ||
| """ | ||
| self.node.safe_psql( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could we replacesafe_psql withexecute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, I overlooked it
| # check 0 columns | ||
| with self.assertRaises(QueryException): | ||
| node.poll_query_until(query='select from pg_class limit 1') | ||
| node.poll_query_until( | ||
| query='select from pg_catalog.pg_class limit 1') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice catch!
The patch looks good! |
Added publication/subscription feature. Typical use case: