Logical replication support #42

zilder · 2018-03-19T15:26:12Z

Added publication/subscription feature. Typical use case:

node1 = get_new_node().init(allow_logical=True).start()
node2 = get_new_node().init().start()
node1.safe_psql('create table test (a int)')
node2.safe_psql('create table test (a int)')
pub = node1.publish('mypub')
sub = node2.subscribe(pub, 'mysub')
node1.safe_psql('insert into test values (1), (2), (3)')
sub.catchup()
node2.execute('select * from test')
> [(1,), (2,), (3,)]

codecov-io · 2018-03-19T15:33:59Z

Codecov Report

Merging #42 into master will decrease coverage by 0.01%.
The diff coverage is 97.84%.

@@            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

Impacted Files	Coverage Δ
testgres/connection.py	`95.08% <100%> (+0.08%)`	⬆️
tests/test_simple.py	`99.42% <100%> (-0.36%)`	⬇️
testgres/consts.py	`100% <100%> (ø)`	⬆️
testgres/utils.py	`95.37% <100%> (+0.13%)`	⬆️
testgres/node.py	`96.93% <94.73%> (+0.06%)`	⬆️
testgres/pubsub.py	`95.91% <95.91%> (ø)`

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 221df4f...50e02ff. Read the comment docs.

funbringer

The code looks good, but there's a few things that definitely could be improved.

funbringer · 2018-03-19T15:53:45Z

testgres/node.py

                         raise_programming_error=True,
-                         raise_internal_error=True):
+                         raise_internal_error=True,
+                         zero_rows_is_ok=False):


Isn't zero_rows_is_ok=True effectively equal to expected=None?

In Subscription.catchup() we are expecting True and at the same time it may happen that there is no rows at the moment (until statistics collected).

funbringer · 2018-03-19T15:55:06Z

testgres/node.py

            raise_from(CatchUpException("Failed to catch up", poll_lsn), e)

+    def publish(self,
+                pubname,


Maybe change pubname to name?

funbringer · 2018-03-19T15:57:50Z

testgres/node.py

+            dbname: database name where objects or interest are located
+            username: replication username
+        """
+        return Publication(pubname, self, tables, dbname, username)


Could you please use keyword args instead of positional args?

funbringer · 2018-03-19T16:02:28Z

testgres/pubsub.py

+
+        # connection info
+        conninfo = (
+            u"dbname={} user={} host={} port={}"


I think we could extract common conninfo-related code from _create_recovery_conf() into a function and add it to utils.py. What do you think?

funbringer · 2018-03-19T16:04:29Z

testgres/pubsub.py

+                query=query,
+                dbname=self.pub.dbname,
+                username=username or self.pub.username,
+                max_attempts=60,


IMHO it's better to replace max_attempts=60 and zero_rows_is_ok=True with kwargs.

funbringer · 2018-03-19T16:05:36Z

testgres/pubsub.py

+                            dbname=dbname,
+                            username=username)
+
+    def close(self, dbname=None, username=None):


Shouldn't it be called drop instead?

funbringer · 2018-03-26T10:51:18Z

tests/test_simple.py


+            # check 0 rows equivalent to expected=None
+            node.poll_query_until(
+                query='select * from pg_class where true = false', expected=None)


pg_class should be schema-qualified.

… the Sphinx templates and module header was added as well.

funbringer · 2018-04-04T09:30:48Z

testgres/pubsub.py

+        # 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)


All occurrences of safe_psql should be replaced with execute for better performance.

Done. However I had to refactor PostgresNode.execute() method a little bit. Some statements (such as CREATE/DROP SUBSCRIPTION) require non-transactional environment but execute would start transaction automatically. So I used autocommit option which allows to run that kind of queries.

funbringer · 2018-04-04T09:33:46Z

testgres/pubsub.py

+
+class Subscription(object):
+    def __init__(self,
+                 name,


It's better to place name right after the mandatory args. Chances are we'll come up with some name generation facility, but we won't be able to make name optional without moving it, which is something I'd like to avoid.

funbringer · 2018-04-04T09:35:45Z

testgres/pubsub.py

+        constructing subscription objects.
+
+        Args:
+            name: subscription name


I've also noticed that some doc strings end with commas while others don't.

funbringer · 2018-04-04T09:37:00Z

testgres/pubsub.py

+                query=query,
+                dbname=self.pub.dbname,
+                username=username or self.pub.username,
+                max_attempts=60)


Personally, I don't like the hard-coded number.

I think it should be a parameter here.

Ok, added LOGICAL_REPL_MAX_CATCHUP_ATTEMPTS parameter

ildus · 2018-04-04T09:42:20Z

testgres/pubsub.py

+After that :meth:`~.PostgresNode.publish()` and :meth:`~.PostgresNode.subscribe()`
+methods may be used to setup replication. Example:
+
+>>> from .api import get_new_node


This should be changed to from testgres import get_new_node. Documentation should contain working code, even it would require remove doctest.

funbringer · 2018-05-31T18:24:52Z

testgres/api.py

 ...             print(replica.execute('postgres', 'select count(*) from test'))
 PostgresNode(name='...', port=..., base_dir='...')
-[(3,)]
+[(3L,)]


is this necessary?

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.

funbringer · 2018-05-31T18:26:18Z

testgres/node.py

                     fsync=False,
                     unix_sockets=True,
                     allow_streaming=True,
+                     allow_logical=False,


Why don't we enable this by default?

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.

funbringer · 2018-05-31T18:28:28Z

testgres/pubsub.py

+        """
+        Drop publication
+        """
+        self.node.safe_psql(


Could we replace safe_psql with execute?

Yep, I overlooked it

funbringer · 2018-05-31T18:29:55Z

tests/test_simple.py

            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')


Nice catch!

funbringer · 2018-05-31T18:30:45Z

The patch looks good!

zilder added 4 commits March 16, 2018 18:47

Logical replication

ca5b546

Skip logical replication test on PostgreSQL versions below 10

bb01c7d

Fix logical replication for python3

782484b

Merge branch 'master' into logical

f8b95c6

funbringer requested review from funbringer and ildus March 19, 2018 15:48

funbringer requested changes Mar 19, 2018

View reviewed changes

zilder mentioned this pull request Mar 20, 2018

Replication slots #41

Merged

zilder added 6 commits March 22, 2018 15:25

Some minor refactoring of logical replication

b1cba73

Added options_string() func

954879a

Merge branch 'master' into logical

0741c70

Minor refactoring

f4e0bd0

Add failing logical replication test for 9.6

f48623b

Added test for Publication.add_tables()

138c6cc

funbringer reviewed Mar 26, 2018

View reviewed changes

zilder added 3 commits March 26, 2018 19:49

Merge branch 'master' into logical. Also testgres.pubsub was added to…

f652bf4

… the Sphinx templates and module header was added as well.

Additional subscription catchup test

bc1002f

Some refactoring of logical replication API and formatting

08ed6ef

funbringer reviewed Apr 4, 2018

View reviewed changes

ildus reviewed Apr 4, 2018

View reviewed changes

zilder added 2 commits May 31, 2018 16:49

minor refactoring

4b279ef

Merge branch 'master' into logical

d60cdcb

funbringer approved these changes May 31, 2018

View reviewed changes

change safe_psql() call to execute() in pubsub.py

50e02ff

funbringer merged commit 0583873 into postgrespro:master Jun 1, 2018

Uh oh!

Logical replication support #42

Logical replication support #42

Uh oh!

Conversation

zilder commented Mar 19, 2018

Uh oh!

codecov-io commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

funbringer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

funbringer commented May 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Mar 19, 2018 •

edited

Loading