Skip to content

Replication slots #41

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

Merged
merged 6 commits into from
Mar 21, 2018
Merged

Replication slots #41

merged 6 commits into from
Mar 21, 2018

Conversation

zilder
Copy link
Collaborator

@zilder zilder commented Mar 13, 2018

No description provided.

@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #41 into master will increase coverage by 0.24%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   97.02%   97.27%   +0.24%     
==========================================
  Files          16       16              
  Lines        1245     1356     +111     
==========================================
+ Hits         1208     1319     +111     
  Misses         37       37
Impacted Files Coverage Δ
testgres/backup.py 96.92% <100%> (ø) ⬆️
tests/test_simple.py 99.77% <100%> (+0.02%) ⬆️
testgres/consts.py 100% <100%> (ø) ⬆️
testgres/node.py 97.55% <84.21%> (+0.28%) ⬆️
testgres/enums.py 100% <0%> (ø) ⬆️
testgres/connection.py 95% <0%> (+0.17%) ⬆️

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 5bc608e...59dbe42. Read the comment docs.

@funbringer funbringer requested review from ildus and funbringer March 14, 2018 10:14
testgres/node.py Outdated
).format(slot_name)

self.execute(query=query,
dbname=dbname or default_dbname(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't have to explicitly use default_dbname() and such since execute() method is able to handle None args properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree

testgres/node.py Outdated
u"log_statement = {}\n"
u"listen_addresses = '{}'\n"
u"port = {}\n"
u"max_replication_slots = {}\n".format(log_statement,
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO max_replication_slots should be placed under allow_streaming=True, next to max_wal_senders etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree as well

testgres/node.py Outdated
@@ -856,7 +863,24 @@ def backup(self, **kwargs):

return NodeBackup(node=self, **kwargs)

def replicate(self, name=None, **kwargs):
def create_replication_slot(self, slot_name, dbname=None, username=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we allow user to choose his own slot_name? If not, it's better to inline this method.

Copy link
Collaborator Author

@zilder zilder Mar 20, 2018

Choose a reason for hiding this comment

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

If we don't allow this, then it could contradict with logical replication interface (#42) which allows user to choose the subscriber name (which implicitly creates logical replication slot with the same name) voluntarily.

Speaking of inlining, I can call create_replication_slot() from replicate() if slot_name != None so that user won't need to call it explicitly. It would be more convenient for user.

Copy link
Collaborator

@funbringer funbringer Mar 20, 2018

Choose a reason for hiding this comment

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

@zilder You're right, of course. Maybe we'll add a default name generator later.

Speaking of inlining, I can call create_replication_slot() from replicate() if slot_name != None

Sure, that would be nice. It's better to check if slot exists, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's better to check if slot exists, though.

True

@funbringer
Copy link
Collaborator

Please make sure that code won't be rewritten by yapf.

@funbringer funbringer merged commit b4051cf into postgrespro:master Mar 21, 2018
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.

3 participants