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-19T16:06:40Z

testgres/node.py

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


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-19T16:06:40Z

testgres/node.py

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


Maybe change pubname to name?

funbringer · 2018-03-19T16:06:40Z

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:06:40Z

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:06:40Z

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:06:40Z

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

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


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:30:15Z

testgres/api.py

@@ -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,)]


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:30:15Z

testgres/node.py

@@ -413,6 +417,7 @@ def default_conf(self,
                     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:30:15Z

testgres/pubsub.py

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


Could we replace safe_psql with execute?

Yep, I overlooked it

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

tests/test_simple.py

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


Nice catch!

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

The patch looks good!

zilder added 4 commits Mar 16, 2018

Logical replication

ca5b546

Skip logical replication test on PostgreSQL versions below 10

bb01c7d

Fix logical replication for python3

782484b

Merge branch 'master' into logical

Loading status checks…

f8b95c6

funbringer requested review from ildus and funbringer Mar 19, 2018

funbringer requested changes Mar 19, 2018

View changes

zilder mentioned this pull request Mar 20, 2018

Replication slots #41

Merged

zilder added 6 commits Mar 22, 2018

Some minor refactoring of logical replication

b1cba73

Added options_string() func

954879a

Merge branch 'master' into logical

Loading status checks…

0741c70

Minor refactoring

Loading status checks…

f4e0bd0

Add failing logical replication test for 9.6

Loading status checks…

f48623b

Added test for Publication.add_tables()

Loading status checks…

138c6cc

funbringer reviewed Mar 26, 2018

View changes

zilder added 3 commits Mar 26, 2018

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

Loading status checks…

f652bf4

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

Additional subscription catchup test

Loading status checks…

bc1002f

Some refactoring of logical replication API and formatting

Loading status checks…

08ed6ef

funbringer reviewed Apr 4, 2018

View changes

ildus reviewed Apr 4, 2018

View changes

zilder added 2 commits May 31, 2018

minor refactoring

4b279ef

Merge branch 'master' into logical

Loading status checks…

d60cdcb

funbringer approved these changes May 31, 2018

View changes

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

Loading status checks…

50e02ff

postgrespro / testgres

Logical replication support #42

Logical replication support #42

zilder commented Mar 19, 2018

codecov-io commented Mar 19, 2018 •

edited

funbringer left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

funbringer commented May 31, 2018

postgrespro / testgres

Join GitHub today

GitHub is where the world builds software

Logical replication support #42

Logical replication support #42

Conversation

zilder commented Mar 19, 2018

codecov-io commented Mar 19, 2018 • edited

Codecov Report

funbringer left a comment

This comment has been minimized.

funbringer Mar 19, 2018 Collaborator

This comment has been minimized.

zilder Mar 22, 2018 Author Collaborator

This comment has been minimized.

funbringer Mar 19, 2018 Collaborator

This comment has been minimized.

funbringer Mar 19, 2018 Collaborator

This comment has been minimized.

funbringer Mar 19, 2018 Collaborator

This comment has been minimized.

funbringer Mar 19, 2018 Collaborator

This comment has been minimized.

funbringer Mar 19, 2018 Collaborator

This comment has been minimized.

funbringer Mar 26, 2018 Collaborator

This comment has been minimized.

zilder May 31, 2018 Author Collaborator

This comment has been minimized.

funbringer Apr 4, 2018 Collaborator

This comment has been minimized.

zilder May 31, 2018 Author Collaborator

This comment has been minimized.

funbringer Apr 4, 2018 Collaborator

This comment has been minimized.

funbringer Apr 4, 2018 Collaborator

This comment has been minimized.

zilder May 31, 2018 Author Collaborator

This comment has been minimized.

funbringer Apr 4, 2018 Collaborator

This comment has been minimized.

ildus Apr 4, 2018 Collaborator

This comment has been minimized.

zilder May 31, 2018 Author Collaborator

This comment has been minimized.

ildus Apr 4, 2018 Collaborator

This comment has been minimized.

zilder May 31, 2018 Author Collaborator

This comment has been minimized.

funbringer May 31, 2018 Collaborator

This comment has been minimized.

zilder Jun 1, 2018 Author Collaborator

This comment has been minimized.

funbringer May 31, 2018 Collaborator

This comment has been minimized.

zilder Jun 1, 2018 Author Collaborator

This comment has been minimized.

funbringer Jun 1, 2018 Collaborator

This comment has been minimized.

funbringer May 31, 2018 Collaborator

This comment has been minimized.

zilder Jun 1, 2018 Author Collaborator

This comment has been minimized.

funbringer May 31, 2018 Collaborator

funbringer commented May 31, 2018

Essential cookies

Always active

Analytics cookies

codecov-io commented Mar 19, 2018 •

edited

funbringer Mar 19, 2018
Collaborator

zilder Mar 22, 2018
Author Collaborator

funbringer Mar 19, 2018
Collaborator

funbringer Mar 19, 2018
Collaborator

funbringer Mar 19, 2018
Collaborator

funbringer Mar 19, 2018
Collaborator

funbringer Mar 19, 2018
Collaborator

funbringer Mar 26, 2018
Collaborator

zilder May 31, 2018
Author Collaborator

funbringer Apr 4, 2018
Collaborator

zilder May 31, 2018
Author Collaborator

funbringer Apr 4, 2018
Collaborator

funbringer Apr 4, 2018
Collaborator

zilder May 31, 2018
Author Collaborator

funbringer Apr 4, 2018
Collaborator

ildus Apr 4, 2018
Collaborator

zilder May 31, 2018
Author Collaborator

ildus Apr 4, 2018
Collaborator

zilder May 31, 2018
Author Collaborator

funbringer May 31, 2018
Collaborator

zilder Jun 1, 2018
Author Collaborator

funbringer May 31, 2018
Collaborator

zilder Jun 1, 2018
Author Collaborator

funbringer Jun 1, 2018
Collaborator

funbringer May 31, 2018
Collaborator

zilder Jun 1, 2018
Author Collaborator

funbringer May 31, 2018
Collaborator